From 2c5b4484de1d809a95d216c8075668e11e24db7a Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:18:04 +0800 Subject: [PATCH 01/34] fix(devices): emit structured JSON for command --dry-run --json (bug #36) --- src/commands/devices.ts | 19 +++++++++++++++ tests/commands/devices.test.ts | 43 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/commands/devices.ts b/src/commands/devices.ts index 76a672d5..1bba0a9d 100644 --- a/src/commands/devices.ts +++ b/src/commands/devices.ts @@ -25,6 +25,7 @@ import { registerExplainCommand } from './explain.js'; import { registerExpandCommand } from './expand.js'; import { registerDevicesMetaCommand } from './device-meta.js'; import { isDryRun } from '../utils/flags.js'; +import { DryRunSignal } from '../api/client.js'; export function registerDevicesCommand(program: Command): void { const COMMAND_TYPES = ['command', 'customize'] as const; @@ -371,6 +372,10 @@ Examples: $ switchbot devices command unlock --yes `) .action(async (deviceIdArg: string | undefined, cmdArg: string | undefined, parameter: string | undefined, options: { name?: string; nameStrategy?: string; nameType?: string; nameCategory?: 'physical' | 'ir'; nameRoom?: string; type: string; yes?: boolean; idempotencyKey?: string }) => { + // Declared outside try so the DryRunSignal catch branch can reference them. + let _deviceId: string | undefined; + let _cmd: string | undefined; + let _parsedParam: unknown; try { // BUG-FIX: When --name is provided, Commander fills positionals left-to-right // starting at [deviceId]. Shift them back to their semantic slots. @@ -404,6 +409,7 @@ Examples: category: options.nameCategory, room: options.nameRoom, }); + _deviceId = deviceId; if (!getCachedDevice(deviceId)) { console.error( `Note: device ${deviceId} is not in the local cache — run 'switchbot devices list' first to enable command validation.`, @@ -513,6 +519,9 @@ Examples: // keep as string } } + // Capture for DryRunSignal catch branch (which runs after executeCommand throws). + _cmd = cmd; + _parsedParam = parsedParam; const body = await executeCommand( deviceId, @@ -558,6 +567,16 @@ Examples: // Re-throw mock process.exit signals (Vitest intercepts process.exit as thrown // Error('__exit__')) so they aren't double-handled and the exit code is preserved. if (error instanceof Error && error.message === '__exit__') throw error; + if (error instanceof DryRunSignal) { + const commandType = (options.type ?? 'command') as string; + const wouldSend = { deviceId: _deviceId, command: _cmd, parameter: _parsedParam, commandType }; + if (isJsonMode()) { + printJson({ dryRun: true, wouldSend }); + } else { + console.log(`[dry-run] Would POST devices/${_deviceId}/commands with ${JSON.stringify({ command: _cmd, parameter: _parsedParam, commandType })}`); + } + return; + } handleError(error); } }); diff --git a/tests/commands/devices.test.ts b/tests/commands/devices.test.ts index 231b8d29..c844fbf8 100644 --- a/tests/commands/devices.test.ts +++ b/tests/commands/devices.test.ts @@ -2061,4 +2061,47 @@ describe('devices command', () => { ); }); }); + + // ===================================================================== + // command — dry-run structured output (bug #36) + // ===================================================================== + describe('command — dry-run output', () => { + const DRY_ID = 'DRY-DEV-1'; + + beforeEach(() => { + // Make post throw DryRunSignal (simulates --dry-run interceptor) + apiMock.__instance.post.mockImplementation(async () => { + throw new apiMock.DryRunSignal('POST', `/v1.1/devices/${DRY_ID}/commands`); + }); + }); + + it('emits structured JSON with dryRun:true when --dry-run --json', async () => { + const res = await runCli(registerDevicesCommand, [ + '--dry-run', '--json', 'devices', 'command', DRY_ID, 'turnOff', + ]); + expect(res.exitCode).toBeNull(); + // post was called (and threw DryRunSignal — that's the mechanism), but the API + // result was never used; we verify the structured output instead. + expect(apiMock.__instance.post).toHaveBeenCalledTimes(1); + // stdout must have valid JSON + const out = res.stdout.join('\n'); + expect(out).toBeTruthy(); + const parsed = JSON.parse(out); + expect(parsed.schemaVersion).toBe('1.1'); + expect(parsed.data.dryRun).toBe(true); + expect(parsed.data.wouldSend.deviceId).toBe(DRY_ID); + expect(parsed.data.wouldSend.command).toBe('turnOff'); + expect(parsed.data.wouldSend.commandType).toBe('command'); + }); + + it('emits human-readable dry-run message to stdout when --dry-run (no --json)', async () => { + const res = await runCli(registerDevicesCommand, [ + '--dry-run', 'devices', 'command', DRY_ID, 'turnOn', + ]); + expect(res.exitCode).toBeNull(); + const out = res.stdout.join('\n'); + expect(out).toMatch(/dry-run/i); + expect(out).toContain(DRY_ID); + }); + }); }); From 7fae5c2bf3e31e97547725591a8c0b8f83f4844e Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:26:21 +0800 Subject: [PATCH 02/34] fix(mcp): preserve structured error metadata in tool responses (bug #38) MCP tool-call errors were collapsing to a plain-text message through the SDK's generic error wrapper, losing subKind / transient / hint / retryAfterMs / errorClass / retryable. Agents had to parse English strings to branch on device-offline vs auth-failed. Enrich mcpError() to emit the full ErrorPayload shape under structuredContent.error. Add apiErrorToMcpError() helper that routes any thrown error through buildErrorPayload(). Wire into the three tool handlers that previously rethrew (send_command, describe_device) or ran unprotected (run_scene). Tests: 3 new MCP tests asserting structuredContent.error shape for ApiError codes 161 / 401 / 190 through each of the three tools. --- src/commands/mcp.ts | 44 +++++++++++++++++--- tests/commands/mcp.test.ts | 84 +++++++++++++++++++++++++++++++++++++- 2 files changed, 122 insertions(+), 6 deletions(-) diff --git a/src/commands/mcp.ts b/src/commands/mcp.ts index 1c32cb2d..47991081 100644 --- a/src/commands/mcp.ts +++ b/src/commands/mcp.ts @@ -4,7 +4,7 @@ import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; import { z } from 'zod'; import { intArg, stringArg } from '../utils/arg-parsers.js'; -import { handleError, isJsonMode } from '../utils/output.js'; +import { handleError, isJsonMode, buildErrorPayload } from '../utils/output.js'; import { VERSION } from '../version.js'; import { fetchDeviceList, @@ -50,18 +50,48 @@ function mcpError( kind: McpErrorKind, code: number, message: string, - options?: { hint?: string; retryable?: boolean; context?: Record }, + options?: { + hint?: string; + retryable?: boolean; + context?: Record; + subKind?: string; + errorClass?: string; + transient?: boolean; + retryAfterMs?: number; + }, ) { const obj: Record = { code, kind, message }; if (options?.hint) obj.hint = options.hint; if (options?.retryable) obj.retryable = true; if (options?.context) obj.context = options.context; + if (options?.subKind !== undefined) obj.subKind = options.subKind; + if (options?.errorClass !== undefined) obj.errorClass = options.errorClass; + if (options?.transient !== undefined) obj.transient = options.transient; + if (options?.retryAfterMs !== undefined) obj.retryAfterMs = options.retryAfterMs; return { isError: true as const, content: [{ type: 'text' as const, text: JSON.stringify({ error: obj }, null, 2) }], + structuredContent: { error: obj }, }; } +/** + * Convert any thrown error into a structured MCP tool-error response, + * preserving all ErrorPayload fields (subKind, transient, hint, etc.). + */ +function apiErrorToMcpError(err: unknown) { + const payload = buildErrorPayload(err); + return mcpError(payload.kind, payload.code, payload.message, { + hint: payload.hint, + retryable: payload.retryable, + context: payload.context, + subKind: payload.subKind, + errorClass: payload.errorClass, + transient: payload.transient, + retryAfterMs: payload.retryAfterMs, + }); +} + export function createSwitchBotMcpServer(options?: { eventManager?: EventSubscriptionManager }): McpServer { const eventManager = options?.eventManager; const server = new McpServer( @@ -400,7 +430,7 @@ API docs: https://github.com/OpenWonderLabs/SwitchBotAPI`, }, }); } - throw err; + return apiErrorToMcpError(err); } const isIr = getCachedDevice(deviceId)?.category === 'ir'; const structured: { @@ -460,7 +490,11 @@ API docs: https://github.com/OpenWonderLabs/SwitchBotAPI`, structuredContent: structured, }; } - await executeScene(sceneId); + try { + await executeScene(sceneId); + } catch (err) { + return apiErrorToMcpError(err); + } const structured = { ok: true as const, sceneId }; return { content: [{ type: 'text', text: JSON.stringify(structured, null, 2) }], @@ -578,7 +612,7 @@ API docs: https://github.com/OpenWonderLabs/SwitchBotAPI`, context: { deviceId }, }); } - throw err; + return apiErrorToMcpError(err); } } ); diff --git a/tests/commands/mcp.test.ts b/tests/commands/mcp.test.ts index f48106e4..3acf57ad 100644 --- a/tests/commands/mcp.test.ts +++ b/tests/commands/mcp.test.ts @@ -17,9 +17,21 @@ const apiMock = vi.hoisted(() => { vi.mock('../../src/api/client.js', () => ({ createClient: apiMock.createClient, ApiError: class ApiError extends Error { - constructor(message: string, public readonly code: number) { + public readonly retryable: boolean; + public readonly hint?: string; + public readonly retryAfterMs?: number; + public readonly transient: boolean; + constructor( + message: string, + public readonly code: number, + meta: { retryable?: boolean; hint?: string; retryAfterMs?: number; transient?: boolean } = {} + ) { super(message); this.name = 'ApiError'; + this.retryable = meta.retryable ?? false; + this.hint = meta.hint; + this.retryAfterMs = meta.retryAfterMs; + this.transient = meta.transient ?? false; } }, DryRunSignal: class DryRunSignal extends Error { @@ -60,6 +72,7 @@ vi.mock('../../src/devices/cache.js', () => ({ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; import { createSwitchBotMcpServer } from '../../src/commands/mcp.js'; +import { ApiError } from '../../src/api/client.js'; /** Connect a fresh server + client pair and return both. */ async function pair() { @@ -393,4 +406,73 @@ describe('mcp server', () => { fs.rmSync(tmpHome, { recursive: true, force: true }); } }); + + // --------------------------------------------------------------------------- + // Bug #38: structured error metadata preserved in MCP tool responses + // --------------------------------------------------------------------------- + + it('send_command preserves structured error metadata on ApiError (code 161 device-offline)', async () => { + cacheMock.map.set('BLE1', { type: 'Bot', name: 'BLE Bot', category: 'physical' }); + // Mock the POST to throw a device-offline ApiError + apiMock.__instance.post.mockRejectedValueOnce( + new ApiError('Device offline (check Wi-Fi / Bluetooth connection)', 161, { transient: false }) + ); + const { client } = await pair(); + + const res = await client.callTool({ + name: 'send_command', + arguments: { deviceId: 'BLE1', command: 'turnOn' }, + }); + + expect(res.isError).toBe(true); + const sc = (res as { structuredContent?: unknown }).structuredContent as + | { error?: { code?: number; subKind?: string; transient?: boolean; hint?: string } } + | undefined; + expect(sc?.error?.code).toBe(161); + expect(sc?.error?.subKind).toBe('device-offline'); + expect(sc?.error?.transient).toBe(false); + expect(sc?.error?.hint).toMatch(/Hub/); + // content[0].text must still be a JSON string (backwards compat) + const text = (res.content as Array<{ type: string; text: string }>)[0].text; + expect(() => JSON.parse(text)).not.toThrow(); + }); + + it('describe_device preserves structured error metadata on ApiError (code 401 auth-failed)', async () => { + // Mock the GET (fetchDeviceList inside describeDevice) to throw auth error + apiMock.__instance.get.mockRejectedValueOnce( + new ApiError('Authentication failed', 401, { transient: false, retryable: false }) + ); + const { client } = await pair(); + + const res = await client.callTool({ + name: 'describe_device', + arguments: { deviceId: 'ANY1' }, + }); + + expect(res.isError).toBe(true); + const sc = (res as { structuredContent?: unknown }).structuredContent as + | { error?: { subKind?: string; errorClass?: string } } + | undefined; + expect(sc?.error?.subKind).toBe('auth-failed'); + expect(sc?.error?.errorClass).toBe('api'); + }); + + it('run_scene preserves structured error metadata on ApiError (code 190 device-busy)', async () => { + // Mock the POST (executeScene) to throw device-busy ApiError + apiMock.__instance.post.mockRejectedValueOnce( + new ApiError('Device internal error', 190, { transient: false }) + ); + const { client } = await pair(); + + const res = await client.callTool({ + name: 'run_scene', + arguments: { sceneId: 'SCENE1' }, + }); + + expect(res.isError).toBe(true); + const sc = (res as { structuredContent?: unknown }).structuredContent as + | { error?: { subKind?: string } } + | undefined; + expect(sc?.error?.subKind).toBe('device-busy'); + }); }); From b5806bd4cca0848052f980f5b25b6acc1f13c12b Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:31:23 +0800 Subject: [PATCH 03/34] fix(mcp): narrow mcpError option types to ErrorSubKind / errorClass union Follow-up to 7fae5c2. subKind and errorClass were typed as wide strings, losing compile-time catches for typos at direct call sites. Import the canonical types from utils/output and reuse them. --- src/commands/mcp.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/commands/mcp.ts b/src/commands/mcp.ts index 47991081..3cb15052 100644 --- a/src/commands/mcp.ts +++ b/src/commands/mcp.ts @@ -4,7 +4,7 @@ import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; import { z } from 'zod'; import { intArg, stringArg } from '../utils/arg-parsers.js'; -import { handleError, isJsonMode, buildErrorPayload } from '../utils/output.js'; +import { handleError, isJsonMode, buildErrorPayload, type ErrorPayload, type ErrorSubKind } from '../utils/output.js'; import { VERSION } from '../version.js'; import { fetchDeviceList, @@ -54,8 +54,8 @@ function mcpError( hint?: string; retryable?: boolean; context?: Record; - subKind?: string; - errorClass?: string; + subKind?: ErrorSubKind; + errorClass?: NonNullable; transient?: boolean; retryAfterMs?: number; }, From ad5bfc3bfc64db7288807d40d548fcbaec22a91f Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:36:13 +0800 Subject: [PATCH 04/34] fix(batch): propagate verification and subKind for IR devices (bug #28) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit devices batch was stripping subKind and verification from per-device results — the very IR unverifiability signal 2.4.0 introduced. An 8-device AC batch would emit zero unverifiability signal. Mirror the single-device IR annotation in BatchResult.succeeded[] and add summary.unverifiableCount aggregate. Tests: 2 new cases covering IR (attached) vs physical (absent). --- src/commands/batch.ts | 45 +++++++++++++++++++++------- tests/commands/batch.test.ts | 57 ++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/src/commands/batch.ts b/src/commands/batch.ts index 8ecfcdb2..dbe8531a 100644 --- a/src/commands/batch.ts +++ b/src/commands/batch.ts @@ -12,7 +12,7 @@ import { createClient } from '../api/client.js'; import { parseFilter, applyFilter, FilterSyntaxError } from '../utils/filter.js'; import { isDryRun } from '../utils/flags.js'; import { DryRunSignal } from '../api/client.js'; -import { getCachedTypeMap } from '../devices/cache.js'; +import { getCachedTypeMap, getCachedDevice } from '../devices/cache.js'; interface BatchStepTiming { startedAt: string; @@ -22,7 +22,16 @@ interface BatchStepTiming { } interface BatchResult { - succeeded: Array<{ deviceId: string; result: unknown } & BatchStepTiming>; + succeeded: Array<{ + deviceId: string; + result: unknown; + subKind?: 'ir-no-feedback'; + verification?: { + verifiable: false; + reason: string; + suggestedFollowup: string; + }; + } & BatchStepTiming>; failed: Array<{ deviceId: string; error: ErrorPayload } & BatchStepTiming>; summary: { total: number; @@ -30,6 +39,7 @@ interface BatchResult { failed: number; skipped: number; durationMs: number; + unverifiableCount: number; dryRun?: boolean; schemaVersion?: string; maxConcurrent?: number; @@ -241,7 +251,7 @@ Examples: const out: BatchResult = { succeeded: [], failed: [], - summary: { total: 0, ok: 0, failed: 0, skipped: 0, durationMs: 0 }, + summary: { total: 0, ok: 0, failed: 0, skipped: 0, durationMs: 0, unverifiableCount: 0 }, }; if (isJsonMode()) printJson(out); else console.log('No devices matched — nothing to do.'); @@ -416,14 +426,26 @@ Examples: }>; const result: BatchResult = { - succeeded: succeeded.map((s) => ({ - deviceId: s.deviceId, - result: s.result, - startedAt: s.startedAt, - finishedAt: s.finishedAt, - durationMs: s.durationMs, - replayed: s.replayed, - })), + succeeded: succeeded.map((s) => { + const isIr = getCachedDevice(s.deviceId)?.category === 'ir'; + const entry: BatchResult['succeeded'][number] = { + deviceId: s.deviceId, + result: s.result, + startedAt: s.startedAt, + finishedAt: s.finishedAt, + durationMs: s.durationMs, + replayed: s.replayed, + }; + if (isIr) { + entry.subKind = 'ir-no-feedback'; + entry.verification = { + verifiable: false, + reason: 'IR transmission is unidirectional; no receipt acknowledgment is possible.', + suggestedFollowup: 'Confirm visible change manually or via a paired state sensor.', + }; + } + return entry; + }), failed: failed.map((f) => ({ deviceId: f.deviceId, error: f.error, @@ -437,6 +459,7 @@ Examples: failed: failed.length, skipped: dryRunned.length, durationMs: Date.now() - startedAt, + unverifiableCount: succeeded.filter((s) => getCachedDevice(s.deviceId)?.category === 'ir').length, schemaVersion: '1.1', maxConcurrent: concurrency, staggerMs, diff --git a/tests/commands/batch.test.ts b/tests/commands/batch.test.ts index 2f22fb56..5605e494 100644 --- a/tests/commands/batch.test.ts +++ b/tests/commands/batch.test.ts @@ -401,4 +401,61 @@ describe('devices batch', () => { expect(parsed.data.plan.stepCount).toBe(2); expect(parsed.data.plan.steps.map((s: { deviceId: string }) => s.deviceId).sort()).toEqual(['BOT1', 'BOT2']); }); + + it('bug28: batch over IR devices attaches subKind + verification and sets summary.unverifiableCount', async () => { + cacheMock.map.set('IR1', { type: 'Air Conditioner', name: 'Living Room AC', category: 'ir' }); + cacheMock.map.set('IR2', { type: 'Air Conditioner', name: 'Bedroom AC', category: 'ir' }); + apiMock.__instance.post.mockResolvedValue({ data: { statusCode: 100, body: {} } }); + + const result = await runCli(registerDevicesCommand, [ + '--json', + 'devices', + 'batch', + 'turnOff', + '--ids', + 'IR1,IR2', + ]); + + expect(result.exitCode).toBeNull(); + const parsed = JSON.parse(result.stdout[0]); + expect(parsed.data.summary.ok).toBe(2); + expect(parsed.data.summary.unverifiableCount).toBe(2); + + for (const s of parsed.data.succeeded) { + expect(s.subKind).toBe('ir-no-feedback'); + expect(s.verification).toBeDefined(); + expect(s.verification.verifiable).toBe(false); + expect(s.verification.reason).toBe( + 'IR transmission is unidirectional; no receipt acknowledgment is possible.' + ); + expect(s.verification.suggestedFollowup).toBe( + 'Confirm visible change manually or via a paired state sensor.' + ); + } + }); + + it('bug28: batch over physical devices does NOT attach subKind/verification and unverifiableCount is 0', async () => { + cacheMock.map.set('BOT1', { type: 'Bot', name: 'Kitchen', category: 'physical' }); + cacheMock.map.set('BOT2', { type: 'Bot', name: 'Office', category: 'physical' }); + apiMock.__instance.post.mockResolvedValue({ data: { statusCode: 100, body: {} } }); + + const result = await runCli(registerDevicesCommand, [ + '--json', + 'devices', + 'batch', + 'turnOn', + '--ids', + 'BOT1,BOT2', + ]); + + expect(result.exitCode).toBeNull(); + const parsed = JSON.parse(result.stdout[0]); + expect(parsed.data.summary.ok).toBe(2); + expect(parsed.data.summary.unverifiableCount).toBe(0); + + for (const s of parsed.data.succeeded) { + expect(s.subKind).toBeUndefined(); + expect(s.verification).toBeUndefined(); + } + }); }); From bebc1d7003dec248e096a92e33c52b4d5a9fa2ab Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:42:55 +0800 Subject: [PATCH 05/34] fix(cache): scope devices/status cache by active profile (bug #37) Rotating credentials or switching profiles was still serving the prior session's inventory because devices.json lived at a fixed disk path. In multi-tenant automation this looked like account-A data bleeding into account-B. When an active profile is set, cache files now live under ~/.switchbot/cache//{devices,status}.json. Unnamed/default profile keeps the legacy ~/.switchbot/devices.json path for backwards compatibility. --config-path override unchanged. Tests: 4 new cases covering default, scoped, per-profile isolation, and status-cache parity. Also fixes name-resolver.test.ts partial flags mock to include getProfile (required by getActiveProfile). --- src/devices/cache.ts | 29 ++++- tests/devices/cache-scoping.test.ts | 171 ++++++++++++++++++++++++++++ tests/utils/name-resolver.test.ts | 2 +- 3 files changed, 197 insertions(+), 5 deletions(-) create mode 100644 tests/devices/cache-scoping.test.ts diff --git a/src/devices/cache.ts b/src/devices/cache.ts index e428ef09..88dad9fa 100644 --- a/src/devices/cache.ts +++ b/src/devices/cache.ts @@ -1,9 +1,30 @@ import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; +import { createHash } from 'node:crypto'; import { getConfigPath } from '../utils/flags.js'; - -/** GC cutoff for status entries: evict anything older than this. */ +import { getActiveProfile } from '../lib/request-context.js'; + +/** + * Returns the directory where cache files should be stored. + * + * - If a profile is active, scopes into a per-profile sub-directory so that + * rotating credentials or switching profiles never serves stale inventory + * from a prior session (Bug #37). + * - If no profile is active (unnamed / default), returns `baseDir` unchanged + * so the existing legacy path (~/.switchbot/devices.json) is preserved. + * + * Only called when `getConfigPath()` returns undefined — the --config-path + * override takes full precedence and bypasses this helper entirely. + */ +function scopedCacheDir(baseDir: string): string { + const profile = getActiveProfile(); + if (profile === undefined) return baseDir; + const hash = createHash('sha256').update(profile).digest('hex').slice(0, 8); + const dir = path.join(baseDir, 'cache', hash); + if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true }); + return dir; +} const DEFAULT_STATUS_GC_TTL_MS = 24 * 60 * 60 * 1000; // 24 h export interface CachedDevice { @@ -48,7 +69,7 @@ function cacheFilePath(): string { const override = getConfigPath(); const dir = override ? path.dirname(path.resolve(override)) - : path.join(os.homedir(), '.switchbot'); + : scopedCacheDir(path.join(os.homedir(), '.switchbot')); return path.join(dir, 'devices.json'); } @@ -205,7 +226,7 @@ function statusCacheFilePath(): string { const override = getConfigPath(); const dir = override ? path.dirname(path.resolve(override)) - : path.join(os.homedir(), '.switchbot'); + : scopedCacheDir(path.join(os.homedir(), '.switchbot')); return path.join(dir, 'status.json'); } diff --git a/tests/devices/cache-scoping.test.ts b/tests/devices/cache-scoping.test.ts new file mode 100644 index 00000000..1f73d32e --- /dev/null +++ b/tests/devices/cache-scoping.test.ts @@ -0,0 +1,171 @@ +/** + * Tests for per-profile cache scoping (Bug #37). + * + * Each test: + * - Redirects os.homedir() to a fresh tmpdir so no real ~/.switchbot is touched. + * - Sets/clears the active profile via process.argv or withRequestContext. + * - Verifies that the file created on disk is at the expected scoped path. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; +import os from 'node:os'; +import { createHash } from 'node:crypto'; + +import { + updateCacheFromDeviceList, + setCachedStatus, + resetListCache, + resetStatusCache, +} from '../../src/devices/cache.js'; +import { withRequestContext } from '../../src/lib/request-context.js'; + +let tmpDir: string; + +const sampleBody = { + deviceList: [{ deviceId: 'DEV-1', deviceName: 'Bot', deviceType: 'Bot' }], + infraredRemoteList: [], +}; + +function sha8(profile: string): string { + return createHash('sha256').update(profile).digest('hex').slice(0, 8); +} + +beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sbcli-scoping-')); + vi.spyOn(os, 'homedir').mockReturnValue(tmpDir); + // Start each test with no profile flag and clean argv + process.argv = ['node', 'switchbot']; + resetListCache(); + resetStatusCache(); +}); + +afterEach(() => { + vi.restoreAllMocks(); + resetListCache(); + resetStatusCache(); + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +// ── a. No profile (default) ───────────────────────────────────────────────── + +describe('cache scoping — no profile (legacy path)', () => { + it('writes devices.json to the legacy ~/.switchbot/devices.json path', () => { + // No profile set — process.argv has no --profile flag + updateCacheFromDeviceList(sampleBody); + + const legacy = path.join(tmpDir, '.switchbot', 'devices.json'); + expect(fs.existsSync(legacy)).toBe(true); + + // The scoped sub-directory must NOT exist + const scopedDir = path.join(tmpDir, '.switchbot', 'cache'); + expect(fs.existsSync(scopedDir)).toBe(false); + }); +}); + +// ── b. Named profile → scoped path ────────────────────────────────────────── + +describe('cache scoping — named profile "alpha"', () => { + it('writes devices.json under ~/.switchbot/cache//devices.json', () => { + const expected = path.join(tmpDir, '.switchbot', 'cache', sha8('alpha'), 'devices.json'); + + withRequestContext({ profile: 'alpha' }, () => { + updateCacheFromDeviceList(sampleBody); + }); + + expect(fs.existsSync(expected)).toBe(true); + + // Legacy path must NOT have been created + const legacy = path.join(tmpDir, '.switchbot', 'devices.json'); + expect(fs.existsSync(legacy)).toBe(false); + }); +}); + +// ── c. Different profiles → different directories, no cross-contamination ─── + +describe('cache scoping — profile isolation', () => { + it('alpha and beta get separate directories; switching profile is a cache miss', () => { + // Write as "alpha" + withRequestContext({ profile: 'alpha' }, () => { + updateCacheFromDeviceList(sampleBody); + }); + + resetListCache(); + + // Read as "beta" — should be a cache miss (null) + const result = withRequestContext({ profile: 'beta' }, () => { + // loadCache is imported inside the module; we test the side-effect: + // after writing for alpha, writing for beta creates a separate file + updateCacheFromDeviceList({ + deviceList: [{ deviceId: 'DEV-2', deviceName: 'Plug', deviceType: 'Plug' }], + infraredRemoteList: [], + }); + return fs.existsSync(path.join(tmpDir, '.switchbot', 'cache', sha8('beta'), 'devices.json')); + }); + + expect(result).toBe(true); + + // Alpha's file must still exist independently + const alphaFile = path.join(tmpDir, '.switchbot', 'cache', sha8('alpha'), 'devices.json'); + expect(fs.existsSync(alphaFile)).toBe(true); + + // Alpha's content must be the original write, not beta's + const alphaCache = JSON.parse(fs.readFileSync(alphaFile, 'utf-8')); + expect(alphaCache.devices['DEV-1']).toBeDefined(); + expect(alphaCache.devices['DEV-2']).toBeUndefined(); + }); +}); + +// ── d. Status cache parity ─────────────────────────────────────────────────── + +describe('cache scoping — status cache follows the same rule', () => { + it('no profile → status.json at legacy ~/.switchbot/status.json', () => { + setCachedStatus('DEV-1', { power: 'on' }); + + const legacy = path.join(tmpDir, '.switchbot', 'status.json'); + expect(fs.existsSync(legacy)).toBe(true); + + const scopedDir = path.join(tmpDir, '.switchbot', 'cache'); + expect(fs.existsSync(scopedDir)).toBe(false); + }); + + it('profile "alpha" → status.json at ~/.switchbot/cache//status.json', () => { + const expected = path.join(tmpDir, '.switchbot', 'cache', sha8('alpha'), 'status.json'); + + withRequestContext({ profile: 'alpha' }, () => { + setCachedStatus('DEV-1', { power: 'on' }); + }); + + expect(fs.existsSync(expected)).toBe(true); + + const legacy = path.join(tmpDir, '.switchbot', 'status.json'); + expect(fs.existsSync(legacy)).toBe(false); + }); +}); + +// ── e. --config-path override takes precedence (profile is ignored) ────────── + +describe('cache scoping — --config-path override', () => { + it('uses config override dirname regardless of active profile', () => { + const custom = path.join(tmpDir, 'alt', 'cfg.json'); + fs.mkdirSync(path.dirname(custom), { recursive: true }); + process.argv = ['node', 'switchbot', '--config', custom]; + resetListCache(); + resetStatusCache(); + + withRequestContext({ profile: 'alpha' }, () => { + updateCacheFromDeviceList(sampleBody); + }); + + // Must land in the override dir, not the scoped dir + const overrideFile = path.join(tmpDir, 'alt', 'devices.json'); + expect(fs.existsSync(overrideFile)).toBe(true); + + // Neither legacy nor scoped paths should exist + const legacy = path.join(tmpDir, '.switchbot', 'devices.json'); + const scoped = path.join(tmpDir, '.switchbot', 'cache', sha8('alpha'), 'devices.json'); + expect(fs.existsSync(legacy)).toBe(false); + expect(fs.existsSync(scoped)).toBe(false); + }); +}); diff --git a/tests/utils/name-resolver.test.ts b/tests/utils/name-resolver.test.ts index 2e1d062b..f010acc8 100644 --- a/tests/utils/name-resolver.test.ts +++ b/tests/utils/name-resolver.test.ts @@ -5,7 +5,7 @@ import { resolveDeviceId } from '../../src/utils/name-resolver.js'; import { updateCacheFromDeviceList, resetListCache } from '../../src/devices/cache.js'; import { saveDeviceMeta } from '../../src/devices/device-meta.js'; -vi.mock('../../src/utils/flags.js', () => ({ getConfigPath: () => undefined })); +vi.mock('../../src/utils/flags.js', () => ({ getConfigPath: () => undefined, getProfile: () => undefined })); const sampleBody = { deviceList: [ From db5a2cf361fa0fa7b560fd67a6ec2df68598527b Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:49:41 +0800 Subject: [PATCH 06/34] fix(cache): key in-memory hot cache by profile to prevent leak in mcp serve (bug #37) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to bebc1d7. Disk was scoped by profile but the module-level _listCache / _statusCache globals were not — so mcp serve, which rotates profiles per request via withRequestContext, would return the first profile's inventory on subsequent requests regardless of the active profile. Replace the two singletons with Map keyed by getActiveProfile() (with '__default__' sentinel for the unnamed profile). resetListCache/resetStatusCache still clear everything. Also restore the JSDoc comment on DEFAULT_STATUS_GC_TTL_MS dropped in bebc1d7. Tests: 1 new case asserting no leak across profile switches in a single process. --- src/devices/cache.ts | 58 ++++++++++++++++++----------- tests/devices/cache-scoping.test.ts | 46 ++++++++++++++++++++++- 2 files changed, 81 insertions(+), 23 deletions(-) diff --git a/src/devices/cache.ts b/src/devices/cache.ts index 88dad9fa..1a4c9943 100644 --- a/src/devices/cache.ts +++ b/src/devices/cache.ts @@ -25,6 +25,8 @@ function scopedCacheDir(baseDir: string): string { if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true }); return dir; } + +/** GC cutoff for status entries: evict anything older than this. */ const DEFAULT_STATUS_GC_TTL_MS = 24 * 60 * 60 * 1000; // 24 h export interface CachedDevice { @@ -73,38 +75,46 @@ function cacheFilePath(): string { return path.join(dir, 'devices.json'); } -// In-memory hot-cache: undefined = not yet loaded, null = loaded but empty. -let _listCache: DeviceCache | null | undefined = undefined; -let _statusCache: StatusCache | undefined = undefined; +// In-memory hot-cache keyed by active profile (or '__default__' for no profile). +// Using Maps instead of module-level singletons ensures that mcp serve, which +// rotates profiles per request via withRequestContext, never leaks inventory +// across profiles within the same process (Bug #37). +const _listCacheByProfile = new Map(); +const _statusCacheByProfile = new Map(); + +function cacheKey(): string { + return getActiveProfile() ?? '__default__'; +} /** Force the next loadCache() call to re-read from disk. Used in tests. */ export function resetListCache(): void { - _listCache = undefined; + _listCacheByProfile.clear(); } /** Force the next loadStatusCache() call to re-read from disk. Used in tests. */ export function resetStatusCache(): void { - _statusCache = undefined; + _statusCacheByProfile.clear(); } export function loadCache(): DeviceCache | null { - if (_listCache !== undefined) return _listCache; + const key = cacheKey(); + if (_listCacheByProfile.has(key)) return _listCacheByProfile.get(key)!; const file = cacheFilePath(); if (!fs.existsSync(file)) { - _listCache = null; + _listCacheByProfile.set(key, null); return null; } try { const raw = fs.readFileSync(file, 'utf-8'); const cache = JSON.parse(raw) as DeviceCache; if (!cache || typeof cache.devices !== 'object' || cache.devices === null) { - _listCache = null; + _listCacheByProfile.set(key, null); return null; } - _listCache = cache; + _listCacheByProfile.set(key, cache); return cache; } catch { - _listCache = null; + _listCacheByProfile.set(key, null); return null; } } @@ -173,7 +183,7 @@ export function updateCacheFromDeviceList(body: DeviceListBodyShape): void { const dir = path.dirname(file); if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true }); fs.writeFileSync(file, JSON.stringify(cache, null, 2), { mode: 0o600 }); - _listCache = cache; + _listCacheByProfile.set(cacheKey(), cache); } catch { // Cache write failures must not break the command that triggered them. } @@ -182,7 +192,7 @@ export function updateCacheFromDeviceList(body: DeviceListBodyShape): void { export function clearCache(): void { const file = cacheFilePath(); if (fs.existsSync(file)) fs.unlinkSync(file); - _listCache = null; + _listCacheByProfile.set(cacheKey(), null); } // ---- Device list freshness ------------------------------------------------- @@ -231,29 +241,33 @@ function statusCacheFilePath(): string { } export function loadStatusCache(): StatusCache { - if (_statusCache !== undefined) return _statusCache; + const key = cacheKey(); + if (_statusCacheByProfile.has(key)) return _statusCacheByProfile.get(key)!; const file = statusCacheFilePath(); if (!fs.existsSync(file)) { - _statusCache = { entries: {} }; - return _statusCache; + const empty = { entries: {} }; + _statusCacheByProfile.set(key, empty); + return empty; } try { const raw = fs.readFileSync(file, 'utf-8'); const parsed = JSON.parse(raw) as StatusCache; if (!parsed || typeof parsed.entries !== 'object' || parsed.entries === null) { - _statusCache = { entries: {} }; - return _statusCache; + const empty = { entries: {} }; + _statusCacheByProfile.set(key, empty); + return empty; } - _statusCache = parsed; + _statusCacheByProfile.set(key, parsed); return parsed; } catch { - _statusCache = { entries: {} }; - return _statusCache; + const empty = { entries: {} }; + _statusCacheByProfile.set(key, empty); + return empty; } } function saveStatusCache(cache: StatusCache): void { - _statusCache = cache; + _statusCacheByProfile.set(cacheKey(), cache); try { const file = statusCacheFilePath(); const dir = path.dirname(file); @@ -308,7 +322,7 @@ export function setCachedStatus( export function clearStatusCache(): void { const file = statusCacheFilePath(); if (fs.existsSync(file)) fs.unlinkSync(file); - _statusCache = { entries: {} }; + _statusCacheByProfile.set(cacheKey(), { entries: {} }); } /** Summary for `switchbot cache show`. */ diff --git a/tests/devices/cache-scoping.test.ts b/tests/devices/cache-scoping.test.ts index 1f73d32e..190e8202 100644 --- a/tests/devices/cache-scoping.test.ts +++ b/tests/devices/cache-scoping.test.ts @@ -14,6 +14,7 @@ import os from 'node:os'; import { createHash } from 'node:crypto'; import { + loadCache, updateCacheFromDeviceList, setCachedStatus, resetListCache, @@ -144,7 +145,50 @@ describe('cache scoping — status cache follows the same rule', () => { }); }); -// ── e. --config-path override takes precedence (profile is ignored) ────────── +// ── e. In-memory cache does not leak across profile switches ───────────────── + +describe('cache scoping — in-memory hot cache isolation across profiles', () => { + it('in-memory cache does not leak across profile switches within a single process', () => { + const alphaBody = { + deviceList: [{ deviceId: 'ALPHA-1', deviceName: 'Alpha Bot', deviceType: 'Bot' }], + infraredRemoteList: [], + }; + const betaBody = { + deviceList: [{ deviceId: 'BETA-1', deviceName: 'Beta Plug', deviceType: 'Plug' }], + infraredRemoteList: [], + }; + + // Write alpha cache on disk and populate in-memory hot cache for alpha. + withRequestContext({ profile: 'alpha' }, () => { + updateCacheFromDeviceList(alphaBody); + }); + + // Verify alpha is in-memory. + const alphaResult = withRequestContext({ profile: 'alpha' }, () => loadCache()); + expect(alphaResult?.devices['ALPHA-1']).toBeDefined(); + + // Write beta's inventory directly to disk (bypassing the hot-cache write path), + // simulating the scenario where beta's data was written in a prior process and + // only the hot cache is "stale" (points to alpha). + const betaDir = path.join(tmpDir, '.switchbot', 'cache', sha8('beta')); + fs.mkdirSync(betaDir, { recursive: true }); + const betaCache = { + lastUpdated: new Date().toISOString(), + devices: { 'BETA-1': { type: 'Plug', name: 'Beta Plug', category: 'physical' } }, + }; + fs.writeFileSync(path.join(betaDir, 'devices.json'), JSON.stringify(betaCache)); + + // Read under profile "beta" WITHOUT calling resetListCache() first. + // With the bug (single global _listCache), this would return alpha's data. + // With the fix (Map keyed by profile), this must read from disk and return beta's data. + const betaResult = withRequestContext({ profile: 'beta' }, () => loadCache()); + + expect(betaResult?.devices['BETA-1']).toBeDefined(); + expect(betaResult?.devices['ALPHA-1']).toBeUndefined(); + }); +}); + +// ── f. --config-path override takes precedence (profile is ignored) ────────── describe('cache scoping — --config-path override', () => { it('uses config override dirname regardless of active profile', () => { From e42e7a3c02c45d478011c868a56e9b3b76ee131d Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:53:05 +0800 Subject: [PATCH 07/34] fix(devices): restore --fields id and name aliases (bug #22) 2.4.0 accepted --fields id / --fields name as aliases for deviceId and deviceName; the 2.5.0 list-alias refactor dropped them. Restore the mapping so scripts calling `devices list --fields id,name` keep working. --- src/commands/devices.ts | 2 +- tests/commands/devices.test.ts | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/commands/devices.ts b/src/commands/devices.ts index 1bba0a9d..568d673a 100644 --- a/src/commands/devices.ts +++ b/src/commands/devices.ts @@ -194,7 +194,7 @@ Examples: const defaultFields = options.wide ? undefined : narrowHeaders; // Accept API field names and short aliases alongside canonical column names const DEVICE_LIST_ALIASES: Record = { - name: 'deviceName', deviceType: 'type', type: 'type', + id: 'deviceId', name: 'deviceName', deviceType: 'type', type: 'type', roomName: 'room', familyName: 'family', hubDeviceId: 'hub', enableCloudService: 'cloud', }; diff --git a/tests/commands/devices.test.ts b/tests/commands/devices.test.ts index c844fbf8..73cbd158 100644 --- a/tests/commands/devices.test.ts +++ b/tests/commands/devices.test.ts @@ -374,6 +374,16 @@ describe('devices command', () => { expect(lines[1]).not.toContain('Living Lamp'); }); + it('--fields id,name aliases resolve to deviceId/deviceName columns (bug #22)', async () => { + apiMock.__instance.get.mockResolvedValue({ data: { body: sampleBody } }); + const res = await runCli(registerDevicesCommand, ['devices', 'list', '--format', 'tsv', '--fields', 'id,name']); + const lines = res.stdout.join('\n').split('\n'); + // Header row must show the resolved canonical column names + expect(lines[0]).toBe('deviceId\tdeviceName'); + // Data rows must contain the device id and name values + expect(lines[1]).toBe('ABC123\tLiving Lamp'); + }); + it('--format=id outputs one deviceId per line', async () => { apiMock.__instance.get.mockResolvedValue({ data: { body: sampleBody } }); const res = await runCli(registerDevicesCommand, ['devices', 'list', '--format', 'id']); From 2a51e89fd576178f0dddc87ca71db543e204c880 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:54:52 +0800 Subject: [PATCH 08/34] fix(errors): reclassify API code 190 as device-internal-error (bug #27) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code 190 is SwitchBot's generic "internal error" — it fires for invalid deviceIds, unsupported parameters, AND non-device endpoints like `webhook query` with no webhook configured. The "device-busy" subKind and device-specific hint were misleading for webhook. Rename subKind device-busy → device-internal-error and rewrite the hint to reflect the real semantics. --- src/utils/output.ts | 8 ++++---- tests/commands/mcp.test.ts | 6 +++--- tests/utils/output.test.ts | 11 +++++++++-- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/utils/output.ts b/src/utils/output.ts index aa50433e..8b727e6b 100644 --- a/src/utils/output.ts +++ b/src/utils/output.ts @@ -138,7 +138,7 @@ export type ErrorSubKind = | 'command-not-supported' | 'auth-failed' | 'quota-exceeded' - | 'device-busy' + | 'device-internal-error' | 'unknown-api-error'; export interface ErrorPayload { @@ -151,7 +151,7 @@ export interface ErrorPayload { context?: Record; retryAfterMs?: number; transient?: boolean; - errorClass?: 'network' | 'api' | 'device-offline' | 'device-busy' | 'guard' | 'usage'; + errorClass?: 'network' | 'api' | 'device-offline' | 'device-internal-error' | 'guard' | 'usage'; } export class StructuredUsageError extends Error { @@ -168,7 +168,7 @@ function classifyApiError(code: number): ErrorSubKind { case 152: return 'device-not-found'; case 161: case 171: return 'device-offline'; - case 190: return 'device-busy'; + case 190: return 'device-internal-error'; case 401: return 'auth-failed'; case 429: return 'quota-exceeded'; default: return 'unknown-api-error'; @@ -291,7 +291,7 @@ function errorHint(code: number): string | null { case 171: return 'The Hub itself is offline — check its power and Wi-Fi.'; case 190: - return "Often means the deviceId is wrong or the command/parameter is invalid for this device. Double-check with 'switchbot devices list' and 'switchbot devices describe '. Use --verbose to see the raw API response."; + return 'SwitchBot API code 190 is a generic internal error. Common causes: invalid deviceId, unsupported command/parameter, or the endpoint does not apply (e.g., "webhook query" with no webhook configured). Verify with --verbose.'; case 401: return "Re-run 'switchbot config set-token ', or verify SWITCHBOT_TOKEN / SWITCHBOT_SECRET."; case 429: diff --git a/tests/commands/mcp.test.ts b/tests/commands/mcp.test.ts index 3acf57ad..4e86d417 100644 --- a/tests/commands/mcp.test.ts +++ b/tests/commands/mcp.test.ts @@ -457,8 +457,8 @@ describe('mcp server', () => { expect(sc?.error?.errorClass).toBe('api'); }); - it('run_scene preserves structured error metadata on ApiError (code 190 device-busy)', async () => { - // Mock the POST (executeScene) to throw device-busy ApiError + it('run_scene preserves structured error metadata on ApiError (code 190 device-internal-error)', async () => { + // Mock the POST (executeScene) to throw device-internal-error ApiError apiMock.__instance.post.mockRejectedValueOnce( new ApiError('Device internal error', 190, { transient: false }) ); @@ -473,6 +473,6 @@ describe('mcp server', () => { const sc = (res as { structuredContent?: unknown }).structuredContent as | { error?: { subKind?: string } } | undefined; - expect(sc?.error?.subKind).toBe('device-busy'); + expect(sc?.error?.subKind).toBe('device-internal-error'); }); }); diff --git a/tests/utils/output.test.ts b/tests/utils/output.test.ts index 58a65a59..d8d7c444 100644 --- a/tests/utils/output.test.ts +++ b/tests/utils/output.test.ts @@ -199,7 +199,7 @@ describe('handleError', () => { expect(() => handleError(new ApiError('x', 190))).toThrow('__exit'); const joined = errSpy.mock.calls.map((c) => String(c[0])).join('\n'); expect(joined).toContain('Hint:'); - expect(joined).toMatch(/devices list|devices describe/); + expect(joined).toMatch(/generic internal error/); }); it('does not print a hint for unknown/unmapped codes', async () => { @@ -252,7 +252,7 @@ describe('handleError', () => { expect(parsed.schemaVersion).toBe('1.1'); expect(parsed.error.code).toBe(190); expect(parsed.error.message).toBe('bad device'); - expect(parsed.error.hint).toMatch(/devices/); + expect(parsed.error.hint).toMatch(/generic internal error/); }); it('marks 429 errors as retryable when ApiError.retryable is true', async () => { @@ -349,4 +349,11 @@ describe('buildErrorPayload', () => { expect(p.hint).toContain('deviceId'); expect(p.transient).toBe(false); }); + + it('ApiError code 190 → subKind device-internal-error (bug #27)', async () => { + const { ApiError } = await import('../../src/api/client.js'); + const p = buildErrorPayload(new ApiError('internal error', 190)); + expect(p.subKind).toBe('device-internal-error'); + expect(p.hint).toMatch(/generic internal error/); + }); }); From 178e19aeb073287509587ff6af24ecdb9436c021 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:55:32 +0800 Subject: [PATCH 09/34] fix(scenes): pre-validate sceneId in execute against scene list (bug #31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scenes execute ` returned ok:true because the upstream API doesn't validate sceneIds. `scenes describe` already guards against this — port the same check to execute so agents can't silently burn quota on nonexistent scenes. --- src/commands/scenes.ts | 9 +++++++++ tests/commands/scenes.test.ts | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/commands/scenes.ts b/src/commands/scenes.ts index 47bf6f74..fc1859f8 100644 --- a/src/commands/scenes.ts +++ b/src/commands/scenes.ts @@ -58,6 +58,15 @@ Example: `) .action(async (sceneId: string) => { try { + const sceneList = await fetchScenes(); + const found = sceneList.find((s) => s.sceneId === sceneId); + if (!found) { + throw new StructuredUsageError(`scene not found: ${sceneId}`, { + error: 'scene_not_found', + sceneId, + candidates: sceneList.map((s) => ({ sceneId: s.sceneId, sceneName: s.sceneName })), + }); + } await executeScene(sceneId); if (isJsonMode()) { printJson({ ok: true, sceneId }); diff --git a/tests/commands/scenes.test.ts b/tests/commands/scenes.test.ts index 42b02f29..f84848b7 100644 --- a/tests/commands/scenes.test.ts +++ b/tests/commands/scenes.test.ts @@ -110,6 +110,9 @@ describe('scenes command', () => { describe('execute', () => { it('POSTs to the scene execute endpoint and prints success', async () => { + apiMock.__instance.get.mockResolvedValue({ + data: { body: [{ sceneId: 'SCENE-1', sceneName: 'Morning' }] }, + }); apiMock.__instance.post.mockResolvedValue({ data: {} }); const res = await runCli(registerScenesCommand, ['scenes', 'execute', 'SCENE-1']); expect(apiMock.__instance.post).toHaveBeenCalledWith('/v1.1/scenes/SCENE-1/execute'); @@ -117,6 +120,9 @@ describe('scenes command', () => { }); it('exits 1 when execution fails', async () => { + apiMock.__instance.get.mockResolvedValue({ + data: { body: [{ sceneId: 'missing', sceneName: 'X' }] }, + }); apiMock.__instance.post.mockRejectedValue(new Error('not found')); const res = await runCli(registerScenesCommand, ['scenes', 'execute', 'missing']); expect(res.exitCode).toBe(1); @@ -128,6 +134,19 @@ describe('scenes command', () => { expect(apiMock.__instance.post).not.toHaveBeenCalled(); expect(res.stderr.join('\n').toLowerCase()).toContain('missing required'); }); + + it('exits 2 with scene_not_found and never calls executeScene for bogus sceneId (bug #31)', async () => { + apiMock.__instance.get.mockResolvedValue({ + data: { body: [{ sceneId: 'S1', sceneName: 'Good Morning' }] }, + }); + const res = await runCli(registerScenesCommand, ['scenes', 'execute', 'BOGUS-ID', '--json']); + expect(res.exitCode).toBe(2); + expect(apiMock.__instance.post).not.toHaveBeenCalled(); + const out = res.stderr.join('\n'); + const parsed = JSON.parse(out); + expect(parsed.error?.context?.error).toBe('scene_not_found'); + expect(parsed.error?.context?.sceneId).toBe('BOGUS-ID'); + }); }); describe('describe', () => { From 419556e75aa73873f1f373b2c66adaef0d9f045a Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:56:14 +0800 Subject: [PATCH 10/34] fix(cache): accept --status and --list as shorthand for --key (bug #35) Round-2 report: `cache clear --status` was rejected with "unknown option"; users had to know the more verbose --key status form. Both shorthands now work; using them with --key or together errors with a clear UsageError. --- src/commands/cache.ts | 15 +++++++++++++-- tests/commands/cache.test.ts | 28 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/commands/cache.ts b/src/commands/cache.ts index 7bf65c3c..079ddb34 100644 --- a/src/commands/cache.ts +++ b/src/commands/cache.ts @@ -92,9 +92,20 @@ Examples: .command('clear') .description('Delete cache files') .option('--key ', 'Which cache to clear: "list" | "status" | "all" (default)', enumArg('--key', CACHE_KEYS), 'all') - .action((options: { key: string }) => { + .option('--status', 'Shorthand for --key status') + .option('--list', 'Shorthand for --key list') + .action((options: { key: string; status?: boolean; list?: boolean }) => { try { - const key = options.key; + if (options.status && options.list) { + throw new UsageError('--status and --list are mutually exclusive.'); + } + if ((options.status || options.list) && options.key !== 'all') { + throw new UsageError('--status / --list cannot be combined with --key.'); + } + let key = options.key; + if (options.status) key = 'status'; + if (options.list) key = 'list'; + if (!['list', 'status', 'all'].includes(key)) { throw new UsageError(`Unknown --key "${key}". Expected: list, status, all.`); } diff --git a/tests/commands/cache.test.ts b/tests/commands/cache.test.ts index 15471282..35170392 100644 --- a/tests/commands/cache.test.ts +++ b/tests/commands/cache.test.ts @@ -163,4 +163,32 @@ describe('cache clear', () => { expect(result.exitCode).toBeNull(); expect(result.stdout.join('\n')).toMatch(/Cleared/); }); + + it('--status shorthand clears only status cache (bug #35)', async () => { + updateCacheFromDeviceList(SAMPLE_BODY); + setCachedStatus('BOT1', { power: 'on' }); + + const listFile = path.join(tmpHome, '.switchbot', 'devices.json'); + const statusFile = path.join(tmpHome, '.switchbot', 'status.json'); + + const result = await runCli(registerCacheCommand, ['cache', 'clear', '--status']); + expect(result.exitCode).toBeNull(); + expect(fs.existsSync(listFile)).toBe(true); + expect(fs.existsSync(statusFile)).toBe(false); + expect(result.stdout.join('\n')).toMatch(/Cleared:.*status/); + }); + + it('--list shorthand clears only list cache (bug #35)', async () => { + updateCacheFromDeviceList(SAMPLE_BODY); + setCachedStatus('BOT1', { power: 'on' }); + + const listFile = path.join(tmpHome, '.switchbot', 'devices.json'); + const statusFile = path.join(tmpHome, '.switchbot', 'status.json'); + + const result = await runCli(registerCacheCommand, ['cache', 'clear', '--list']); + expect(result.exitCode).toBeNull(); + expect(fs.existsSync(listFile)).toBe(false); + expect(fs.existsSync(statusFile)).toBe(true); + expect(result.stdout.join('\n')).toMatch(/Cleared:.*list/); + }); }); From 0e6b6f3076d79c014d130909bb23b80809be2fce Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:57:06 +0800 Subject: [PATCH 11/34] fix(meta): enforce alias uniqueness across devices (bug #41) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing stopped two devices from carrying the same alias — --name resolution against a duplicated alias was undefined. Reject duplicate aliases with a clear error naming the existing holder; --force reassigns (clears the old holder's alias) with a log line. --- src/commands/device-meta.ts | 25 ++++++++++++++++++++++++- tests/commands/device-meta.test.ts | 25 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/commands/device-meta.ts b/src/commands/device-meta.ts index a8989f42..3c52c322 100644 --- a/src/commands/device-meta.ts +++ b/src/commands/device-meta.ts @@ -3,6 +3,7 @@ import { stringArg } from '../utils/arg-parsers.js'; import { handleError, isJsonMode, printJson, printTable, UsageError } from '../utils/output.js'; import { loadDeviceMeta, + saveDeviceMeta, setDeviceMeta, clearDeviceMeta, getDeviceMeta, @@ -23,7 +24,8 @@ export function registerDevicesMetaCommand(devices: Command): void { .option('--hide', 'Hide this device from "devices list"') .option('--show', 'Un-hide this device') .option('--notes ', 'Freeform notes shown in "devices describe"', stringArg('--notes')) - .action((deviceId: string, options: { alias?: string; hide?: boolean; show?: boolean; notes?: string }) => { + .option('--force', 'Reassign alias even if it already belongs to another device') + .action((deviceId: string, options: { alias?: string; hide?: boolean; show?: boolean; notes?: string; force?: boolean }) => { try { if (options.hide && options.show) { throw new UsageError('--hide and --show cannot be used together.'); @@ -32,6 +34,27 @@ export function registerDevicesMetaCommand(devices: Command): void { throw new UsageError('Specify at least one of: --alias, --hide, --show, --notes'); } + // Enforce alias uniqueness across devices + if (options.alias !== undefined) { + const meta = loadDeviceMeta(); + const holder = Object.entries(meta.devices).find( + ([id, m]) => m.alias === options.alias && id !== deviceId, + ); + if (holder) { + if (!options.force) { + throw new UsageError( + `Alias "${options.alias}" is already assigned to device ${holder[0]}. Use --force to reassign.`, + ); + } + // --force: clear the alias from the previous holder + meta.devices[holder[0]] = { ...meta.devices[holder[0]], alias: undefined }; + saveDeviceMeta(meta); + if (!isJsonMode()) { + console.log(`(reassigned alias from ${holder[0]})`); + } + } + } + const patch: Record = {}; if (options.alias !== undefined) patch.alias = options.alias; if (options.notes !== undefined) patch.notes = options.notes; diff --git a/tests/commands/device-meta.test.ts b/tests/commands/device-meta.test.ts index b15e53ef..6d2561cb 100644 --- a/tests/commands/device-meta.test.ts +++ b/tests/commands/device-meta.test.ts @@ -79,4 +79,29 @@ describe('devices meta', () => { const res = await runCli(registerDevicesCommand, ['devices', 'meta', 'get', 'LAMP-1']); expect(res.stdout.join('\n')).toContain('No local metadata'); }); + + it('setting alias on device B (without --force) when device A already holds it → exit 2 mentioning device A (bug #41)', async () => { + await runCli(registerDevicesCommand, ['devices', 'meta', 'set', 'LAMP-1', '--alias', 'myAlias']); + const res = await runCli(registerDevicesCommand, ['devices', 'meta', 'set', 'LAMP-2', '--alias', 'myAlias']); + expect(res.exitCode).toBe(2); + expect(res.stderr.join('\n')).toContain('LAMP-1'); + }); + + it('--force reassigns alias from device A to device B and clears A (bug #41)', async () => { + await runCli(registerDevicesCommand, ['devices', 'meta', 'set', 'LAMP-1', '--alias', 'myAlias']); + const res = await runCli(registerDevicesCommand, ['devices', 'meta', 'set', 'LAMP-2', '--alias', 'myAlias', '--force']); + expect(res.exitCode).toBeNull(); + // LAMP-1 should have no alias now + const lamp1 = await runCli(registerDevicesCommand, ['devices', 'meta', 'get', 'LAMP-1']); + expect(lamp1.stdout.join('\n')).not.toContain('myAlias'); + // LAMP-2 should hold the alias + const lamp2 = await runCli(registerDevicesCommand, ['devices', 'meta', 'get', 'LAMP-2']); + expect(lamp2.stdout.join('\n')).toContain('myAlias'); + }); + + it('re-asserting the same alias on the same device is a no-op (no conflict with self, bug #41)', async () => { + await runCli(registerDevicesCommand, ['devices', 'meta', 'set', 'LAMP-1', '--alias', 'myAlias']); + const res = await runCli(registerDevicesCommand, ['devices', 'meta', 'set', 'LAMP-1', '--alias', 'myAlias']); + expect(res.exitCode).toBeNull(); + }); }); From 5f1aa17c7267e14f85c3c717676047059195a293 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 19:58:16 +0800 Subject: [PATCH 12/34] fix(history): mark --metric as requiredOption in aggregate (bug #42) Help text showed `(default: [])`, implying optional; the command actually required it and threw a custom error. Switch to Commander's .requiredOption so `--help` says "required" and the error message matches other required options in the CLI. --- src/commands/history.ts | 5 +---- tests/commands/history.test.ts | 9 +++++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/commands/history.ts b/src/commands/history.ts index 88066952..61b284c9 100644 --- a/src/commands/history.ts +++ b/src/commands/history.ts @@ -280,7 +280,7 @@ Examples: .option('--since ', 'Relative window ending now, e.g. "1h", "7d" (mutually exclusive with --from/--to)', stringArg('--since')) .option('--from ', 'Range start (ISO-8601)', stringArg('--from')) .option('--to ', 'Range end (ISO-8601)', stringArg('--to')) - .option('--metric ', 'Payload field to aggregate (repeat for multiple)', (v: string, acc: string[] = []) => acc.concat(v), [] as string[]) + .requiredOption('--metric ', 'Payload field to aggregate (repeat for multiple; required)', (v: string, acc: string[] = []) => acc.concat(v)) .option('--agg ', 'Comma-separated aggregation functions (count,min,max,avg,sum,p50,p95)', stringArg('--agg')) .option('--bucket ', 'Bucket width, e.g. "15m", "1h", "1d"', stringArg('--bucket')) .option('--max-bucket-samples ', 'Max samples per bucket for quantiles (1–100000)', intArg('--max-bucket-samples', { min: 1, max: 100_000 })) @@ -289,9 +289,6 @@ Examples: options: { since?: string; from?: string; to?: string; metric?: string[]; agg?: string; bucket?: string; maxBucketSamples?: string }, ) => { const metrics: string[] = options.metric ?? []; - if (metrics.length === 0) { - handleError(new UsageError('at least one --metric is required.')); - } if (options.since && (options.from || options.to)) { handleError(new UsageError('--since is mutually exclusive with --from/--to.')); diff --git a/tests/commands/history.test.ts b/tests/commands/history.test.ts index e41c439f..8457113d 100644 --- a/tests/commands/history.test.ts +++ b/tests/commands/history.test.ts @@ -346,12 +346,13 @@ describe('history aggregate (D7)', () => { expect(parsed.data.buckets[0].metrics.temperature.avg).toBe(22); }); - it('exits 2 with UsageError when --metric is missing', async () => { - const exitSpy = vi.spyOn(process, 'exit').mockImplementation((_code) => { throw new Error('process.exit'); }); + it('exits with error when --metric is missing (requiredOption enforcement, bug #42)', async () => { const res = await runCli(registerHistoryCommand, [ 'history', 'aggregate', 'DEV1', '--since', '1h', ]); - exitSpy.mockRestore(); - expect(res.exitCode).toBe(2); + expect(res.exitCode).not.toBeNull(); + expect(res.exitCode).not.toBe(0); + const errOut = res.stderr.join('\n'); + expect(errOut).toMatch(/--metric/); }); }); From c9570feb5a5793ef0d5e90cfa9378a0d5db71823 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 20:04:33 +0800 Subject: [PATCH 13/34] test(cache): cover --status/--list conflict paths (bug #35) Follow-up to 419556e. The reject paths for (a) --status combined with --list and (b) --status combined with --key were implemented in src but not exercised by tests. Add two small UsageError assertions to close the coverage gap. --- tests/commands/cache.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/commands/cache.test.ts b/tests/commands/cache.test.ts index 35170392..b61ea3b2 100644 --- a/tests/commands/cache.test.ts +++ b/tests/commands/cache.test.ts @@ -191,4 +191,16 @@ describe('cache clear', () => { expect(fs.existsSync(statusFile)).toBe(true); expect(result.stdout.join('\n')).toMatch(/Cleared:.*list/); }); + + it('--status + --list together exits 2 (bug #35)', async () => { + const result = await runCli(registerCacheCommand, ['cache', 'clear', '--status', '--list']); + expect(result.exitCode).toBe(2); + expect(result.stderr.join('\n')).toMatch(/mutually exclusive/i); + }); + + it('--status combined with --key exits 2 (bug #35)', async () => { + const result = await runCli(registerCacheCommand, ['cache', 'clear', '--status', '--key', 'status']); + expect(result.exitCode).toBe(2); + expect(result.stderr.join('\n')).toMatch(/cannot be combined with --key/i); + }); }); From 220af2efb502f797ba0ad42d97a8aca027afbe29 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 20:06:45 +0800 Subject: [PATCH 14/34] fix(errors): map API code 3005 to command-not-supported (bug #29) Code 3005 "invalid value" is the API's catch-all for model-specific command rejections (e.g., Fan speed commands on stock IR remotes that only work under --type customize). Previously surfaced as unknown-api-error with no actionable hint. --- src/utils/output.ts | 5 ++++- tests/utils/output.test.ts | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/utils/output.ts b/src/utils/output.ts index 8b727e6b..bc0aae6d 100644 --- a/src/utils/output.ts +++ b/src/utils/output.ts @@ -164,7 +164,8 @@ export class StructuredUsageError extends Error { function classifyApiError(code: number): ErrorSubKind { switch (code) { case 151: - case 160: return 'command-not-supported'; + case 160: + case 3005: return 'command-not-supported'; case 152: return 'device-not-found'; case 161: case 171: return 'device-offline'; @@ -296,6 +297,8 @@ function errorHint(code: number): string | null { return "Re-run 'switchbot config set-token ', or verify SWITCHBOT_TOKEN / SWITCHBOT_SECRET."; case 429: return 'Daily quota is 10,000 requests/account — retry after midnight UTC.'; + case 3005: + return "SwitchBot rejected the command as invalid for this specific device model. For IR remotes, this often means the command works only on --type customize (user-learned buttons). Try 'switchbot devices commands ' or check the device's capabilities."; default: return null; } diff --git a/tests/utils/output.test.ts b/tests/utils/output.test.ts index d8d7c444..b7b452cb 100644 --- a/tests/utils/output.test.ts +++ b/tests/utils/output.test.ts @@ -356,4 +356,11 @@ describe('buildErrorPayload', () => { expect(p.subKind).toBe('device-internal-error'); expect(p.hint).toMatch(/generic internal error/); }); + + it('ApiError code 3005 → subKind command-not-supported with --type customize hint (bug #29)', async () => { + const { ApiError } = await import('../../src/api/client.js'); + const p = buildErrorPayload(new ApiError('invalid value', 3005, { transient: false })); + expect(p.subKind).toBe('command-not-supported'); + expect(p.hint).toMatch(/--type customize/); + }); }); From 8b23345caf6a0ae942bfa9b8703bb88748acc407 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 20:07:25 +0800 Subject: [PATCH 15/34] fix(plan): clarify `plan validate` is structural-only (bug #32) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agents chaining validate → run were surprised that plans with bogus deviceNames or sceneIds passed validation. Make the scope explicit in the description and point users to `plan run --dry-run` for semantic checks. --- src/commands/plan.ts | 7 ++++++- tests/commands/plan.test.ts | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/commands/plan.ts b/src/commands/plan.ts index 5621b51f..16e79f61 100644 --- a/src/commands/plan.ts +++ b/src/commands/plan.ts @@ -251,8 +251,13 @@ Workflow: plan .command('validate') - .description('Validate a plan file (or stdin) against the schema') + .description('Validate a plan file (or stdin) against the schema (structural only; does not verify device or scene existence)') .argument('[file]', 'Path to plan.json, or "-" / omit to read stdin') + .addHelpText('after', ` +To check semantic validity (e.g., that deviceIds and sceneIds actually exist), +use 'plan run --dry-run' which exercises name resolution and device lookup +against the live API without executing any mutations. +`) .action(async (file: string | undefined) => { let raw: unknown; try { diff --git a/tests/commands/plan.test.ts b/tests/commands/plan.test.ts index 7b73c05c..08d5563f 100644 --- a/tests/commands/plan.test.ts +++ b/tests/commands/plan.test.ts @@ -160,6 +160,12 @@ describe('plan command', () => { expect(out.valid).toBe(true); expect(out.steps).toBe(1); }); + + it('--help output contains "structural only" (bug #32)', async () => { + const res = await runCli(registerPlanCommand, ['plan', 'validate', '--help']); + const all = [...res.stdout, ...res.stderr].join('\n'); + expect(all).toMatch(/structural only/); + }); }); describe('plan run', () => { From 7cd2a69715b342ddd92bbe4a32df5e4d0e198200 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 20:07:37 +0800 Subject: [PATCH 16/34] docs(cache): clarify TTL uses JSON lastUpdated field, not mtime (bug #34) Operators who `touch` cache files to force-refresh were surprised the CLI ignored mtime. One-line note in help text. --- src/commands/cache.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/commands/cache.ts b/src/commands/cache.ts index 079ddb34..3b45151f 100644 --- a/src/commands/cache.ts +++ b/src/commands/cache.ts @@ -53,6 +53,10 @@ Examples: .command('show') .alias('status') .description('Summarize the cache files (paths, ages, entry counts)') + .addHelpText('after', ` +Cache TTL is computed from the 'lastUpdated' field inside the JSON, not the file mtime. +touch does not invalidate; use 'cache clear' to force a refresh. +`) .action(() => { const summary = describeCache(); if (isJsonMode()) { From b2a3658d2e1965003950b8e1fe466a90aca6c417 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 20:08:28 +0800 Subject: [PATCH 17/34] fix(meta): surface devices meta in agent-bootstrap and capabilities (bug #40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local metadata system (devices meta set --alias …) was undiscoverable: not in agent-bootstrap, not in capabilities, not in the devices help footer. Add the four subcommands to COMMAND_META and a 'meta' entry to QUICK_REFERENCE so agents find it on first bootstrap. --- src/commands/agent-bootstrap.ts | 1 + src/commands/capabilities.ts | 5 +++++ tests/commands/capabilities.test.ts | 15 +++++++++++++++ 3 files changed, 21 insertions(+) diff --git a/src/commands/agent-bootstrap.ts b/src/commands/agent-bootstrap.ts index 80f572c3..212e98f3 100644 --- a/src/commands/agent-bootstrap.ts +++ b/src/commands/agent-bootstrap.ts @@ -29,6 +29,7 @@ const QUICK_REFERENCE = { safety: ['--dry-run', '--idempotency-key ', '--audit-log', '--no-quota'], observability: ['doctor --json', 'quota status', 'cache status', 'events mqtt-tail'], history: ['history range --since 7d', 'history stats '], + meta: ['devices meta set --alias ', 'devices meta list', 'devices meta get '], }; interface BootstrapOptions { diff --git a/src/commands/capabilities.ts b/src/commands/capabilities.ts index d49e7b60..3c52f484 100644 --- a/src/commands/capabilities.ts +++ b/src/commands/capabilities.ts @@ -40,6 +40,11 @@ const COMMAND_META: Record = { 'devices types': { mutating: false, consumesQuota: false, idempotencySupported: false, agentSafetyTier: 'read', verifiability: 'local', typicalLatencyMs: 20 }, 'devices commands': { mutating: false, consumesQuota: false, idempotencySupported: false, agentSafetyTier: 'read', verifiability: 'local', typicalLatencyMs: 20 }, 'devices watch': { mutating: false, consumesQuota: true, idempotencySupported: false, agentSafetyTier: 'read', verifiability: 'local', typicalLatencyMs: 500 }, + // devices meta (local metadata — no quota, no API call) + 'devices meta set': { mutating: true, consumesQuota: false, idempotencySupported: false, agentSafetyTier: 'action', verifiability: 'local', typicalLatencyMs: 5 }, + 'devices meta get': { mutating: false, consumesQuota: false, idempotencySupported: false, agentSafetyTier: 'read', verifiability: 'local', typicalLatencyMs: 5 }, + 'devices meta list': { mutating: false, consumesQuota: false, idempotencySupported: false, agentSafetyTier: 'read', verifiability: 'local', typicalLatencyMs: 5 }, + 'devices meta clear': { mutating: true, consumesQuota: false, idempotencySupported: false, agentSafetyTier: 'action', verifiability: 'local', typicalLatencyMs: 5 }, // devices: actions 'devices command': { mutating: true, consumesQuota: true, idempotencySupported: true, agentSafetyTier: 'action', verifiability: 'deviceDependent', typicalLatencyMs: 800 }, 'devices batch': { mutating: true, consumesQuota: true, idempotencySupported: true, agentSafetyTier: 'action', verifiability: 'deviceDependent', typicalLatencyMs: 1200 }, diff --git a/tests/commands/capabilities.test.ts b/tests/commands/capabilities.test.ts index fa96722b..e0012d5d 100644 --- a/tests/commands/capabilities.test.ts +++ b/tests/commands/capabilities.test.ts @@ -19,6 +19,12 @@ function makeProgram(): Command { const describe = devices.command('describe').description('Show full device info'); describe.argument('', 'Device ID'); describe.option('--json', 'JSON output'); + // devices meta subcommands (bug #40) + const meta = devices.command('meta').description('Manage local device metadata'); + meta.command('set').description('Set metadata for a device'); + meta.command('get').description('Get metadata for a device'); + meta.command('list').description('List all device metadata'); + meta.command('clear').description('Clear metadata for a device'); const history = p.command('history').description('Device history and aggregation'); history.command('aggregate').description('Aggregate device history'); @@ -224,4 +230,13 @@ describe('capabilities B3/B4', () => { const mcp = (out.surfaces as Record).mcp; expect(mcp.tools).toContain('aggregate_device_history'); }); + + it('devices meta set appears in compact capabilities output (bug #40)', async () => { + const out = await runCapabilitiesWith(['--compact']); + const cmds = out.commands as Array<{ name: string; agentSafetyTier: string; mutating: boolean }>; + const metaSet = cmds.find((c) => c.name === 'devices meta set'); + expect(metaSet).toBeDefined(); + expect(metaSet!.agentSafetyTier).toBe('action'); + expect(metaSet!.mutating).toBe(true); + }); }); From 5890ddceda370485b5ee4284e5330a047a0d46cc Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 20:08:53 +0800 Subject: [PATCH 18/34] docs(history): describe .json vs .jsonl companion files (bug #43) The .json file in ~/.switchbot/device-history/ was never documented. It's the 100-entry ring buffer read by MCP get_device_history; the .jsonl is the append-only source of truth for history range and aggregate. --- docs/agent-guide.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/agent-guide.md b/docs/agent-guide.md index 0bb5ad83..a8dc3885 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -89,6 +89,18 @@ Reads `~/.switchbot/device-history/.json` written by `events mqtt-tail **Workflow**: run `switchbot events mqtt-tail` in the background (e.g. with pm2) to keep the history files fresh; then call `get_device_history` from any MCP session without consuming REST quota. +#### Device-history directory layout + +After `events mqtt-tail` runs on a device, `~/.switchbot/device-history/` contains up to three companion files per device: + +| File | Description | +|------|-------------| +| `.jsonl` | Append-only, authoritative event log. Source of truth for `history range` and `history aggregate`. Rotated at ~50 MB (up to 3 segments). | +| `.json` | Latest 100-entry ring buffer. Written on every MQTT event. Read by MCP `get_device_history` for fast, zero-quota retrieval. | +| `__control.jsonl` | MQTT connection lifecycle events (heartbeat, connect, disconnect). Not a device log; used for diagnostics. | + +The `.json` file is **not** the source of truth for historical queries — use `.jsonl` (via `history range` or `history aggregate`) when you need a complete, time-bounded record. The `.json` file is optimised for "what is the latest state?" lookups. + ### MCP resource: `switchbot://events` Read-only snapshot of recent MQTT shadow-update events from the ring buffer. Returns `{state, count, events[]}`. From cb4bbc1c14930f4c3ce1e75ad9a320bbd683fad1 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 20:10:59 +0800 Subject: [PATCH 19/34] chore(release): 2.5.1 Round-2 smoke-test response: 13 bug fixes across 18 commits. See CHANGELOG.md for the per-bug breakdown and responses to the two false positives and two deferred feature requests. --- CHANGELOG.md | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++ package.json | 2 +- 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee5969ad..bdb1fc85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,110 @@ All notable changes to `@switchbot/openapi-cli` are documented in this file. The format is loosely based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [2.5.1] - 2026-04-20 + +Round-2 smoke-test response: 13 bugs closed (7 🔴 correctness / safety, 6 🟡 +UX). Source: `switchbot-cli-v2.5.0-round2-report.md`. Two items the report +flagged as bugs were found to be working as designed (false positives); two +were feature requests and deferred to 2.6.0 — see *Not included* below. + +### Fixed (correctness & safety) + +- **`devices command --dry-run --json` no longer emits empty stdout** — + the single-device write path was hitting `handleError`'s silent + `DryRunSignal` exit before the JSON serializer ran. Now mirrors the MCP + `send_command {dryRun:true}` shape: + `{schemaVersion:"1.1", data:{dryRun:true, wouldSend:{deviceId,command,parameter,commandType}}}`. + Batch and plan dry-run paths were already correct. (bug #36) +- **MCP tool-call errors preserve structure** — `send_command` / + `describe_device` / `run_scene` were letting `ApiError`s escape to the + SDK's generic `createToolError`, collapsing `{code, subKind, transient, + hint, retryAfterMs, errorClass}` to a plain-text string. Errors now + return `structuredContent.error` alongside `isError:true` so agents can + branch on `subKind` instead of parsing English. Also narrowed the + `mcpError()` option types so `subKind` / `errorClass` are compile-time + checked. (bug #38) +- **`devices batch` propagates `verification` + `subKind` for IR devices** — + a batch over IR remotes was emitting zero unverifiability signal, the + exact contract 2.4.0 was released to establish. `succeeded[]` entries + now include `subKind:'ir-no-feedback'` and the verification object for + IR devices, plus `summary.unverifiableCount`. (bug #28) +- **Device & status cache scoped per profile** — `devices.json` and + `status.json` lived at a fixed disk path, so rotating credentials or + switching profiles served the *prior* session's inventory. Cache files + now live under `~/.switchbot/cache//` when a profile + is active; unnamed/default profile keeps the legacy `~/.switchbot/` + path (backwards compatible). A follow-up fix also keys the in-memory + hot cache (`_listCache` / `_statusCache`) by profile so `mcp serve` + request-scoped profile switches do not leak either. (bug #37) +- **API code 190 reclassified `device-internal-error`** — 190 fires for + invalid deviceIds, unsupported parameters, AND non-device endpoints + like `webhook query` with no webhook configured. The `device-busy` + subKind and device-specific hint were misleading for webhook. Renamed + subKind + rewrote hint to cover all three causes. (bug #27) +- **API code 3005 mapped to `command-not-supported`** — 3005 "invalid + value" is the API's catch-all for model-specific command rejections + (e.g., Fan `lowSpeed/middleSpeed/highSpeed` on stock IR remotes that + only work under `--type customize`). Now returns a useful subKind + hint + pointing to `devices commands ` and `--type customize`. (bug #29) +- **`scenes execute` pre-validates sceneId** — `scenes execute ` + returned `ok:true` because the API does not validate sceneIds. + `scenes describe` already guarded against this via `scene_not_found` — + port the same check so agents do not silently burn quota. (bug #31) +- **`devices meta set --alias` enforces uniqueness** — nothing stopped + two devices from carrying the same alias; `--name ` behavior + was undefined. Reject duplicate aliases with a clear error naming the + existing holder; `--force` reassigns (clears the old holder's alias) + with a log line. (bug #41) + +### Fixed (UX & docs) + +- **`--fields id` / `--fields name` aliases restored on `devices list`** — + the 2.5.0 alias-map refactor dropped the short forms that 2.4.0 + accepted, breaking scripts. `id → deviceId` is back alongside + `name → deviceName`. (bug #22) +- **`cache clear --status` and `--list` shorthand aliases** — the old + `--key status` form still works, but the shorter flags no longer + error with `unknown option`. Using them with `--key` or together + raises `UsageError`. (bug #35) +- **`history aggregate --metric` marked `requiredOption`** — help text + said `(default: [])` implying optional; the command actually required + at least one metric and threw a custom error. Now Commander enforces + it and `--help` says `required`. (bug #42) +- **`plan validate` help text clarifies scope** — now says "structural + only; does not verify device or scene existence" and points to + `plan run --dry-run` for semantic checks. (bug #32) +- **`cache` help text documents TTL behavior** — the cache TTL is computed + from the `lastUpdated` field *inside* the JSON, not file mtime. + Operators who `touch`ed cache files to force a refresh were surprised. + One-line note added to `cache show --help`. (bug #34) +- **`devices meta` surfaced in agent-bootstrap and capabilities** — the + local metadata system was completely undiscoverable in 2.5.0. `meta set + / get / list / clear` now appear in `capabilities` with correct safety + tiers, and `agent-bootstrap`'s `quickReference` gains a `meta` entry. + (bug #40) +- **`~/.switchbot/device-history/.json` companion file documented** — + the 100-entry ring buffer read by MCP `get_device_history` had no docs, + while only the append-only `.jsonl` was mentioned. `docs/agent-guide.md` + now describes both files and `__control.jsonl`. (bug #43) + +### Not included (response to report) + +- **Report bug #19 (MCP strict schema not enforced) — false positive.** + All 11 MCP tools already have `.strict()` on their Zod input schemas + and the SDK enforces it via `safeParseAsync` → JSON-RPC `-32602`. + Could not reproduce the reported behavior; the existing test suite + exercises the full JSON-RPC path. +- **Report bug #30 (`devices batch --idempotency-key`) — working as + designed.** Batch intentionally uses `--idempotency-key-prefix` and + auto-appends `-` per step so each step has a distinct key. + A single `--idempotency-key` across a batch would cause only the first + step to execute and the rest to be replayed. +- **Deferred to 2.6.0:** `devices batch --skip-offline` (bug #33 — a + preflight status refresh + short-circuit; feature request, not a + correctness bug) and `--filter` DSL expansion with substring/regex + (bug #39 — current `key=value` exact match is documented behavior). + ## [2.5.0] - 2026-04-20 ### Added diff --git a/package.json b/package.json index c6367f63..1c179bcf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@switchbot/openapi-cli", - "version": "2.5.0", + "version": "2.5.1", "description": "SwitchBot smart home CLI — control devices, run scenes, stream real-time events, and integrate AI agents via MCP. Full API v1.1 coverage.", "keywords": [ "switchbot", From 6ade9fd3e2e3b2399b17967e81bda03a431fbe32 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 20:15:12 +0800 Subject: [PATCH 20/34] test(mcp): drop hardcoded version literal The VERSION assertion compared against both package.json and a hardcoded '2.5.0' string. The hardcoded form silently went stale on every version bump; the package.json read already proves the real contract. Remove the redundant literal so future bumps don't require a test edit. --- tests/mcp/server-version.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/mcp/server-version.test.ts b/tests/mcp/server-version.test.ts index a5da5adb..a3c2678d 100644 --- a/tests/mcp/server-version.test.ts +++ b/tests/mcp/server-version.test.ts @@ -13,7 +13,6 @@ describe('mcp server version', () => { // Verify the VERSION constant matches expect(VERSION).toBe(expectedVersion); - expect(VERSION).toBe('2.5.0'); }); }); From 167189045b23994e571facdf4b4a3e79aaf1967b Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 21:47:41 +0800 Subject: [PATCH 21/34] fix(flags): duration parser accepts d and w units (bug #54) Extend parseDurationToMs regex to (ms|s|m|h|d|w)? and update durationArg error message to list all supported units. Previously --cache 1d, --for 2w, etc. returned null and silently fell back to defaults. Unsupported units (y, year, month) still reject but now with a hint listing the six supported suffixes. --- src/utils/arg-parsers.ts | 3 ++- src/utils/flags.ts | 4 +++- tests/utils/arg-parsers.test.ts | 8 ++++++++ tests/utils/flags.test.ts | 29 +++++++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/utils/arg-parsers.ts b/src/utils/arg-parsers.ts index d2cdbca4..13284891 100644 --- a/src/utils/arg-parsers.ts +++ b/src/utils/arg-parsers.ts @@ -48,7 +48,8 @@ export function durationArg(flagName: string): (value: string) => string { const ms = parseDurationToMs(value); if (ms === null) { throw new InvalidArgumentError( - `${flagName} must look like "30s", "1m", "500ms", "1h" (got "${value}")`, + `${flagName} must look like "30s", "1m", "500ms", "1h", "7d", "2w" ` + + `(supported units: ms, s, m, h, d, w — got "${value}")`, ); } return value; diff --git a/src/utils/flags.ts b/src/utils/flags.ts index 28876d7d..2e42e817 100644 --- a/src/utils/flags.ts +++ b/src/utils/flags.ts @@ -112,7 +112,7 @@ export interface CacheMode { const DEFAULT_LIST_TTL_MS = 60 * 60 * 1000; function parseDurationToMs(v: string): number | null { - const m = /^(\d+)(ms|s|m|h)?$/.exec(v.trim().toLowerCase()); + const m = /^(\d+)(ms|s|m|h|d|w)?$/.exec(v.trim().toLowerCase()); if (!m) return null; const n = Number(m[1]); if (!Number.isFinite(n) || n < 0) return null; @@ -122,6 +122,8 @@ function parseDurationToMs(v: string): number | null { case 's': return n * 1000; case 'm': return n * 60 * 1000; case 'h': return n * 60 * 60 * 1000; + case 'd': return n * 24 * 60 * 60 * 1000; + case 'w': return n * 7 * 24 * 60 * 60 * 1000; default: return null; } } diff --git a/tests/utils/arg-parsers.test.ts b/tests/utils/arg-parsers.test.ts index 15daeb65..713bc5bb 100644 --- a/tests/utils/arg-parsers.test.ts +++ b/tests/utils/arg-parsers.test.ts @@ -54,6 +54,8 @@ describe('durationArg', () => { expect(parse('500ms')).toBe('500ms'); expect(parse('1m')).toBe('1m'); expect(parse('1h')).toBe('1h'); + expect(parse('1d')).toBe('1d'); + expect(parse('2w')).toBe('2w'); expect(parse('1000')).toBe('1000'); // bare ms }); @@ -66,6 +68,12 @@ describe('durationArg', () => { expect(() => parse('abc')).toThrow(/must look like/); expect(() => parse('devices')).toThrow(/must look like/); }); + + it('rejects unsupported units (y, month, week) with helpful hint', () => { + expect(() => parse('1y')).toThrow(/ms, s, m, h, d, w/); + expect(() => parse('1year')).toThrow(/ms, s, m, h, d, w/); + expect(() => parse('1month')).toThrow(/must look like/); + }); }); describe('stringArg', () => { diff --git a/tests/utils/flags.test.ts b/tests/utils/flags.test.ts index f437d51e..5cc7795e 100644 --- a/tests/utils/flags.test.ts +++ b/tests/utils/flags.test.ts @@ -4,6 +4,7 @@ import { isDryRun, getTimeout, getConfigPath, + parseDurationToMs, } from '../../src/utils/flags.js'; describe('utils/flags', () => { @@ -81,4 +82,32 @@ describe('utils/flags', () => { expect(getConfigPath()).toBeUndefined(); }); }); + + describe('parseDurationToMs', () => { + it('accepts ms / s / m / h units', () => { + expect(parseDurationToMs('500ms')).toBe(500); + expect(parseDurationToMs('30s')).toBe(30_000); + expect(parseDurationToMs('5m')).toBe(5 * 60_000); + expect(parseDurationToMs('2h')).toBe(2 * 60 * 60_000); + }); + + it('accepts d (days) and w (weeks) units', () => { + expect(parseDurationToMs('1d')).toBe(24 * 60 * 60_000); + expect(parseDurationToMs('7d')).toBe(7 * 24 * 60 * 60_000); + expect(parseDurationToMs('1w')).toBe(7 * 24 * 60 * 60_000); + expect(parseDurationToMs('2w')).toBe(14 * 24 * 60 * 60_000); + }); + + it('treats bare numbers as milliseconds', () => { + expect(parseDurationToMs('1000')).toBe(1000); + }); + + it('rejects unsupported units and malformed values', () => { + expect(parseDurationToMs('1y')).toBeNull(); + expect(parseDurationToMs('1year')).toBeNull(); + expect(parseDurationToMs('1month')).toBeNull(); + expect(parseDurationToMs('abc')).toBeNull(); + expect(parseDurationToMs('')).toBeNull(); + }); + }); }); From 7ff291b3ed100c57481420b71cae76779450c082 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 21:54:35 +0800 Subject: [PATCH 22/34] fix(name-resolver): surface all 6 strategies in help and agent-bootstrap The --name-strategy help text hard-coded the strategy list in three places, which drifted from the source of truth (ALL_STRATEGIES in name-resolver.ts). Export ALL_STRATEGIES and generate the help text from it. Also expose the list as a top-level 'nameStrategies' field in agent-bootstrap so agents can discover all six strategies (exact, prefix, substring, fuzzy, first, require-unique) without grepping help text. Fixes bug #51. --- package-lock.json | 4 ++-- src/commands/agent-bootstrap.ts | 2 ++ src/commands/devices.ts | 8 ++++---- src/utils/name-resolver.ts | 4 ++-- tests/commands/agent-bootstrap.test.ts | 4 ++++ 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index e2d62225..eef6365d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@switchbot/openapi-cli", - "version": "2.5.0", + "version": "2.5.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@switchbot/openapi-cli", - "version": "2.5.0", + "version": "2.5.1", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.29.0", diff --git a/src/commands/agent-bootstrap.ts b/src/commands/agent-bootstrap.ts index 212e98f3..a7d65d0a 100644 --- a/src/commands/agent-bootstrap.ts +++ b/src/commands/agent-bootstrap.ts @@ -4,6 +4,7 @@ import { loadCache } from '../devices/cache.js'; import { getEffectiveCatalog } from '../devices/catalog.js'; import { readProfileMeta } from '../config.js'; import { todayUsage, DAILY_QUOTA } from '../utils/quota.js'; +import { ALL_STRATEGIES } from '../utils/name-resolver.js'; import { createRequire } from 'node:module'; const require = createRequire(import.meta.url); @@ -123,6 +124,7 @@ Examples: identity: IDENTITY, quickReference: QUICK_REFERENCE, safetyTiers: SAFETY_TIERS, + nameStrategies: [...ALL_STRATEGIES], profile: meta ? { label: meta.label ?? null, diff --git a/src/commands/devices.ts b/src/commands/devices.ts index 568d673a..f8fb2247 100644 --- a/src/commands/devices.ts +++ b/src/commands/devices.ts @@ -5,7 +5,7 @@ import { resolveFormat, resolveFields, renderRows } from '../utils/format.js'; import { findCatalogEntry, getEffectiveCatalog, DeviceCatalogEntry } from '../devices/catalog.js'; import { getCachedDevice } from '../devices/cache.js'; import { loadDeviceMeta } from '../devices/device-meta.js'; -import { resolveDeviceId, NameResolveStrategy } from '../utils/name-resolver.js'; +import { resolveDeviceId, NameResolveStrategy, ALL_STRATEGIES } from '../utils/name-resolver.js'; import { fetchDeviceList, fetchDeviceStatus, @@ -217,7 +217,7 @@ Examples: .description('Query the real-time status of a specific device') .argument('[deviceId]', 'Device ID from "devices list" (or use --name or --ids)') .option('--name ', 'Resolve device by fuzzy name instead of deviceId', stringArg('--name')) - .option('--name-strategy ', 'Name match strategy: exact|prefix|substring|fuzzy|first|require-unique (default: fuzzy)', stringArg('--name-strategy')) + .option('--name-strategy ', `Name match strategy: ${ALL_STRATEGIES.join('|')} (default: fuzzy)`, stringArg('--name-strategy')) .option('--name-type ', 'Narrow --name by device type (e.g. "Bot", "Color Bulb")', stringArg('--name-type')) .option('--name-category ', 'Narrow --name by category: physical|ir', enumArg('--name-category', ['physical', 'ir'] as const)) .option('--name-room ', 'Narrow --name by room name (substring match)', stringArg('--name-room')) @@ -319,7 +319,7 @@ Examples: .argument('[cmd]', 'Command name, e.g. turnOn, turnOff, setColor, setBrightness, setAll, startClean') .argument('[parameter]', 'Command parameter. Omit for commands like turnOn/turnOff (defaults to "default"). Format depends on the command (see below).') .option('--name ', 'Resolve device by fuzzy name instead of deviceId', stringArg('--name')) - .option('--name-strategy ', 'Name match strategy: exact|prefix|substring|fuzzy|first|require-unique (default for command: require-unique)', stringArg('--name-strategy')) + .option('--name-strategy ', `Name match strategy: ${ALL_STRATEGIES.join('|')} (default for command: require-unique)`, stringArg('--name-strategy')) .option('--name-type ', 'Narrow --name by device type (e.g. "Bot", "Color Bulb")', stringArg('--name-type')) .option('--name-category ', 'Narrow --name by category: physical|ir', enumArg('--name-category', ['physical', 'ir'] as const)) .option('--name-room ', 'Narrow --name by room name (substring match)', stringArg('--name-room')) @@ -667,7 +667,7 @@ Examples: .description('Describe a device by ID: metadata + supported commands + status fields (1 API call)') .argument('[deviceId]', 'Target device ID (or use --name)') .option('--name ', 'Resolve device by fuzzy name instead of deviceId', stringArg('--name')) - .option('--name-strategy ', 'Name match strategy: exact|prefix|substring|fuzzy|first|require-unique (default: fuzzy)', stringArg('--name-strategy')) + .option('--name-strategy ', `Name match strategy: ${ALL_STRATEGIES.join('|')} (default: fuzzy)`, stringArg('--name-strategy')) .option('--name-type ', 'Narrow --name by device type', stringArg('--name-type')) .option('--name-category ', 'Narrow --name by category: physical|ir', enumArg('--name-category', ['physical', 'ir'] as const)) .option('--name-room ', 'Narrow --name by room name (substring match)', stringArg('--name-room')) diff --git a/src/utils/name-resolver.ts b/src/utils/name-resolver.ts index 0ed87fed..34f1b98a 100644 --- a/src/utils/name-resolver.ts +++ b/src/utils/name-resolver.ts @@ -29,9 +29,9 @@ export type NameResolveResult = | { ok: false; ambiguous: true; candidates: NameMatch[] } | { ok: false; ambiguous: false }; -const ALL_STRATEGIES: NameResolveStrategy[] = [ +export const ALL_STRATEGIES: readonly NameResolveStrategy[] = [ 'exact', 'prefix', 'substring', 'fuzzy', 'first', 'require-unique', -]; +] as const; export function isValidStrategy(s: string): s is NameResolveStrategy { return (ALL_STRATEGIES as string[]).includes(s); diff --git a/tests/commands/agent-bootstrap.test.ts b/tests/commands/agent-bootstrap.test.ts index 1a8290a1..b0cd1f42 100644 --- a/tests/commands/agent-bootstrap.test.ts +++ b/tests/commands/agent-bootstrap.test.ts @@ -66,6 +66,10 @@ describe('agent-bootstrap', () => { expect(data.identity).toBeDefined(); expect(data.safetyTiers).toBeDefined(); expect(data.quickReference).toBeDefined(); + expect(Array.isArray(data.nameStrategies)).toBe(true); + expect(data.nameStrategies).toEqual([ + 'exact', 'prefix', 'substring', 'fuzzy', 'first', 'require-unique', + ]); expect(Array.isArray(data.devices)).toBe(true); expect((data.devices as unknown[]).length).toBe(1); expect(data.catalog).toBeDefined(); From c2c8ab51e96138af568a95093afb42cc5cfb0151 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 21:56:34 +0800 Subject: [PATCH 23/34] fix(mcp): reject empty query in search_catalog Empty queries caused search_catalog to scan the entire catalog, which both hid the agent's intent (was the empty string a bug, or deliberate 'list all'?) and produced unnecessarily large MCP payloads. Reject with a usage error and suggest list_catalog_types for the enumerate-everything case. Fixes bug #57. --- src/commands/mcp.ts | 12 +++++++++++- tests/commands/mcp.test.ts | 22 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/commands/mcp.ts b/src/commands/mcp.ts index 3cb15052..9e2b8c5e 100644 --- a/src/commands/mcp.ts +++ b/src/commands/mcp.ts @@ -533,7 +533,7 @@ API docs: https://github.com/OpenWonderLabs/SwitchBotAPI`, 'Search the built-in device catalog by type name or alias. Returns matching entries with their commands, roles, destructive flags, and status fields. No API call.', _meta: { agentSafetyTier: 'read' }, inputSchema: z.object({ - query: z.string().describe('Search query (matches type and aliases, case-insensitive). Use empty string to list all.'), + query: z.string().describe('Search query (matches type and aliases, case-insensitive). Must be non-empty; use list_catalog_types to enumerate instead.'), limit: z.number().int().min(1).max(100).optional().default(20).describe('Max entries returned (default 20)'), }).strict(), outputSchema: { @@ -557,6 +557,16 @@ API docs: https://github.com/OpenWonderLabs/SwitchBotAPI`, }, }, async ({ query, limit }) => { + if (query.trim() === '') { + return mcpError( + 'usage', + 2, + 'search_catalog requires a non-empty query.', + { + hint: "Pass a search term like 'Bot' or 'Hub', or call list_catalog_types to enumerate all types without a query.", + }, + ); + } const hits = searchCatalog(query, limit); const structured = { results: hits as unknown as Array>, total: hits.length }; return { diff --git a/tests/commands/mcp.test.ts b/tests/commands/mcp.test.ts index 4e86d417..bf14d701 100644 --- a/tests/commands/mcp.test.ts +++ b/tests/commands/mcp.test.ts @@ -287,6 +287,28 @@ describe('mcp server', () => { expect(parsed.some((e: { type: string }) => e.type === 'Strip Light')).toBe(true); }); + it('search_catalog rejects an empty query with a usage error', async () => { + const { client } = await pair(); + const res = await client.callTool({ + name: 'search_catalog', + arguments: { query: '' }, + }); + expect(res.isError).toBe(true); + const structured = (res as { structuredContent?: { error?: { kind?: string; message?: string; hint?: string } } }).structuredContent; + expect(structured?.error?.kind).toBe('usage'); + expect(structured?.error?.message).toMatch(/non-empty query/i); + expect(structured?.error?.hint).toMatch(/list_catalog_types/); + }); + + it('search_catalog rejects whitespace-only query', async () => { + const { client } = await pair(); + const res = await client.callTool({ + name: 'search_catalog', + arguments: { query: ' ' }, + }); + expect(res.isError).toBe(true); + }); + it('run_scene POSTs the scene execute endpoint', async () => { apiMock.__instance.post.mockResolvedValueOnce({ data: { statusCode: 100, body: {} } }); const { client } = await pair(); From dd40f4ab5abd07e11050f4e0ad10891fc7a136df Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 21:58:50 +0800 Subject: [PATCH 24/34] fix(batch): accept --idempotency-key as alias for --idempotency-key-prefix The 2.4.0 release notes and user-facing docs referred to `--idempotency-key`, but the actual flag has always been `--idempotency-key-prefix` (the prefix gets the deviceId appended to form per-device keys). Accepting both spellings removes the documentation/implementation mismatch without breaking existing scripts. Supplying both forms is rejected to keep the intent unambiguous. Fixes bug #30. --- src/commands/batch.ts | 12 +++++++++- tests/commands/batch.test.ts | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/commands/batch.ts b/src/commands/batch.ts index dbe8531a..113ce7f7 100644 --- a/src/commands/batch.ts +++ b/src/commands/batch.ts @@ -1,7 +1,7 @@ import { Command } from 'commander'; import type { AxiosInstance } from 'axios'; import { intArg, enumArg, stringArg } from '../utils/arg-parsers.js'; -import { printJson, isJsonMode, handleError, buildErrorPayload, type ErrorPayload } from '../utils/output.js'; +import { printJson, isJsonMode, handleError, buildErrorPayload, UsageError, type ErrorPayload } from '../utils/output.js'; import { fetchDeviceList, executeCommand, @@ -157,6 +157,7 @@ export function registerBatchCommand(devices: Command): void { .option('--type ', '"command" (default) or "customize" for user-defined IR buttons', enumArg('--type', COMMAND_TYPES), 'command') .option('--stdin', 'Read deviceIds from stdin, one per line (same as trailing "-")') .option('--idempotency-key-prefix ', 'Client-supplied prefix for idempotency keys (key per device: -). process-local 60s window; cache is per Node process (MCP session, batch run, plan run). Independent CLI invocations do not share cache.', stringArg('--idempotency-key-prefix')) + .option('--idempotency-key ', 'Alias for --idempotency-key-prefix.', stringArg('--idempotency-key')) .addHelpText('after', ` Targets are resolved in this priority order: 1. --ids when present (explicit deviceIds) @@ -211,12 +212,21 @@ Examples: type: string; stdin?: boolean; idempotencyKeyPrefix?: string; + idempotencyKey?: string; }, commandObj: Command ) => { // Trailing "-" sentinel selects stdin mode. const extra = commandObj.args ?? []; const readStdin = Boolean(options.stdin) || extra.includes('-'); + // Accept --idempotency-key as alias; reject when both forms are supplied. + if (options.idempotencyKey !== undefined && options.idempotencyKeyPrefix !== undefined) { + handleError(new UsageError('Use either --idempotency-key or --idempotency-key-prefix, not both.')); + return; + } + if (options.idempotencyKey !== undefined && options.idempotencyKeyPrefix === undefined) { + options.idempotencyKeyPrefix = options.idempotencyKey; + } let client: AxiosInstance | undefined; const getClient = (): AxiosInstance => (client ??= createClient()); diff --git a/tests/commands/batch.test.ts b/tests/commands/batch.test.ts index 5605e494..71f92054 100644 --- a/tests/commands/batch.test.ts +++ b/tests/commands/batch.test.ts @@ -402,6 +402,49 @@ describe('devices batch', () => { expect(parsed.data.plan.steps.map((s: { deviceId: string }) => s.deviceId).sort()).toEqual(['BOT1', 'BOT2']); }); + it('--idempotency-key alias sets the same prefix as --idempotency-key-prefix', async () => { + flagsMock.dryRun = true; + apiMock.__instance.get.mockResolvedValue({ data: { statusCode: 100, body: DEVICE_LIST_BODY } }); + + const result = await runCli(registerDevicesCommand, [ + '--json', + 'devices', + 'batch', + 'turnOn', + '--filter', + 'type=Bot', + '--plan', + '--idempotency-key', + 'foo', + ]); + + expect(result.exitCode).toBeNull(); + const parsed = JSON.parse(result.stdout[0]); + const keys = parsed.data.plan.steps.map((s: { idempotencyKey?: string }) => s.idempotencyKey).sort(); + expect(keys).toEqual(['foo-BOT1', 'foo-BOT2']); + }); + + it('rejects when both --idempotency-key and --idempotency-key-prefix are supplied', async () => { + flagsMock.dryRun = true; + apiMock.__instance.get.mockResolvedValue({ data: { statusCode: 100, body: DEVICE_LIST_BODY } }); + + const result = await runCli(registerDevicesCommand, [ + 'devices', + 'batch', + 'turnOn', + '--filter', + 'type=Bot', + '--plan', + '--idempotency-key', + 'foo', + '--idempotency-key-prefix', + 'bar', + ]); + + expect(result.exitCode).toBe(2); + expect(result.stderr.join('\n')).toMatch(/either --idempotency-key or --idempotency-key-prefix/); + }); + it('bug28: batch over IR devices attaches subKind + verification and sets summary.unverifiableCount', async () => { cacheMock.map.set('IR1', { type: 'Air Conditioner', name: 'Living Room AC', category: 'ir' }); cacheMock.map.set('IR2', { type: 'Air Conditioner', name: 'Bedroom AC', category: 'ir' }); From 7474393a732cbc330fa2a90940164ed09bb7fab0 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 22:02:07 +0800 Subject: [PATCH 25/34] feat(batch): --skip-offline flag to short-circuit offline devices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an opt-in --skip-offline flag that checks the local status cache before dispatching commands. Devices whose cached body shows onlineStatus === 'offline' are recorded under result.skipped[] (with reason: 'offline') instead of being hit. Cache miss falls through to the normal send path, so the flag never introduces new network calls for the preflight itself. Default remains off — patch release must not change existing batch behavior. Fixes bug #33. --- src/commands/batch.ts | 28 +++++++++++++++-- tests/commands/batch.test.ts | 58 +++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/src/commands/batch.ts b/src/commands/batch.ts index 113ce7f7..b9125802 100644 --- a/src/commands/batch.ts +++ b/src/commands/batch.ts @@ -12,7 +12,7 @@ import { createClient } from '../api/client.js'; import { parseFilter, applyFilter, FilterSyntaxError } from '../utils/filter.js'; import { isDryRun } from '../utils/flags.js'; import { DryRunSignal } from '../api/client.js'; -import { getCachedTypeMap, getCachedDevice } from '../devices/cache.js'; +import { getCachedTypeMap, getCachedDevice, loadStatusCache } from '../devices/cache.js'; interface BatchStepTiming { startedAt: string; @@ -33,6 +33,7 @@ interface BatchResult { }; } & BatchStepTiming>; failed: Array<{ deviceId: string; error: ErrorPayload } & BatchStepTiming>; + skipped?: Array<{ deviceId: string; reason: 'offline' }>; summary: { total: number; ok: number; @@ -158,6 +159,7 @@ export function registerBatchCommand(devices: Command): void { .option('--stdin', 'Read deviceIds from stdin, one per line (same as trailing "-")') .option('--idempotency-key-prefix ', 'Client-supplied prefix for idempotency keys (key per device: -). process-local 60s window; cache is per Node process (MCP session, batch run, plan run). Independent CLI invocations do not share cache.', stringArg('--idempotency-key-prefix')) .option('--idempotency-key ', 'Alias for --idempotency-key-prefix.', stringArg('--idempotency-key')) + .option('--skip-offline', 'Skip devices whose cached status is offline (no API call; cache miss → send as usual).') .addHelpText('after', ` Targets are resolved in this priority order: 1. --ids when present (explicit deviceIds) @@ -213,6 +215,7 @@ Examples: stdin?: boolean; idempotencyKeyPrefix?: string; idempotencyKey?: string; + skipOffline?: boolean; }, commandObj: Command ) => { @@ -272,6 +275,24 @@ Examples: | 'command' | 'customize'; + // --skip-offline: preflight using the status cache (no network). Cache + // miss = send as usual; only definite "offline" cached entries skip. + const preSkipped: Array<{ deviceId: string; reason: 'offline' }> = []; + if (options.skipOffline && resolved.ids.length > 0) { + const statusCache = loadStatusCache(); + const kept: string[] = []; + for (const id of resolved.ids) { + const entry = statusCache.entries[id]; + const online = entry?.body?.onlineStatus; + if (online === 'offline') { + preSkipped.push({ deviceId: id, reason: 'offline' }); + } else { + kept.push(id); + } + } + resolved = { ...resolved, ids: kept }; + } + // Pre-flight: identify destructive targets before spending API calls. const blockedForDestructive: Array<{ deviceId: string; reason: string }> = []; for (const id of resolved.ids) { @@ -463,11 +484,12 @@ Examples: finishedAt: f.finishedAt, durationMs: f.durationMs, })), + ...(preSkipped.length > 0 ? { skipped: preSkipped } : {}), summary: { - total: resolved.ids.length, + total: resolved.ids.length + preSkipped.length, ok: succeeded.length, failed: failed.length, - skipped: dryRunned.length, + skipped: dryRunned.length + preSkipped.length, durationMs: Date.now() - startedAt, unverifiableCount: succeeded.filter((s) => getCachedDevice(s.deviceId)?.category === 'ir').length, schemaVersion: '1.1', diff --git a/tests/commands/batch.test.ts b/tests/commands/batch.test.ts index 71f92054..4e406973 100644 --- a/tests/commands/batch.test.ts +++ b/tests/commands/batch.test.ts @@ -28,6 +28,7 @@ vi.mock('../../src/api/client.js', () => ({ // Cache: keep deterministic across tests. const cacheMock = vi.hoisted(() => ({ map: new Map(), + statusMap: new Map }>(), getCachedDevice: vi.fn((id: string) => cacheMock.map.get(id) ?? null), getCachedTypeMap: vi.fn((ids?: Iterable) => { const out = new Map(); @@ -39,6 +40,7 @@ const cacheMock = vi.hoisted(() => ({ return out; }), updateCacheFromDeviceList: vi.fn(), + loadStatusCache: vi.fn(() => ({ entries: Object.fromEntries(cacheMock.statusMap) })), })); vi.mock('../../src/devices/cache.js', () => ({ getCachedDevice: cacheMock.getCachedDevice, @@ -52,7 +54,7 @@ vi.mock('../../src/devices/cache.js', () => ({ setCachedStatus: vi.fn(), clearStatusCache: vi.fn(), resetStatusCache: vi.fn(), - loadStatusCache: vi.fn(() => ({ entries: {} })), + loadStatusCache: cacheMock.loadStatusCache, describeCache: vi.fn(() => ({ list: { path: '', exists: false }, status: { path: '', exists: false, entryCount: 0 }, @@ -116,6 +118,7 @@ describe('devices batch', () => { apiMock.__instance.post.mockReset(); apiMock.createClient.mockClear(); cacheMock.map.clear(); + cacheMock.statusMap.clear(); cacheMock.getCachedDevice.mockClear(); cacheMock.getCachedTypeMap.mockClear(); flagsMock.dryRun = false; @@ -445,6 +448,59 @@ describe('devices batch', () => { expect(result.stderr.join('\n')).toMatch(/either --idempotency-key or --idempotency-key-prefix/); }); + it('--skip-offline skips devices whose cached status is offline', async () => { + cacheMock.map.set('BOT1', { type: 'Bot', name: 'Kitchen', category: 'physical' }); + cacheMock.map.set('BOT2', { type: 'Bot', name: 'Office', category: 'physical' }); + cacheMock.statusMap.set('BOT2', { + fetchedAt: new Date().toISOString(), + body: { onlineStatus: 'offline', power: 'off' }, + }); + apiMock.__instance.post.mockResolvedValue({ data: { statusCode: 100, body: {} } }); + + const result = await runCli(registerDevicesCommand, [ + '--json', + 'devices', + 'batch', + 'turnOn', + '--ids', + 'BOT1,BOT2', + '--skip-offline', + ]); + + expect(result.exitCode).toBeNull(); + expect(apiMock.__instance.post).toHaveBeenCalledTimes(1); + const parsed = JSON.parse(result.stdout[0]); + expect(parsed.data.summary.ok).toBe(1); + expect(parsed.data.summary.total).toBe(2); + expect(parsed.data.summary.skipped).toBe(1); + expect(parsed.data.skipped).toEqual([{ deviceId: 'BOT2', reason: 'offline' }]); + expect(parsed.data.succeeded[0].deviceId).toBe('BOT1'); + }); + + it('without --skip-offline, offline-cached devices are still sent', async () => { + cacheMock.map.set('BOT1', { type: 'Bot', name: 'Kitchen', category: 'physical' }); + cacheMock.map.set('BOT2', { type: 'Bot', name: 'Office', category: 'physical' }); + cacheMock.statusMap.set('BOT2', { + fetchedAt: new Date().toISOString(), + body: { onlineStatus: 'offline', power: 'off' }, + }); + apiMock.__instance.post.mockResolvedValue({ data: { statusCode: 100, body: {} } }); + + const result = await runCli(registerDevicesCommand, [ + '--json', + 'devices', + 'batch', + 'turnOn', + '--ids', + 'BOT1,BOT2', + ]); + + expect(result.exitCode).toBeNull(); + expect(apiMock.__instance.post).toHaveBeenCalledTimes(2); + const parsed = JSON.parse(result.stdout[0]); + expect(parsed.data.skipped).toBeUndefined(); + }); + it('bug28: batch over IR devices attaches subKind + verification and sets summary.unverifiableCount', async () => { cacheMock.map.set('IR1', { type: 'Air Conditioner', name: 'Living Room AC', category: 'ir' }); cacheMock.map.set('IR2', { type: 'Air Conditioner', name: 'Bedroom AC', category: 'ir' }); From fd01575d2d5f1361b1bb8ee2c841a968499b7d75 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 22:04:29 +0800 Subject: [PATCH 26/34] fix(devices): accept negative positional parameters for setBrightness, etc. Commander treated negative numbers like '-1' as unknown option tokens and rejected the whole invocation with 'error: unknown option -1'. Enable allowUnknownOption on the 'devices command' subcommand so negative numbers flow through as the parameter positional; they reach the API (and are rejected by device validators if out of range) instead of failing at argv parsing. Trade-off: unknown flag typos on this subcommand (e.g. '--dryrun' instead of '--dry-run') are now silently ignored instead of erroring. Acceptable here because the subcommand's surface area is small and the parameter validator still catches obviously malformed values. Fixes bug #53. --- src/commands/devices.ts | 3 ++- tests/commands/devices.test.ts | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/commands/devices.ts b/src/commands/devices.ts index f8fb2247..1233ab3d 100644 --- a/src/commands/devices.ts +++ b/src/commands/devices.ts @@ -317,7 +317,8 @@ Examples: .description('Send a control command to a device') .argument('[deviceId]', 'Target device ID (or use --name)') .argument('[cmd]', 'Command name, e.g. turnOn, turnOff, setColor, setBrightness, setAll, startClean') - .argument('[parameter]', 'Command parameter. Omit for commands like turnOn/turnOff (defaults to "default"). Format depends on the command (see below).') + .argument('[parameter]', 'Command parameter. Omit for commands like turnOn/turnOff (defaults to "default"). Format depends on the command (see below). Negative numbers like -1 are accepted as-is (use `--` before them only if Commander mis-parses in your shell).') + .allowUnknownOption() .option('--name ', 'Resolve device by fuzzy name instead of deviceId', stringArg('--name')) .option('--name-strategy ', `Name match strategy: ${ALL_STRATEGIES.join('|')} (default for command: require-unique)`, stringArg('--name-strategy')) .option('--name-type ', 'Narrow --name by device type (e.g. "Bot", "Color Bulb")', stringArg('--name-type')) diff --git a/tests/commands/devices.test.ts b/tests/commands/devices.test.ts index 73cbd158..8969863c 100644 --- a/tests/commands/devices.test.ts +++ b/tests/commands/devices.test.ts @@ -884,6 +884,15 @@ describe('devices command', () => { await runCmd('setBrightness', '100'); expectPost('setBrightness', 100); }); + it('setBrightness -1 (negative positional parameter reaches validation)', async () => { + // Regression for bug #53: Commander used to swallow "-1" as an unknown + // option token. With allowUnknownOption on the `command` subcommand + // negative numbers are now treated as the parameter positional and + // reach the API (the device then returns 190 for out-of-range values, + // but that's a device-layer concern, not a CLI parsing failure). + await runCmd('setBrightness', '-1'); + expectPost('setBrightness', -1); + }); it('setColorTemperature', async () => { await runCmd('setColorTemperature', '4000'); expectPost('setColorTemperature', 4000); From ba68033b1e414bec43c5b0abf346f5b0f9b98303 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 22:07:38 +0800 Subject: [PATCH 27/34] feat(watch,events): --for duration alias complementing --max MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds --for to 'devices watch', 'events tail', and 'events mqtt-tail' as a time-based stop condition (vs --max which counts ticks/events). When both are provided, whichever limit hits first wins. Implemented via a setTimeout that aborts the existing AbortController — the timer is cleared in the cleanup/finally paths so long-running loops that exit via other routes don't leak handles. Fixes bug #52. --- src/commands/events.ts | 18 ++++++++++++++++-- src/commands/watch.ts | 8 ++++++++ tests/commands/watch.test.ts | 13 +++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/commands/events.ts b/src/commands/events.ts index 46913d09..f8568f9a 100644 --- a/src/commands/events.ts +++ b/src/commands/events.ts @@ -2,7 +2,8 @@ import { Command } from 'commander'; import http from 'node:http'; import crypto from 'node:crypto'; import { printJson, isJsonMode, handleError, UsageError } from '../utils/output.js'; -import { intArg, stringArg } from '../utils/arg-parsers.js'; +import { intArg, stringArg, durationArg } from '../utils/arg-parsers.js'; +import { parseDurationToMs } from '../utils/flags.js'; import { SwitchBotMqttClient } from '../mqtt/client.js'; import { fetchMqttCredential } from '../mqtt/credential.js'; import { tryLoadConfig } from '../config.js'; @@ -156,6 +157,7 @@ export function registerEventsCommand(program: Command): void { .option('--path

', `HTTP path to match (default "${DEFAULT_PATH}"; use "*" for all paths)`, stringArg('--path'), DEFAULT_PATH) .option('--filter ', 'Filter events, e.g. "deviceId=ABC123" or "type=Bot" (comma-separated)', stringArg('--filter')) .option('--max ', 'Stop after N matching events (default: run until Ctrl-C)', intArg('--max', { min: 1 })) + .option('--for ', 'Stop after elapsed time (e.g. "5m", "30s"). Combines with --max: first limit wins.', durationArg('--for')) .addHelpText( 'after', ` @@ -180,7 +182,7 @@ Examples: $ switchbot events tail --filter 'type=WoMeter' --max 5 --json `, ) - .action(async (options: { port: string; path: string; filter?: string; max?: string }) => { + .action(async (options: { port: string; path: string; filter?: string; max?: string; for?: string }) => { try { const port = Number(options.port); if (!Number.isInteger(port) || port <= 0 || port > 65535) { @@ -190,10 +192,14 @@ Examples: if (maxMatched !== null && (!Number.isFinite(maxMatched) || maxMatched < 1)) { throw new UsageError(`Invalid --max "${options.max}". Must be a positive integer.`); } + const forMs = options.for ? parseDurationToMs(options.for) : null; const filter = parseFilter(options.filter); let matchedCount = 0; const ac = new AbortController(); + const forTimer = forMs !== null && forMs > 0 + ? setTimeout(() => ac.abort(), forMs) + : null; await new Promise((resolve, reject) => { let server: http.Server | null = null; try { @@ -220,6 +226,7 @@ Examples: if (!isJsonMode()) console.error(startMsg); const cleanup = () => { + if (forTimer) clearTimeout(forTimer); server?.close(); resolve(); }; @@ -237,6 +244,7 @@ Examples: .description('Subscribe to SwitchBot MQTT shadow events and stream them as JSONL') .option('--topic ', 'MQTT topic filter (default: SwitchBot shadow topic from credential)', stringArg('--topic')) .option('--max ', 'Stop after N events (default: run until Ctrl-C)', intArg('--max', { min: 1 })) + .option('--for ', 'Stop after elapsed time (e.g. "5m", "30s"). Combines with --max: first limit wins.', durationArg('--for')) .option( '--sink ', 'Output sink: stdout (default), file, webhook, openclaw, telegram, homeassistant (repeatable)', @@ -300,6 +308,7 @@ Examples: .action(async (options: { topic?: string; max?: string; + for?: string; sink: string[]; sinkFile?: string; webhookUrl?: string; @@ -318,6 +327,7 @@ Examples: if (maxEvents !== null && (!Number.isInteger(maxEvents) || maxEvents < 1)) { throw new UsageError(`Invalid --max "${options.max}". Must be a positive integer.`); } + const forMs = options.for ? parseDurationToMs(options.for) : null; const loaded = tryLoadConfig(); if (!loaded) { @@ -381,6 +391,9 @@ Examples: let eventCount = 0; const ac = new AbortController(); + const forTimer = forMs !== null && forMs > 0 + ? setTimeout(() => ac.abort(), forMs) + : null; const client = new SwitchBotMqttClient( credential, () => fetchMqttCredential(loaded.token, loaded.secret), @@ -472,6 +485,7 @@ Examples: await new Promise((resolve) => { const cleanup = () => { + if (forTimer) clearTimeout(forTimer); process.removeListener('SIGINT', cleanup); process.removeListener('SIGTERM', cleanup); unsub(); diff --git a/src/commands/watch.ts b/src/commands/watch.ts index 1192dd84..a2869488 100644 --- a/src/commands/watch.ts +++ b/src/commands/watch.ts @@ -81,6 +81,7 @@ export function registerWatchCommand(devices: Command): void { '30s', ) .option('--max ', 'Stop after N ticks (default: run until Ctrl-C)', intArg('--max', { min: 1 })) + .option('--for ', 'Stop after elapsed time (e.g. "5m", "30s"). Combines with --max: first limit wins.', durationArg('--for')) .option('--include-unchanged', 'Emit a tick even when no field changed') .addHelpText( 'after', @@ -106,6 +107,7 @@ Examples: name?: string; interval: string; max?: string; + for?: string; includeUnchanged?: boolean; }, ) => { @@ -133,12 +135,17 @@ Examples: maxTicks = Math.floor(n); } + const forMs = options.for ? parseDurationToMs(options.for) : null; + const fields: string[] | null = getFields() ?? null; const ac = new AbortController(); const onSig = () => ac.abort(); process.on('SIGINT', onSig); process.on('SIGTERM', onSig); + const forTimer = forMs !== null && forMs > 0 + ? setTimeout(() => ac.abort(), forMs) + : null; try { const prev = new Map>(); @@ -196,6 +203,7 @@ Examples: } catch (err) { handleError(err); } finally { + if (forTimer) clearTimeout(forTimer); process.off('SIGINT', onSig); process.off('SIGTERM', onSig); } diff --git a/tests/commands/watch.test.ts b/tests/commands/watch.test.ts index 094272c2..907fddb9 100644 --- a/tests/commands/watch.test.ts +++ b/tests/commands/watch.test.ts @@ -106,6 +106,19 @@ describe('devices watch', () => { expect(res.stderr.join('\n')).toMatch(/--max/); }); + it('--for stops the loop after elapsed time', async () => { + cacheMock.map.set('BOT1', { type: 'Bot', name: 'Kitchen', category: 'physical' }); + apiMock.__instance.get.mockResolvedValue({ + data: { statusCode: 100, body: { power: 'on', battery: 90 } }, + }); + const res = await runCli(registerDevicesCommand, [ + '--json', 'devices', 'watch', 'BOT1', '--interval', '1s', '--for', '200ms', + ]); + // --for triggers AbortController.abort() after 200ms; the loop exits + // cleanly with exit code null (no unhandled throw). + expect(res.exitCode).toBeNull(); + }, 3000); + it('emits one JSONL event per device on first tick with from:null (--max=1)', async () => { cacheMock.map.set('BOT1', { type: 'Bot', name: 'Kitchen', category: 'physical' }); apiMock.__instance.get.mockResolvedValueOnce({ From a2611546ded27880fea391fad72619b5d172653e Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 22:12:05 +0800 Subject: [PATCH 28/34] fix(events): emit __session_start envelope on mqtt-tail --json (bug #56) Previously, `events mqtt-tail --json` only emitted JSON after the MQTT broker connected. If credential fetch or broker connect failed (or the process exited early via --max/--for), downstream JSONL consumers saw zero lines and could not distinguish "never ran" from "ran but no events". Now an opening `__session_start` envelope is emitted immediately when --json is set, carrying { type, at, eventId, state:'connecting' }. It always appears as the first JSON line, before any credential fetch, mirroring the existing __connect/__disconnect control record pattern. --- src/commands/events.ts | 12 +++++++++++ tests/commands/events.test.ts | 39 ++++++++++++++++++++++++++++------- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/commands/events.ts b/src/commands/events.ts index f8568f9a..d0353e66 100644 --- a/src/commands/events.ts +++ b/src/commands/events.ts @@ -273,6 +273,7 @@ Output (JSONL, one event per line): { "t": "", "eventId": "", "topic": "", "payload": } Control records (interleaved, no "payload" field — use type-prefix to filter): + { "type": "__session_start", "at": "", "eventId": "", "state": "connecting" } before credential fetch (JSON mode only) { "type": "__connect", "at": "", "eventId": "" } first successful connect { "type": "__reconnect", "at": "", "eventId": "" } connect after a disconnect { "type": "__disconnect", "at": "", "eventId": "" } reconnecting or failed @@ -386,6 +387,17 @@ Examples: if (!isJsonMode()) { console.error('Fetching MQTT credentials from SwitchBot service…'); } + // Emit a __session_start envelope immediately (before any credential + // fetch) so JSON consumers can distinguish "connecting" from "never + // connected" even when mqtt-tail exits before the broker connects. + if (isJsonMode()) { + printJson({ + type: '__session_start', + at: new Date().toISOString(), + eventId: crypto.randomUUID(), + state: 'connecting', + }); + } const credential = await fetchMqttCredential(loaded.token, loaded.secret); const topic = options.topic ?? credential.topics.status; diff --git a/tests/commands/events.test.ts b/tests/commands/events.test.ts index 3f5c7e40..0aabad23 100644 --- a/tests/commands/events.test.ts +++ b/tests/commands/events.test.ts @@ -282,9 +282,12 @@ describe('events mqtt-tail', () => { const res = await runCli(registerEventsCommand, ['events', 'mqtt-tail', '--max', '1']); expect(res.exitCode).toBe(null); - const jsonLines = res.stdout.filter((l) => l.trim().startsWith('{')); - expect(jsonLines).toHaveLength(1); - const parsed = JSON.parse(jsonLines[0]) as { t: string; topic: string; payload: unknown }; + const jsonLines = res.stdout + .filter((l) => l.trim().startsWith('{')) + .map((l) => JSON.parse(l) as { type?: string; topic?: string; payload?: unknown; t?: string }); + const events = jsonLines.filter((j) => typeof j.type !== 'string' || !j.type.startsWith('__')); + expect(events).toHaveLength(1); + const parsed = events[0] as { t: string; topic: string; payload: unknown }; expect(parsed.topic).toBe('test/topic'); expect(parsed.payload).toEqual({ state: 'on' }); expect(typeof parsed.t).toBe('string'); @@ -295,11 +298,15 @@ describe('events mqtt-tail', () => { const res = await runCli(registerEventsCommand, ['--json', 'events', 'mqtt-tail', '--max', '1']); expect(res.exitCode).toBe(null); - const jsonLines = res.stdout.filter((l) => l.trim().startsWith('{')); - expect(jsonLines).toHaveLength(1); - const parsed = JSON.parse(jsonLines[0]) as { schemaVersion: string; data: { topic: string } }; - expect(parsed.schemaVersion).toBe('1.1'); - expect(parsed.data.topic).toBe('test/topic'); + const jsonLines = res.stdout + .filter((l) => l.trim().startsWith('{')) + .map((l) => JSON.parse(l) as { schemaVersion: string; data: { type?: string; topic?: string } }); + const events = jsonLines.filter( + (j) => typeof j.data?.type !== 'string' || !j.data.type.startsWith('__'), + ); + expect(events).toHaveLength(1); + expect(events[0].schemaVersion).toBe('1.1'); + expect(events[0].data.topic).toBe('test/topic'); }); it('exits 2 when --max is not a positive integer', async () => { @@ -349,6 +356,22 @@ describe('events mqtt-tail', () => { const disconnect = jsonLines.find((j) => (j as { type?: string }).type === '__disconnect'); expect(disconnect).toBeDefined(); }); + + it('emits __session_start envelope under --json before broker connect (bug #56)', async () => { + mqttMock.connectShouldFireMessage = true; + const res = await runCli(registerEventsCommand, ['--json', 'events', 'mqtt-tail', '--max', '1']); + const jsonLines = res.stdout + .filter((l) => l.trim().startsWith('{')) + .map((l) => JSON.parse(l) as { data: { type?: string; state?: string; at?: string; eventId?: string } }); + const sessionStart = jsonLines.find((j) => j.data?.type === '__session_start'); + expect(sessionStart).toBeDefined(); + expect(sessionStart!.data.state).toBe('connecting'); + expect(typeof sessionStart!.data.at).toBe('string'); + expect(typeof sessionStart!.data.eventId).toBe('string'); + // Must be the FIRST JSON line emitted so consumers see it even if broker + // never connects. + expect((jsonLines[0] as { data: { type?: string } }).data.type).toBe('__session_start'); + }); }); // --------------------------------------------------------------------------- From 7c50e50d7a9c8584aeb4b3454b209daf3f02e183 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 22:14:20 +0800 Subject: [PATCH 29/34] fix(devices): accept ~substring and =/regex/ operators in --filter DSL (bug #39) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous --filter only recognized key=value clauses with implicit substring semantics (except for category, which did exact match). Users couldn't explicitly request substring or regex, and there was no way to pattern-match device types beyond trivial prefix overlap. --filter now parses each comma-separated clause as one of: key=value — current behavior (substring; exact for category) key~value — explicit case-insensitive substring key=/pattern/ — case-insensitive regex; invalid regex exits 2 Supported keys (type, name, category, room) and multi-clause AND composition are unchanged. --- src/commands/devices.ts | 94 ++++++++++++++++++++++++++-------- tests/commands/devices.test.ts | 33 ++++++++++++ 2 files changed, 107 insertions(+), 20 deletions(-) diff --git a/src/commands/devices.ts b/src/commands/devices.ts index 1233ab3d..12adef2f 100644 --- a/src/commands/devices.ts +++ b/src/commands/devices.ts @@ -88,10 +88,12 @@ Examples: $ switchbot devices list --filter type="Air Conditioner" $ switchbot devices list --filter category=ir $ switchbot devices list --filter name=living,category=physical + $ switchbot devices list --filter 'name~living' # explicit substring + $ switchbot devices list --filter 'type=/Air.*/' # regex (case-insensitive) `) .option('--wide', 'Show all columns (controlType, family, roomID, room, hub, cloud)') .option('--show-hidden', 'Include devices hidden via "devices meta set --hide"') - .option('--filter ', 'Filter devices: "type=X", "name=X", "category=physical|ir", "room=X" (comma-separated key=value pairs)', stringArg('--filter')) + .option('--filter ', 'Filter devices: comma-separated clauses. Each clause is "key=value" (substring; exact for category), "key~value" (explicit substring), or "key=/regex/" (case-insensitive regex). Supported keys: type, name, category, room.', stringArg('--filter')) .action(async (options: { wide?: boolean; showHidden?: boolean; filter?: string }) => { try { const body = await fetchDeviceList(); @@ -101,34 +103,86 @@ Examples: const hubLocation = buildHubLocationMap(deviceList); - // Parse --filter into a simple predicate map - interface ListFilter { type?: string; name?: string; category?: string; room?: string; } - let listFilter: ListFilter | null = null; + // Parse --filter into a list of clauses. Each comma-separated pair is + // one of three shapes: + // key=value — current behavior (substring; exact for category) + // key~value — explicit case-insensitive substring + // key=/pattern/ — case-insensitive regex + interface FilterClause { + key: 'type' | 'name' | 'category' | 'room'; + op: 'eq' | 'sub' | 'regex'; + raw: string; + regex?: RegExp; + } + const SUPPORTED_KEYS = ['type', 'name', 'category', 'room'] as const; + let listClauses: FilterClause[] | null = null; if (options.filter) { - listFilter = {}; + listClauses = []; for (const pair of options.filter.split(',')) { - const eq = pair.indexOf('='); - if (eq === -1) throw new UsageError(`Invalid --filter pair "${pair.trim()}". Expected key=value.`); - const k = pair.slice(0, eq).trim(); - const v = pair.slice(eq + 1).trim(); - if (!['type', 'name', 'category', 'room'].includes(k)) { - throw new UsageError(`Unknown --filter key "${k}". Supported: type, name, category, room.`); + const trimmed = pair.trim(); + if (!trimmed) continue; + const regexMatch = /^([^=~]+)=\/(.*)\/$/.exec(trimmed); + const tildeIdx = trimmed.indexOf('~'); + const eqIdx = trimmed.indexOf('='); + let key: string; + let op: 'eq' | 'sub' | 'regex'; + let raw: string; + let regex: RegExp | undefined; + if (regexMatch) { + key = regexMatch[1].trim(); + op = 'regex'; + raw = regexMatch[2]; + try { + regex = new RegExp(raw, 'i'); + } catch (err) { + throw new UsageError( + `Invalid regex in --filter "${trimmed}": ${(err as Error).message}`, + ); + } + } else if (tildeIdx !== -1 && (eqIdx === -1 || tildeIdx < eqIdx)) { + key = trimmed.slice(0, tildeIdx).trim(); + op = 'sub'; + raw = trimmed.slice(tildeIdx + 1).trim().toLowerCase(); + } else if (eqIdx !== -1) { + key = trimmed.slice(0, eqIdx).trim(); + op = 'eq'; + raw = trimmed.slice(eqIdx + 1).trim().toLowerCase(); + } else { + throw new UsageError( + `Invalid --filter pair "${trimmed}". Expected key=value, key~value, or key=/regex/.`, + ); } - (listFilter as Record)[k] = v.toLowerCase(); + if (!(SUPPORTED_KEYS as readonly string[]).includes(key)) { + throw new UsageError( + `Unknown --filter key "${key}". Supported: ${SUPPORTED_KEYS.join(', ')}.`, + ); + } + listClauses.push({ key: key as FilterClause['key'], op, raw, regex }); } } const matchesFilter = (entry: { type: string; name: string; category: 'physical' | 'ir'; room: string }) => { - if (!listFilter) return true; - if (listFilter.type && !entry.type.toLowerCase().includes(listFilter.type)) return false; - if (listFilter.name && !entry.name.toLowerCase().includes(listFilter.name)) return false; - if (listFilter.category && entry.category !== listFilter.category) return false; - if (listFilter.room && !entry.room.toLowerCase().includes(listFilter.room)) return false; + if (!listClauses || listClauses.length === 0) return true; + for (const c of listClauses) { + const fieldVal = (entry as Record)[c.key] ?? ''; + const lower = fieldVal.toLowerCase(); + let ok: boolean; + if (c.op === 'regex') { + ok = c.regex!.test(fieldVal); + } else if (c.op === 'sub') { + ok = lower.includes(c.raw); + } else if (c.key === 'category') { + ok = lower === c.raw; + } else { + ok = lower.includes(c.raw); + } + if (!ok) return false; + } return true; }; if (fmt === 'json' && process.argv.includes('--json')) { - if (listFilter) { + if (listClauses) { const filteredDeviceList = deviceList.filter((d) => matchesFilter({ type: d.deviceType || '', name: d.deviceName, category: 'physical', room: d.roomName || '' }) ); @@ -187,7 +241,7 @@ Examples: } if (rows.length === 0 && fmt === 'table') { - console.log(listFilter ? 'No devices matched the filter.' : 'No devices found'); + console.log(listClauses ? 'No devices matched the filter.' : 'No devices found'); return; } @@ -200,7 +254,7 @@ Examples: }; renderRows(wideHeaders, rows, fmt, userFields ?? defaultFields, DEVICE_LIST_ALIASES); if (fmt === 'table') { - const totalLabel = listFilter + const totalLabel = listClauses ? `${rows.length} match(es) (${deviceList.length} physical + ${infraredRemoteList.length} IR before filter)` : `${deviceList.length} physical device(s), ${infraredRemoteList.length} IR remote device(s)`; console.log(`\nTotal: ${totalLabel}`); diff --git a/tests/commands/devices.test.ts b/tests/commands/devices.test.ts index 8969863c..95c1fe74 100644 --- a/tests/commands/devices.test.ts +++ b/tests/commands/devices.test.ts @@ -449,6 +449,39 @@ describe('devices command', () => { expect(out.data.deviceList).toHaveLength(3); expect(out.data.infraredRemoteList).toHaveLength(0); }); + + it('--filter name~Kitchen uses substring match (bug #39)', async () => { + apiMock.__instance.get.mockResolvedValue({ data: { body: sampleBody } }); + const res = await runCli(registerDevicesCommand, ['devices', 'list', '--filter', 'name~Kitchen', '--json']); + const out = JSON.parse(res.stdout.join('\n')); + expect(out.data.deviceList).toHaveLength(1); + expect(out.data.deviceList[0].deviceId).toBe('BLE-001'); + }); + + it('--filter type=/regex/ uses case-insensitive regex (bug #39)', async () => { + apiMock.__instance.get.mockResolvedValue({ data: { body: sampleBody } }); + const res = await runCli(registerDevicesCommand, ['devices', 'list', '--filter', 'type=/^Strip.*/', '--json']); + const out = JSON.parse(res.stdout.join('\n')); + expect(out.data.deviceList).toHaveLength(1); + expect(out.data.deviceList[0].deviceId).toBe('NOHUB-1'); + }); + + it('--filter with invalid regex exits 2 with UsageError (bug #39)', async () => { + apiMock.__instance.get.mockResolvedValue({ data: { body: sampleBody } }); + const res = await runCli(registerDevicesCommand, ['devices', 'list', '--filter', 'name=/[unterminated/']); + expect(res.exitCode).toBe(2); + expect(res.stderr.join('\n')).toMatch(/Invalid regex/i); + }); + + it('--filter combines AND clauses across ops (bug #39)', async () => { + apiMock.__instance.get.mockResolvedValue({ data: { body: sampleBody } }); + const res = await runCli(registerDevicesCommand, [ + 'devices', 'list', '--filter', 'category=physical,name~Lamp', '--json', + ]); + const out = JSON.parse(res.stdout.join('\n')); + expect(out.data.deviceList).toHaveLength(1); + expect(out.data.deviceList[0].deviceId).toBe('ABC123'); + }); }); // ===================================================================== From bc473b0edf730b0d7bd28f5c545ae9874e29c12b Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 22:24:58 +0800 Subject: [PATCH 30/34] fix(output): route --json errors to stdout so pipelines can decode them (bug #SYS-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously every error under --json landed on stderr, while every success landed on stdout — a pipeline like `cli --json devices status X | jq ...` saw nothing on the error path, making the structured envelope useless for automation. handleError and all bespoke JSON error emitters now go through emitJsonError(), which writes the envelope on stdout and mirrors a short red message on stderr only when stderr is a TTY. Consolidated ~15 ad-hoc `console.error(JSON.stringify({ error: … }))` call sites across batch/config/devices/expand/history/mcp/format into the new emitJsonError helper so the contract stays consistent. --- src/commands/batch.ts | 22 ++++----- src/commands/config.ts | 12 ++--- src/commands/devices.ts | 38 +++++++--------- src/commands/expand.ts | 8 ++-- src/commands/history.ts | 6 +-- src/commands/mcp.ts | 6 +-- src/utils/format.ts | 4 +- src/utils/output.ts | 31 ++++++++++++- tests/commands/devices.test.ts | 4 +- tests/commands/explain.test.ts | 7 +-- tests/commands/scenes.test.ts | 6 ++- tests/utils/output.test.ts | 83 +++++++++++++++++++++++++++++----- 12 files changed, 159 insertions(+), 68 deletions(-) diff --git a/src/commands/batch.ts b/src/commands/batch.ts index b9125802..59112f62 100644 --- a/src/commands/batch.ts +++ b/src/commands/batch.ts @@ -1,7 +1,7 @@ import { Command } from 'commander'; import type { AxiosInstance } from 'axios'; import { intArg, enumArg, stringArg } from '../utils/arg-parsers.js'; -import { printJson, isJsonMode, handleError, buildErrorPayload, UsageError, type ErrorPayload } from '../utils/output.js'; +import { printJson, isJsonMode, handleError, buildErrorPayload, UsageError, emitJsonError, type ErrorPayload } from '../utils/output.js'; import { fetchDeviceList, executeCommand, @@ -243,7 +243,7 @@ Examples: } catch (error) { if (error instanceof FilterSyntaxError) { if (isJsonMode()) { - console.error(JSON.stringify({ error: { code: 2, kind: 'usage', message: error.message } })); + emitJsonError({ code: 2, kind: 'usage', message: error.message }); } else { console.error(`Error: ${error.message}`); } @@ -251,7 +251,7 @@ Examples: } if (error instanceof Error && error.message.startsWith('No target devices')) { if (isJsonMode()) { - console.error(JSON.stringify({ error: { code: 2, kind: 'usage', message: error.message } })); + emitJsonError({ code: 2, kind: 'usage', message: error.message }); } else { console.error(`Error: ${error.message}`); } @@ -308,15 +308,13 @@ Examples: if (blockedForDestructive.length > 0 && !options.yes) { if (isJsonMode()) { const deviceIds = blockedForDestructive.map((b) => b.deviceId); - console.error(JSON.stringify({ - error: { - code: 2, - kind: 'guard', - message: `Destructive command "${cmd}" requires --yes to run on ${blockedForDestructive.length} device(s).`, - hint: 'Re-issue the call with --yes to proceed.', - context: { command: cmd, deviceIds }, - }, - })); + emitJsonError({ + code: 2, + kind: 'guard', + message: `Destructive command "${cmd}" requires --yes to run on ${blockedForDestructive.length} device(s).`, + hint: 'Re-issue the call with --yes to proceed.', + context: { command: cmd, deviceIds }, + }); } else { console.error( `Refusing to run destructive command "${cmd}" on ${blockedForDestructive.length} device(s) without --yes:` diff --git a/src/commands/config.ts b/src/commands/config.ts index 304228b3..c1188000 100644 --- a/src/commands/config.ts +++ b/src/commands/config.ts @@ -5,7 +5,7 @@ import { execFileSync } from 'node:child_process'; import { stringArg } from '../utils/arg-parsers.js'; import { intArg } from '../utils/arg-parsers.js'; import { saveConfig, showConfig, listProfiles, readProfileMeta } from '../config.js'; -import { isJsonMode, printJson } from '../utils/output.js'; +import { isJsonMode, printJson, emitJsonError } from '../utils/output.js'; import chalk from 'chalk'; function parseEnvFile(file: string): { token?: string; secret?: string } { @@ -164,7 +164,7 @@ Files are written with mode 0600. Profiles live under ~/.switchbot/profiles/ = { code: 2, kind: 'usage', message: err.message }; if (err.hint) obj.hint = err.hint; obj.context = { validationKind: err.kind }; - console.error(JSON.stringify({ error: obj })); + emitJsonError(obj); } else { console.error(`Error: ${err.message}`); if (err.hint) console.error(err.hint); @@ -512,14 +512,12 @@ Examples: const paramCheck = validateParameter(cachedForParam.type, cmd, parameter); if (!paramCheck.ok) { if (isJsonMode()) { - console.error(JSON.stringify({ - error: { - code: 2, - kind: 'usage', - message: paramCheck.error, - context: { command: cmd, deviceType: cachedForParam.type, deviceId }, - }, - })); + emitJsonError({ + code: 2, + kind: 'usage', + message: paramCheck.error, + context: { command: cmd, deviceType: cachedForParam.type, deviceId }, + }); } else { console.error(`Error: ${paramCheck.error}`); } @@ -537,17 +535,15 @@ Examples: const typeLabel = cachedForGuard?.type ?? 'unknown'; const reason = getDestructiveReason(cachedForGuard?.type, cmd, options.type); if (isJsonMode()) { - console.error(JSON.stringify({ - error: { - code: 2, - kind: 'guard', - message: `"${cmd}" on ${typeLabel} is destructive and requires --yes.`, - hint: reason - ? `Re-run with --yes to confirm. Reason: ${reason}` - : 'Re-run with --yes to confirm, or --dry-run to preview without sending.', - context: { command: cmd, deviceType: typeLabel, deviceId, ...(reason ? { destructiveReason: reason } : {}) }, - }, - })); + emitJsonError({ + code: 2, + kind: 'guard', + message: `"${cmd}" on ${typeLabel} is destructive and requires --yes.`, + hint: reason + ? `Re-run with --yes to confirm. Reason: ${reason}` + : 'Re-run with --yes to confirm, or --dry-run to preview without sending.', + context: { command: cmd, deviceType: typeLabel, deviceId, ...(reason ? { destructiveReason: reason } : {}) }, + }); } else { console.error( `Refusing to run destructive command "${cmd}" on ${typeLabel} without --yes.` diff --git a/src/commands/expand.ts b/src/commands/expand.ts index 8e26c878..3431ebbc 100644 --- a/src/commands/expand.ts +++ b/src/commands/expand.ts @@ -1,6 +1,6 @@ import { Command } from 'commander'; import { intArg, stringArg } from '../utils/arg-parsers.js'; -import { handleError, isJsonMode, printJson, UsageError } from '../utils/output.js'; +import { handleError, isJsonMode, printJson, UsageError, emitJsonError } from '../utils/output.js'; import { getCachedDevice } from '../devices/cache.js'; import { executeCommand, isDestructiveCommand, getDestructiveReason } from '../lib/devices.js'; import { isDryRun } from '../utils/flags.js'; @@ -115,10 +115,12 @@ Examples: if (!options.yes && !isDryRun() && isDestructiveCommand(deviceType, command, 'command')) { const reason = getDestructiveReason(deviceType, command, 'command'); if (isJsonMode()) { - console.error(JSON.stringify({ error: { code: 2, kind: 'guard', + emitJsonError({ + code: 2, + kind: 'guard', message: `"${command}" on ${deviceType || 'device'} is destructive and requires --yes.`, hint: reason ? `Re-run with --yes. Reason: ${reason}` : 'Re-run with --yes to confirm.', - }})); + }); } else { console.error(`Refusing to run destructive command "${command}" without --yes.`); if (reason) console.error(`Reason: ${reason}`); diff --git a/src/commands/history.ts b/src/commands/history.ts index 61b284c9..84a54f7c 100644 --- a/src/commands/history.ts +++ b/src/commands/history.ts @@ -2,7 +2,7 @@ import { Command } from 'commander'; import path from 'node:path'; import os from 'node:os'; import { intArg, stringArg } from '../utils/arg-parsers.js'; -import { printJson, isJsonMode, handleError, UsageError } from '../utils/output.js'; +import { printJson, isJsonMode, handleError, UsageError, emitJsonError } from '../utils/output.js'; import { readAudit, verifyAudit, type AuditEntry } from '../utils/audit.js'; import { executeCommand } from '../lib/devices.js'; import { @@ -88,7 +88,7 @@ Examples: if (!Number.isInteger(idx) || idx < 1 || idx > entries.length) { const msg = `Invalid index ${indexArg}. Log has ${entries.length} entries.`; if (isJsonMode()) { - console.error(JSON.stringify({ error: { code: 2, kind: 'usage', message: msg } })); + emitJsonError({ code: 2, kind: 'usage', message: msg }); } else { console.error(msg); } @@ -98,7 +98,7 @@ Examples: if (entry.kind !== 'command') { const msg = `Entry ${idx} is not a command (kind=${entry.kind}).`; if (isJsonMode()) { - console.error(JSON.stringify({ error: { code: 2, kind: 'usage', message: msg } })); + emitJsonError({ code: 2, kind: 'usage', message: msg }); } else { console.error(msg); } diff --git a/src/commands/mcp.ts b/src/commands/mcp.ts index 9e2b8c5e..41462597 100644 --- a/src/commands/mcp.ts +++ b/src/commands/mcp.ts @@ -4,7 +4,7 @@ import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; import { z } from 'zod'; import { intArg, stringArg } from '../utils/arg-parsers.js'; -import { handleError, isJsonMode, buildErrorPayload, type ErrorPayload, type ErrorSubKind } from '../utils/output.js'; +import { handleError, isJsonMode, buildErrorPayload, emitJsonError, type ErrorPayload, type ErrorSubKind } from '../utils/output.js'; import { VERSION } from '../version.js'; import { fetchDeviceList, @@ -851,7 +851,7 @@ Inspect locally: if (!Number.isFinite(port) || port < 1 || port > 65535) { const msg = `Invalid --port "${options.port}". Must be 1-65535.`; if (isJsonMode()) { - console.error(JSON.stringify({ error: { code: 2, kind: 'usage', message: msg } })); + emitJsonError({ code: 2, kind: 'usage', message: msg }); } else { console.error(msg); } @@ -868,7 +868,7 @@ Inspect locally: if (!isLocalhost && !authToken) { const msg = 'Refusing to listen on 0.0.0.0 without --auth-token. Pass --auth-token or bind to localhost (default).'; if (isJsonMode()) { - console.error(JSON.stringify({ error: { code: 2, kind: 'usage', message: msg } })); + emitJsonError({ code: 2, kind: 'usage', message: msg }); } else { console.error(msg); } diff --git a/src/utils/format.ts b/src/utils/format.ts index ff2e6e36..e44a731e 100644 --- a/src/utils/format.ts +++ b/src/utils/format.ts @@ -1,4 +1,4 @@ -import { printTable, printJson, isJsonMode, UsageError } from './output.js'; +import { printTable, printJson, isJsonMode, UsageError, emitJsonError } from './output.js'; import { getFormat, getFields } from './flags.js'; import { dump as yamlDump } from 'js-yaml'; @@ -18,7 +18,7 @@ export function parseFormat(flag: string | undefined): OutputFormat { default: { const msg = `Unknown --format "${flag}". Expected: table, json, jsonl, tsv, yaml, id, markdown.`; if (isJsonMode()) { - console.error(JSON.stringify({ error: { code: 2, kind: 'usage', message: msg } })); + emitJsonError({ code: 2, kind: 'usage', message: msg }); } else { console.error(msg); } diff --git a/src/utils/output.ts b/src/utils/output.ts index bc0aae6d..b4d0428a 100644 --- a/src/utils/output.ts +++ b/src/utils/output.ts @@ -14,6 +14,26 @@ export function printJson(data: unknown): void { console.log(JSON.stringify({ schemaVersion: SCHEMA_VERSION, data }, null, 2)); } +/** + * Emit a structured JSON error envelope on stdout. + * + * Bug #SYS-1: Under `--json`, both success and error payloads must share + * the same output channel (stdout) so a single `cli --json ... | jq` pipe + * can decode either shape. Use this helper everywhere that previously + * called `console.error(JSON.stringify({ error: ... }))` in --json mode. + * + * The envelope is always `{ schemaVersion, error }` — callers pass only the + * error payload. Also emits a brief human-readable line on stderr when a + * TTY is attached, so interactive runs still see the failure. + */ +export function emitJsonError(errorPayload: Record): void { + console.log(JSON.stringify({ schemaVersion: SCHEMA_VERSION, error: errorPayload })); + if (process.stderr.isTTY) { + const msg = typeof errorPayload.message === 'string' ? errorPayload.message : 'Error'; + console.error(chalk.red(msg)); + } +} + function escapeMarkdownCell(s: string): string { // Pipes break markdown table layout; backslash-escape them. Collapse // newlines into
so each row stays on one line. @@ -256,7 +276,16 @@ export function handleError(error: unknown): never { const payload = buildErrorPayload(error); if (isJsonMode()) { - console.error(JSON.stringify({ schemaVersion: SCHEMA_VERSION, error: payload })); + // Bug #SYS-1: Under --json, route the structured envelope to stdout so + // `cli --json ... | jq` pipelines can decode the error shape exactly + // the same way they decode success. Previously it went to stderr, which + // silently broke every error-path pipeline. TTY users still get a + // terse human-readable line on stderr so interactive runs don't look + // like the process simply exited. + console.log(JSON.stringify({ schemaVersion: SCHEMA_VERSION, error: payload })); + if (process.stderr.isTTY) { + console.error(chalk.red(payload.message)); + } process.exit(payload.code === 2 ? 2 : 1); } diff --git a/tests/commands/devices.test.ts b/tests/commands/devices.test.ts index 95c1fe74..156a988a 100644 --- a/tests/commands/devices.test.ts +++ b/tests/commands/devices.test.ts @@ -1860,7 +1860,9 @@ describe('devices command', () => { '--json', 'devices', 'command', LOCK_ID, 'unlock', ]); expect(res.exitCode).toBe(2); - const parsed = JSON.parse(res.stderr.join('\n')); + // Bug #SYS-1: --json errors now go to stdout so piped consumers can + // decode them the same way as success envelopes. + const parsed = JSON.parse(res.stdout.join('\n')); expect(parsed.error.kind).toBe('guard'); expect(parsed.error.code).toBe(2); expect(parsed.error.context.deviceId).toBe(LOCK_ID); diff --git a/tests/commands/explain.test.ts b/tests/commands/explain.test.ts index 3587d5bf..6f2796da 100644 --- a/tests/commands/explain.test.ts +++ b/tests/commands/explain.test.ts @@ -132,14 +132,15 @@ describe('devices explain', () => { expect(parsed.data.warnings).toHaveLength(0); }); - it('--json: device not found emits { error: { code:1, kind:"runtime" } } on stderr', async () => { + it('--json: device not found emits { error: { code:1, kind:"runtime" } } on stdout (bug #SYS-1)', async () => { devicesMock.describeDevice.mockRejectedValue(new devicesMock.DeviceNotFoundError('MISSING')); const res = await runExplain('--json', 'MISSING'); expect(res.exitCode).toBe(1); - expect(res.stdout).toHaveLength(0); - const parsed = JSON.parse(res.stderr[0]); + // Non-TTY: stderr stays clean so jq consumers aren't polluted. + expect(res.stderr).toHaveLength(0); + const parsed = JSON.parse(res.stdout[0]); expect(parsed.error.code).toBe(1); expect(parsed.error.kind).toBe('runtime'); expect(parsed.error.message).toContain('MISSING'); diff --git a/tests/commands/scenes.test.ts b/tests/commands/scenes.test.ts index f84848b7..193d6394 100644 --- a/tests/commands/scenes.test.ts +++ b/tests/commands/scenes.test.ts @@ -142,7 +142,8 @@ describe('scenes command', () => { const res = await runCli(registerScenesCommand, ['scenes', 'execute', 'BOGUS-ID', '--json']); expect(res.exitCode).toBe(2); expect(apiMock.__instance.post).not.toHaveBeenCalled(); - const out = res.stderr.join('\n'); + // Bug #SYS-1: --json errors now emit on stdout so piped consumers see them. + const out = res.stdout.join('\n'); const parsed = JSON.parse(out); expect(parsed.error?.context?.error).toBe('scene_not_found'); expect(parsed.error?.context?.sceneId).toBe('BOGUS-ID'); @@ -179,7 +180,8 @@ describe('scenes command', () => { }); const res = await runCli(registerScenesCommand, ['scenes', 'describe', 'MISSING', '--json']); expect(res.exitCode).toBe(2); - const out = res.stderr.join('\n'); + // Bug #SYS-1: --json errors now emit on stdout so piped consumers see them. + const out = res.stdout.join('\n'); const parsed = JSON.parse(out); expect(parsed.error?.context?.error).toBe('scene_not_found'); expect(parsed.error?.context?.sceneId).toBe('MISSING'); diff --git a/tests/utils/output.test.ts b/tests/utils/output.test.ts index b7b452cb..05b6a739 100644 --- a/tests/utils/output.test.ts +++ b/tests/utils/output.test.ts @@ -231,23 +231,29 @@ describe('handleError', () => { describe('--json mode', () => { let originalArgv: string[]; + let originalIsTTY: boolean | undefined; beforeEach(() => { originalArgv = process.argv; process.argv = ['node', 'cli', '--json', 'devices', 'status', 'X']; + originalIsTTY = process.stderr.isTTY; + // Force non-TTY so the human-readable stderr mirror is suppressed and + // the structured JSON envelope is the only output we assert on. + (process.stderr as { isTTY?: boolean }).isTTY = false; }); afterEach(() => { process.argv = originalArgv; + (process.stderr as { isTTY?: boolean }).isTTY = originalIsTTY; }); - it('outputs structured JSON error to stderr for ApiError', async () => { + it('outputs structured JSON error to stdout for ApiError (bug #SYS-1)', async () => { const { ApiError } = await import('../../src/api/client.js'); - const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('__exit'); }); expect(() => handleError(new ApiError('bad device', 190))).toThrow('__exit'); - const raw = errSpy.mock.calls[0][0]; + const raw = logSpy.mock.calls[0][0]; const parsed = JSON.parse(raw); expect(parsed.schemaVersion).toBe('1.1'); expect(parsed.error.code).toBe(190); @@ -257,51 +263,51 @@ describe('handleError', () => { it('marks 429 errors as retryable when ApiError.retryable is true', async () => { const { ApiError } = await import('../../src/api/client.js'); - const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('__exit'); }); // Simulate what client.ts creates: retryable: true set explicitly. expect(() => handleError(new ApiError('rate limited', 429, { retryable: true, hint: 'check quota' }))).toThrow('__exit'); - const parsed = JSON.parse(errSpy.mock.calls[0][0]); + const parsed = JSON.parse(logSpy.mock.calls[0][0]); expect(parsed.error.retryable).toBe(true); expect(parsed.error.hint).toBe('check quota'); }); it('prefers ApiError.hint over errorHint fallback when both exist', async () => { const { ApiError } = await import('../../src/api/client.js'); - const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('__exit'); }); // code 429 has an errorHint, but the explicit hint should win. expect(() => handleError(new ApiError('over limit', 429, { retryable: true, hint: 'custom hint from client' }))).toThrow('__exit'); - const parsed = JSON.parse(errSpy.mock.calls[0][0]); + const parsed = JSON.parse(logSpy.mock.calls[0][0]); expect(parsed.error.hint).toBe('custom hint from client'); }); it('does NOT set retryable when ApiError.retryable is false', async () => { const { ApiError } = await import('../../src/api/client.js'); - const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('__exit'); }); expect(() => handleError(new ApiError('auth failed', 401, { retryable: false }))).toThrow('__exit'); - const parsed = JSON.parse(errSpy.mock.calls[0][0]); + const parsed = JSON.parse(logSpy.mock.calls[0][0]); expect(parsed.error.retryable).toBeUndefined(); }); it('outputs structured JSON error for generic Error', () => { - const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('__exit'); }); expect(() => handleError(new Error('kaboom'))).toThrow('__exit'); - const parsed = JSON.parse(errSpy.mock.calls[0][0]); + const parsed = JSON.parse(logSpy.mock.calls[0][0]); expect(parsed.error.code).toBe(1); expect(parsed.error.message).toBe('kaboom'); }); @@ -364,3 +370,58 @@ describe('buildErrorPayload', () => { expect(p.hint).toMatch(/--type customize/); }); }); + +describe('handleError under --json (bug #SYS-1)', () => { + const originalArgv = process.argv; + const originalIsTTY = process.stderr.isTTY; + beforeEach(() => { + process.argv = ['node', 'test', '--json']; + }); + afterEach(() => { + process.argv = originalArgv; + (process.stderr as { isTTY?: boolean }).isTTY = originalIsTTY; + vi.restoreAllMocks(); + }); + + it('routes the structured error envelope to stdout, not stderr', () => { + (process.stderr as { isTTY?: boolean }).isTTY = false; + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(process, 'exit').mockImplementation((() => { + throw new Error('__exit'); + }) as never); + + expect(() => handleError(new Error('boom'))).toThrow('__exit'); + expect(logSpy).toHaveBeenCalledTimes(1); + const payload = JSON.parse(String(logSpy.mock.calls[0][0])); + expect(payload.schemaVersion).toBe(SCHEMA_VERSION); + expect(payload.error.message).toBe('boom'); + expect(payload.error.kind).toBe('runtime'); + // In non-TTY mode, stderr stays clean so `cli --json | jq` is unpolluted. + expect(errSpy).not.toHaveBeenCalled(); + }); + + it('emits a human-readable one-liner on stderr when stderr is a TTY', () => { + (process.stderr as { isTTY?: boolean }).isTTY = true; + vi.spyOn(console, 'log').mockImplementation(() => {}); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(process, 'exit').mockImplementation((() => { + throw new Error('__exit'); + }) as never); + + expect(() => handleError(new Error('ttys eek'))).toThrow('__exit'); + expect(errSpy).toHaveBeenCalled(); + expect(String(errSpy.mock.calls[0][0])).toContain('ttys eek'); + }); + + it('preserves exit code 2 for usage errors under --json', () => { + (process.stderr as { isTTY?: boolean }).isTTY = false; + vi.spyOn(console, 'log').mockImplementation(() => {}); + const exitSpy = vi.spyOn(process, 'exit').mockImplementation((() => { + throw new Error('__exit'); + }) as never); + + expect(() => handleError(new UsageError('bad input'))).toThrow('__exit'); + expect(exitSpy).toHaveBeenCalledWith(2); + }); +}); From 5ca21880d130556d120c40473ff53a4c1bb8a66d Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 22:31:30 +0800 Subject: [PATCH 31/34] fix(mcp): preflight deviceId in send_command dryRun (bug #SYS-3) send_command with dryRun:true now validates the deviceId against the local device cache before returning the wouldSend envelope. Fabricated IDs (e.g. 'DEADBEEF') now return a usage error with subKind 'device-not-found' instead of silently echoing back a plausible-looking preview. Dry-run is a validation surface; silently accepting arbitrary input defeated the point. Existing regression tests that exercise dryRun with specific IDs now seed the cache explicitly. --- src/commands/mcp.ts | 13 +++++++++++- tests/commands/dry-run.test.ts | 1 + tests/commands/mcp.test.ts | 36 ++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/commands/mcp.ts b/src/commands/mcp.ts index 41462597..241bd568 100644 --- a/src/commands/mcp.ts +++ b/src/commands/mcp.ts @@ -346,8 +346,19 @@ API docs: https://github.com/OpenWonderLabs/SwitchBotAPI`, async ({ deviceId, command, parameter, commandType, confirm, idempotencyKey, dryRun }) => { const effectiveType = commandType ?? 'command'; - // dryRun early-return — no API call, no validation against live device list + // dryRun early-return — no API call. We still preflight the deviceId + // against the local cache so fabricated IDs don't silently pass + // validation (bug #SYS-3). Dry-run is meant to catch bad inputs; a + // dry-run that accepts anything is worse than no dry-run at all. if (dryRun) { + const cached = getCachedDevice(deviceId); + if (!cached) { + return mcpError('usage', 2, `Device "${deviceId}" not found in local cache.`, { + subKind: 'device-not-found', + hint: "Run 'list_devices' first to warm the cache, then retry with dryRun:true.", + context: { deviceId }, + }); + } const wouldSend = { deviceId, command, diff --git a/tests/commands/dry-run.test.ts b/tests/commands/dry-run.test.ts index 03acbc0a..1feb8cad 100644 --- a/tests/commands/dry-run.test.ts +++ b/tests/commands/dry-run.test.ts @@ -105,6 +105,7 @@ describe('dryRun support on mutating tools', () => { }); it('send_command dryRun:true with parameter and commandType mirrors the full request shape', async () => { + cacheMock.map.set('IR1', { type: 'IR TV', name: 'Living Room', category: 'ir' }); const { client } = await pair(); const res = await client.callTool({ diff --git a/tests/commands/mcp.test.ts b/tests/commands/mcp.test.ts index bf14d701..2513a1c0 100644 --- a/tests/commands/mcp.test.ts +++ b/tests/commands/mcp.test.ts @@ -195,6 +195,42 @@ describe('mcp server', () => { expect(apiMock.__instance.post).toHaveBeenCalledTimes(1); }); + it('send_command dryRun rejects unknown deviceId against local cache (bug #SYS-3)', async () => { + // Cache is empty — no devices known. + const { client } = await pair(); + + const res = await client.callTool({ + name: 'send_command', + arguments: { deviceId: 'DEADBEEF', command: 'turnOff', dryRun: true }, + }); + + expect(res.isError).toBe(true); + const structured = res.structuredContent as { error?: { kind?: string; subKind?: string; context?: { deviceId?: string } } }; + expect(structured.error?.kind).toBe('usage'); + expect(structured.error?.subKind).toBe('device-not-found'); + expect(structured.error?.context?.deviceId).toBe('DEADBEEF'); + // Dry-run must not hit the network even for preflight. + expect(apiMock.__instance.post).not.toHaveBeenCalled(); + expect(apiMock.__instance.get).not.toHaveBeenCalled(); + }); + + it('send_command dryRun succeeds when deviceId is cached (bug #SYS-3 happy path)', async () => { + cacheMock.map.set('BULB1', { type: 'Color Bulb', name: 'Desk Lamp', category: 'physical' }); + const { client } = await pair(); + + const res = await client.callTool({ + name: 'send_command', + arguments: { deviceId: 'BULB1', command: 'turnOff', dryRun: true }, + }); + + expect(res.isError).toBeFalsy(); + const structured = res.structuredContent as { ok?: boolean; dryRun?: boolean; wouldSend?: { deviceId?: string; command?: string } }; + expect(structured.ok).toBe(true); + expect(structured.dryRun).toBe(true); + expect(structured.wouldSend?.deviceId).toBe('BULB1'); + expect(structured.wouldSend?.command).toBe('turnOff'); + }); + it('list_devices returns the raw API body and refreshes the cache', async () => { const body = { deviceList: [], infraredRemoteList: [] }; apiMock.__instance.get.mockResolvedValueOnce({ data: { statusCode: 100, body } }); From 55c5d33372d4c8f2fdc36074d197cd2f220e275e Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 22:35:44 +0800 Subject: [PATCH 32/34] docs(2.5.1): fold round-3 fixes into CHANGELOG + README MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGELOG: rework [2.5.1] section — expand scope statement from "Round-2 only" to "Round-2 + Round-3", add three new subsections covering the 10 extra commits (2 🔴 contract bugs SYS-1/SYS-3, 3 round-2 leftovers, 5 DX polish items), and revise "Not included" to list the three items actually deferred (parallel-status profiling, watch --json doc wording, meta import/export). README: document the three --filter operators (=, ~, =/regex/), --skip-offline and --idempotency-key alias under devices batch, --for on watch / events tail / events mqtt-tail, and a note that negative positional parameters (setBrightness -1) reach the validation layer. --- CHANGELOG.md | 101 ++++++++++++++++++++++++++++++++++++++++++++------- README.md | 28 +++++++++++++- 2 files changed, 114 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bdb1fc85..2f7389d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,16 @@ This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [2.5.1] - 2026-04-20 -Round-2 smoke-test response: 13 bugs closed (7 🔴 correctness / safety, 6 🟡 -UX). Source: `switchbot-cli-v2.5.0-round2-report.md`. Two items the report -flagged as bugs were found to be working as designed (false positives); two -were feature requests and deferred to 2.6.0 — see *Not included* below. +Round-2 + Round-3 smoke-test response: 24 bugs closed across three groups — +Round-2 correctness (13), Round-2 leftovers (3), and Round-3 contract & DX +(8). Sources: `switchbot-cli-v2.5.0-round2-report.md` and +`switchbot-cli-v2.5.0-round3-report.md`. + +The release was cut initially against the Round-2 report; the Round-3 report +arrived shortly after and is folded into the same patch so consumers of +2.5.1 get the full fix set in one version bump. The two Round-3 🔴 items +(`#SYS-1`, `#SYS-3`) are contract bugs that break agent pipelines and could +not wait. ### Fixed (correctness & safety) @@ -92,22 +98,89 @@ were feature requests and deferred to 2.6.0 — see *Not included* below. while only the append-only `.jsonl` was mentioned. `docs/agent-guide.md` now describes both files and `__control.jsonl`. (bug #43) -### Not included (response to report) +### Fixed (Round 3 contract bugs — 🔴) + +- **`--json` errors now emit on stdout instead of stderr** — piped + consumers (`cli --json ... | jq`) could not decode failure envelopes + because `handleError` wrote them to stderr. The JSON envelope + `{schemaVersion, error:{...}}` now lands on stdout for both success + and failure; TTY users still get a colored human-readable summary on + stderr, non-TTY invocations get silence on stderr. 15+ bespoke JSON + error sites across `batch`, `config`, `devices`, `expand`, `history`, + `mcp`, and `format` were consolidated through a new `emitJsonError` + helper. (bug #SYS-1) +- **MCP `send_command { dryRun:true }` validates deviceId against the + local cache** — dryRun previously accepted any string and echoed back + a plausible-looking preview, defeating the whole point of a + validation surface. Unknown IDs now return `subKind:'device-not-found'` + with a hint to run `list_devices` first. Happy path unchanged for + cached IDs. (bug #SYS-3) + +### Fixed (Round 2 leftovers) + +- **`devices batch --idempotency-key`** accepted as alias for + `--idempotency-key-prefix`. Still uses prefix semantics internally + (auto-appends `-` per step). (bug #30) +- **`--filter` DSL accepts three operators** — `key=value` (legacy + exact/substring), `key~value` (case-insensitive substring), and + `key=/pattern/` (case-insensitive regex; invalid regex returns a + usage error). (bug #39) + +### Added (Round 2/3 features) + +- **`devices batch --skip-offline`** (default off) skips devices whose + cached status is offline, with each skip recorded under + `summary.skipped` with `skippedReason:'offline'`. Reads the local + status cache only — no new API calls. Off by default preserves 2.5.0 + behavior. (bug #33) +- **`--for ` alias** on `devices watch`, `events tail`, and + `events mqtt-tail` — stops after elapsed time instead of tick/event + count. Accepts the same duration grammar as `--since` (ms/s/m/h/d/w). + When both `--for` and `--max` are set, the first limit to hit wins. + (bug #52) +- **Duration parser accepts `d` (days) and `w` (weeks)** in addition + to `ms/s/m/h`. Unsupported units like `1y` / `1month` now produce a + usage error that lists the supported unit set. (bug #54) +- **`events mqtt-tail --json` emits a `__session_start` envelope** + immediately on invocation (before the broker connect), so downstream + tools can distinguish "connecting" from "never connected" and get an + eventId to correlate with subsequent `__connect` / `__disconnect` + events. (bug #56) + +### Polish (Round 3 DX) + +- **`--name-strategy` help + `agent-bootstrap` list all six + strategies** — `exact`, `prefix`, `substring`, `fuzzy`, `first`, + `require-unique`. `ALL_STRATEGIES` in `name-resolver.ts` is the + single source of truth; help text is generated from it. (bug #51) +- **MCP `search_catalog` rejects empty queries** with a usage error + pointing to `list_catalog_types` for enumeration. Silent + "return everything" behavior was surprising and agent-hostile. + (bug #57) +- **Negative positional parameters reach the validation layer** — + `setBrightness -1` was being swallowed by Commander as "unknown + option `-1`". `devices command` now uses `.passThroughOptions()` so + negative numeric positionals are forwarded to the command-specific + validator, where they can be accepted or range-rejected as + appropriate. (bug #53) + +### Not included (response to reports) - **Report bug #19 (MCP strict schema not enforced) — false positive.** All 11 MCP tools already have `.strict()` on their Zod input schemas and the SDK enforces it via `safeParseAsync` → JSON-RPC `-32602`. Could not reproduce the reported behavior; the existing test suite exercises the full JSON-RPC path. -- **Report bug #30 (`devices batch --idempotency-key`) — working as - designed.** Batch intentionally uses `--idempotency-key-prefix` and - auto-appends `-` per step so each step has a distinct key. - A single `--idempotency-key` across a batch would cause only the first - step to execute and the rest to be replayed. -- **Deferred to 2.6.0:** `devices batch --skip-offline` (bug #33 — a - preflight status refresh + short-circuit; feature request, not a - correctness bug) and `--filter` DSL expansion with substring/regex - (bug #39 — current `key=value` exact match is documented behavior). +- **Deferred to 2.6.0:** + - Report bug #58 (parallel `devices status` outlier) — needs + profiling to separate CLI-side latency from API-side, and the fix + likely involves a concurrency knob rather than a single flip. + - Report bug #55 (`devices watch --json` rewording) — already works + via the global `--json` flag; pure doc rewording scheduled with + other doc sweeps. + - MCP / CLI naming alignment (`live` vs `includeStatus`, `metric` vs + `metrics`) flagged in Round-3 §4. + - `devices meta import/export` (Round-2 #40 follow-up). ## [2.5.0] - 2026-04-20 diff --git a/README.md b/README.md index 71bfc52e..1837c2c8 100644 --- a/README.md +++ b/README.md @@ -221,6 +221,12 @@ switchbot devices list --filter category=physical switchbot devices list --filter type=Bot switchbot devices list --filter name=living,category=physical +# Filter operators: = (legacy exact/substring), ~ (case-insensitive substring), +# =/regex/ (case-insensitive regex). Clauses are AND-ed. +switchbot devices list --filter 'name~living' +switchbot devices list --filter 'type=/Hub.*/' +switchbot devices list --filter 'name~office,type=/Bulb|Strip/' + # Filter by family / room (family & room info requires the 'src: OpenClaw' # header, which this CLI sends on every request) switchbot devices list --json | jq '.deviceList[] | select(.familyName == "Home")' @@ -280,6 +286,8 @@ Generic parameter shapes (which one applies is decided by the device — see the Parameters for `setAll` (Air Conditioner), `setPosition` (Curtain / Blind Tilt), and `setMode` (Relay Switch) are validated client-side before the request — malformed shapes, out-of-range values, and JSON for CSV fields all fail fast with exit 2. Command names are also case-normalized against the catalog (e.g. `turnon` is auto-corrected to `turnOn` with a stderr warning); unknown names still exit 2 with the supported-commands list. +Negative numeric parameters (e.g. `setBrightness -1` for a probe) are passed through to the command validator instead of being swallowed by the flag parser as an unknown option. + For the complete per-device command reference, see the [SwitchBot API docs](https://github.com/OpenWonderLabs/SwitchBotAPI#send-device-control-commands). #### `devices expand` — named flags for packed parameters @@ -343,10 +351,19 @@ switchbot devices list --format=id --filter 'type=Bot' | switchbot devices batch # Destructive commands require --yes switchbot devices batch unlock --filter 'type=Smart Lock' --yes + +# Skip devices whose cached status is offline (default: off) +switchbot devices batch turnOn --ids ID1,ID2 --skip-offline + +# --idempotency-key is an alias for --idempotency-key-prefix; both append - +switchbot devices batch turnOn --ids ID1,ID2 --idempotency-key morning-lights ``` Sends the same command to many devices in one run. Uses the same `--filter` expressions as `devices list`. Destructive commands (Smart Lock unlock, Garage Door Opener, etc.) require `--yes` to prevent accidents. +`--skip-offline` reads from the local status cache only (no new API calls); +skipped devices appear under `summary.skipped` with `skippedReason:'offline'`. + ### `scenes` — run manual scenes ```bash @@ -390,6 +407,9 @@ switchbot events tail --filter deviceId=ABC123 # Stop after 5 matching events switchbot events tail --filter 'type=WoMeter' --max 5 +# Stop after 10 minutes regardless of event count +switchbot events tail --for 10m + # Custom port / path switchbot events tail --port 8080 --path /hook --json ``` @@ -414,6 +434,9 @@ switchbot events mqtt-tail --topic 'switchbot/#' # Stop after 10 events switchbot events mqtt-tail --max 10 --json + +# Stop after a fixed duration (emits __session_start under --json before connect) +switchbot events mqtt-tail --for 30s --json ``` Connects to the SwitchBot MQTT service automatically using the same credentials configured for the REST API (`SWITCHBOT_TOKEN` + `SWITCHBOT_SECRET`). No additional MQTT configuration is required — the client certificates are provisioned on first use. @@ -514,9 +537,12 @@ switchbot devices watch # Custom interval; emit every tick even when nothing changed switchbot devices watch --interval 10s --include-unchanged --json + +# Time-bounded: stop after 5 minutes instead of a fixed tick count +switchbot devices watch --for 5m ``` -Output is a JSONL stream of status-change events (with `--json`) or a refreshed table. Use `--max ` to stop after N ticks. +Output is a JSONL stream of status-change events (with `--json`) or a refreshed table. Use `--max ` to stop after N ticks, or `--for ` to stop after an elapsed wall-clock window (e.g. `30s`, `1h`, `2d`). When both are set, whichever limit trips first wins. ### `mcp` — Model Context Protocol server From d557357cf5fe42c04e714aef9955d688e1eb2414 Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 23:26:59 +0800 Subject: [PATCH 33/34] fix(devices): route describe DeviceNotFoundError through emitJsonError (bug #SYS-1 followup) Canary testing on 2.5.1 uncovered a bespoke DeviceNotFoundError handler in `devices describe` that was writing plain text to stderr under --json, bypassing the emitJsonError helper added in bc473b0. Clients piping `devices describe --json | jq` saw an empty stdout and couldn't distinguish this from a silent failure. Route the error through emitJsonError when isJsonMode() so the schemaVersion envelope reaches stdout with exit 1; keep the human message on stderr for TTY users. Also ignore tmp/ in .gitignore so the canary harness stays out of the repo. --- .gitignore | 1 + src/commands/devices.ts | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index c8026823..87f911f5 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,4 @@ CLAUDE.md # Init transcript 2026-04-10-155920-command-messageinitcommand-message.txt +tmp/ diff --git a/src/commands/devices.ts b/src/commands/devices.ts index c060ca9e..f15c7e4c 100644 --- a/src/commands/devices.ts +++ b/src/commands/devices.ts @@ -827,8 +827,19 @@ Examples: } } catch (error) { if (error instanceof DeviceNotFoundError) { - console.error(error.message); - console.error(`Try 'switchbot devices list' to see the full list.`); + const message = `${error.message} Try 'switchbot devices list' to see the full list.`; + if (isJsonMode()) { + emitJsonError({ + code: 1, + kind: 'runtime', + message, + errorClass: 'runtime', + transient: false, + }); + } else { + console.error(error.message); + console.error(`Try 'switchbot devices list' to see the full list.`); + } process.exit(1); } handleError(error); From 279de64f4d7a9f3d20ea167d8b8f4a921de7fa1b Mon Sep 17 00:00:00 2001 From: chenliuyun Date: Mon, 20 Apr 2026 23:53:24 +0800 Subject: [PATCH 34/34] feat(filter)!: unify --filter DSL across list/batch/events (bug #39) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: three independent parsers with mismatched grammars: - devices list: =/~/=/regex/ (substring, category exact) - devices batch: = (exact) / ~= (substring) - events tail: = (exact) only Now all three share one grammar: `key=value` (case-insensitive substring; exact only for `category`), `key~value` (explicit case-insensitive substring), `key=/pattern/` (case-insensitive regex; invalid regex is a usage error). Clauses AND-ed. Each command still exposes its own key set. BREAKING CHANGE: `devices batch --filter 'type=Bot'` previously required an exact match and now treats `Bot` as a substring (matches `Bot Plus` too). `devices batch --filter 'type~=X'` (the `~=` spelling) is removed — use `~` instead. `events tail --filter 'deviceId=ABC'` is now a substring match (previously exact). See CHANGELOG §"Changed (BREAKING)" and README §"Filter expressions — per-command reference" for the full per-command table and migration. Implementation: - src/utils/filter.ts: add parseFilterExpr(expr, allowedKeys) + matchClause(candidate, clause, { exactKeys }); keep legacy parseFilter/applyFilter exports so src/commands/batch.ts needs no change. - src/commands/events.ts: switch to shared parser with EVENT_FILTER_KEYS=['deviceId','type']; FilterClause[]|null replaces the old ad-hoc {deviceId?,type?} shape. - src/commands/devices.ts (list): already on the new grammar since 2.5.1 bug #39 — unchanged. - Tests rewritten for new shape; added 'Bot Plus' fixture and a regex-alternation case to prove the substring switch. Verification: 959/959 vitest green; 516/516 canary green at 50 workers (48s); real-account smoke on list/batch/events tail for all three operators; `type~=X` rejected with a hint that points to `type~X`. --- CHANGELOG.md | 28 +++++- README.md | 23 ++++- src/commands/events.ts | 74 +++++++------- src/utils/filter.ts | 183 +++++++++++++++++++++++++--------- tests/commands/events.test.ts | 7 +- tests/utils/filter.test.ts | 152 ++++++++++++++++++++++++---- 6 files changed, 351 insertions(+), 116 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f7389d4..8a0b8e0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,30 @@ arrived shortly after and is folded into the same patch so consumers of (`#SYS-1`, `#SYS-3`) are contract bugs that break agent pipelines and could not wait. +This version also contains one **breaking change** — the `--filter` grammar +is now unified across `devices list`, `devices batch`, and +`events tail` / `mqtt-tail`. `devices batch` and `events tail` keys that +used to require exact matches are now substrings. See +**Changed (BREAKING)** below for the migration. + +### Changed (BREAKING) + +- **`--filter` grammar unified across three surfaces** — `devices list`, + `devices batch`, and `events tail` / `mqtt-tail` now share one DSL: + `key=value` (case-insensitive substring; exact only for `category`), + `key~value` (explicit case-insensitive substring), and + `key=/pattern/` (case-insensitive regex; invalid regex returns a usage + error). Each command still exposes its own key set — see README + §"Filter expressions — per-command reference". (bug #39) + - **Breaking**: `devices batch --filter 'type=Bot'` previously required + an exact match and now treats `Bot` as a substring (matches `Bot Plus` + too). Pair `=` with a more specific value, or filter post-hoc, if + exact match was load-bearing. + - **Breaking**: `devices batch --filter 'type~=...'` (the `~=` spelling) + is removed. Use `~` instead: `type~Light`. + - **Breaking**: `events tail --filter 'deviceId=ABC'` is now a substring + match (previously exact). + ### Fixed (correctness & safety) - **`devices command --dry-run --json` no longer emits empty stdout** — @@ -121,10 +145,6 @@ not wait. - **`devices batch --idempotency-key`** accepted as alias for `--idempotency-key-prefix`. Still uses prefix semantics internally (auto-appends `-` per step). (bug #30) -- **`--filter` DSL accepts three operators** — `key=value` (legacy - exact/substring), `key~value` (case-insensitive substring), and - `key=/pattern/` (case-insensitive regex; invalid regex returns a - usage error). (bug #39) ### Added (Round 2/3 features) diff --git a/README.md b/README.md index 1837c2c8..9afe076d 100644 --- a/README.md +++ b/README.md @@ -221,7 +221,7 @@ switchbot devices list --filter category=physical switchbot devices list --filter type=Bot switchbot devices list --filter name=living,category=physical -# Filter operators: = (legacy exact/substring), ~ (case-insensitive substring), +# Filter operators: = (substring; exact for `category`), ~ (substring), # =/regex/ (case-insensitive regex). Clauses are AND-ed. switchbot devices list --filter 'name~living' switchbot devices list --filter 'type=/Hub.*/' @@ -261,6 +261,21 @@ switchbot devices commands "Smart Lock" switchbot devices commands curtain # Case-insensitive, substring match ``` +#### Filter expressions — per-command reference + +Three commands accept `--filter`. They share one three-operator grammar, +but each exposes its own key set: + +| Command | Operators | Supported keys | +|-------------------------------------|-----------------------------------------------------------------------------------------------|---------------------------------------| +| `devices list` | `=` (substring; **exact** for `category`), `~` (substring), `=/regex/` (case-insensitive regex) | `type`, `name`, `category`, `room` | +| `devices batch` | same | `type`, `family`, `room`, `category` | +| `events tail` / `events mqtt-tail` | same (tail only; mqtt-tail uses `--topic` instead) | `deviceId`, `type` | + +Clauses are comma-separated and AND-ed. No OR across clauses — use regex +alternation (`=/A|B/`) for that. `category` is the one key that stays exact +under `=` to preserve `category=physical` / `category=ir` semantics. + #### Parameter formats `parameter` is optional — omit it for commands like `turnOn`/`turnOff` (auto-defaults to `"default"`). @@ -341,7 +356,7 @@ Stores local annotations (alias, hidden flag, notes) in `~/.switchbot/device-met ```bash # Send the same command to every device matching a filter switchbot devices batch turnOff --filter 'type=Bot' -switchbot devices batch setBrightness 50 --filter 'type~=Light,family=Living' +switchbot devices batch setBrightness 50 --filter 'type~Light,family=Living' # Explicit device IDs (comma-separated) switchbot devices batch turnOn --ids ID1,ID2,ID3 @@ -359,7 +374,7 @@ switchbot devices batch turnOn --ids ID1,ID2 --skip-offline switchbot devices batch turnOn --ids ID1,ID2 --idempotency-key morning-lights ``` -Sends the same command to many devices in one run. Uses the same `--filter` expressions as `devices list`. Destructive commands (Smart Lock unlock, Garage Door Opener, etc.) require `--yes` to prevent accidents. +Sends the same command to many devices in one run. Filter grammar matches `devices list` (`=` substring, `~` substring, `=/regex/` regex — clauses AND-ed); supported keys here are `type`, `family`, `room`, `category`. Destructive commands (Smart Lock unlock, Garage Door Opener, etc.) require `--yes` to prevent accidents. `--skip-offline` reads from the local status cache only (no new API calls); skipped devices appear under `summary.skipped` with `skippedReason:'offline'`. @@ -421,7 +436,7 @@ Output (one JSON line per matched event): { "t": "2024-01-01T12:00:00.000Z", "remote": "1.2.3.4:54321", "path": "/", "body": {...}, "matched": true } ``` -Filter keys: `deviceId=`, `type=` (comma-separated for AND logic). +Filter keys: `deviceId`, `type`. Operators: `=` (substring), `~` (substring), `=/regex/` (case-insensitive regex). Clauses comma-separated and AND-ed. #### `events mqtt-tail` — real-time MQTT stream diff --git a/src/commands/events.ts b/src/commands/events.ts index d0353e66..0e18c28d 100644 --- a/src/commands/events.ts +++ b/src/commands/events.ts @@ -4,6 +4,7 @@ import crypto from 'node:crypto'; import { printJson, isJsonMode, handleError, UsageError } from '../utils/output.js'; import { intArg, stringArg, durationArg } from '../utils/arg-parsers.js'; import { parseDurationToMs } from '../utils/flags.js'; +import { parseFilterExpr, matchClause, FilterSyntaxError, type FilterClause } from '../utils/filter.js'; import { SwitchBotMqttClient } from '../mqtt/client.js'; import { fetchMqttCredential } from '../mqtt/credential.js'; import { tryLoadConfig } from '../config.js'; @@ -41,54 +42,47 @@ interface EventRecord { function matchFilter( body: unknown, - filter: { deviceId?: string; type?: string } | null, + clauses: FilterClause[] | null, ): boolean { - if (!filter) return true; + if (!clauses || clauses.length === 0) return true; if (!body || typeof body !== 'object') return false; const b = body as Record; const ctx = (b.context ?? b) as Record; - if (filter.deviceId && ctx.deviceMac !== filter.deviceId && ctx.deviceId !== filter.deviceId) { - return false; - } - if (filter.type && ctx.deviceType !== filter.type) { - return false; + for (const c of clauses) { + let candidate: string; + if (c.key === 'deviceId') { + const mac = ctx.deviceMac; + const id = ctx.deviceId; + candidate = String( + typeof mac === 'string' && mac ? mac : typeof id === 'string' ? id : '', + ); + } else { + const t = ctx.deviceType; + candidate = typeof t === 'string' ? t : ''; + } + if (!matchClause(candidate, c)) return false; } return true; } -function parseFilter(flag: string | undefined): { deviceId?: string; type?: string } | null { +const EVENT_FILTER_KEYS = ['deviceId', 'type'] as const; + +function parseFilter(flag: string | undefined): FilterClause[] | null { if (!flag) return null; - const allowed = new Set(['deviceId', 'type']); - const out: { deviceId?: string; type?: string } = {}; - for (const pair of flag.split(',')) { - const eq = pair.indexOf('='); - if (eq === -1 || eq === 0) { - throw new UsageError( - `Invalid --filter pair "${pair.trim()}". Expected "key=value". Supported keys: deviceId, type.` - ); + try { + return parseFilterExpr(flag, EVENT_FILTER_KEYS); + } catch (e) { + if (e instanceof FilterSyntaxError) { + throw new UsageError(e.message); } - const k = pair.slice(0, eq).trim(); - const v = pair.slice(eq + 1).trim(); - if (!v) { - throw new UsageError( - `Empty value for --filter key "${k}". Expected "key=value". Supported keys: deviceId, type.` - ); - } - if (!allowed.has(k)) { - throw new UsageError( - `Unknown --filter key "${k}". Supported keys: deviceId, type.` - ); - } - if (k === 'deviceId') out.deviceId = v; - else if (k === 'type') out.type = v; + throw e; } - return out; } export function startReceiver( port: number, pathMatch: string, - filter: { deviceId?: string; type?: string } | null, + filter: FilterClause[] | null, onEvent: (ev: EventRecord) => void, ): http.Server { const server = http.createServer((req, res) => { @@ -155,7 +149,7 @@ export function registerEventsCommand(program: Command): void { .description('Run a local HTTP receiver and print incoming webhook events as JSONL') .option('--port ', `Local port to listen on (default ${DEFAULT_PORT})`, intArg('--port', { min: 1, max: 65535 }), String(DEFAULT_PORT)) .option('--path

', `HTTP path to match (default "${DEFAULT_PATH}"; use "*" for all paths)`, stringArg('--path'), DEFAULT_PATH) - .option('--filter ', 'Filter events, e.g. "deviceId=ABC123" or "type=Bot" (comma-separated)', stringArg('--filter')) + .option('--filter ', 'Filter events by deviceId / type. Grammar: "key=value" (substring), "key~value" (substring), "key=/regex/" (regex). Comma-separated clauses are AND-ed.', stringArg('--filter')) .option('--max ', 'Stop after N matching events (default: run until Ctrl-C)', intArg('--max', { min: 1 })) .option('--for ', 'Stop after elapsed time (e.g. "5m", "30s"). Combines with --max: first limit wins.', durationArg('--for')) .addHelpText( @@ -172,14 +166,20 @@ Output (JSONL, one event per line): { "t": "", "remote": "", "path": "/", "body": , "matched": true } -Filter grammar: comma-separated "key=value" pairs. Supported keys: - deviceId= match by context.deviceMac / context.deviceId - type= match by context.deviceType (e.g. "Bot", "WoMeter") +Filter grammar: comma-separated clauses (AND-ed). Each clause is one of + key=value — case-insensitive substring + key~value — explicit case-insensitive substring + key=/regex/ — case-insensitive regex + +Supported keys: + deviceId match by context.deviceMac / context.deviceId + type match by context.deviceType (e.g. "Bot", "WoMeter") Examples: $ switchbot events tail --port 3000 $ switchbot events tail --port 3000 --filter deviceId=ABC123 - $ switchbot events tail --filter 'type=WoMeter' --max 5 --json + $ switchbot events tail --filter 'type~Meter' --max 5 --json + $ switchbot events tail --filter 'type=/Bot|Meter/' `, ) .action(async (options: { port: string; path: string; filter?: string; max?: string; for?: string }) => { diff --git a/src/utils/filter.ts b/src/utils/filter.ts index b85755d4..f55e293c 100644 --- a/src/utils/filter.ts +++ b/src/utils/filter.ts @@ -2,12 +2,21 @@ import type { Device, InfraredDevice } from '../lib/devices.js'; /** * A parsed filter clause. Each clause is an (op, key, value) triple that runs - * against a candidate device. All clauses from a single expression are AND-ed. + * against a candidate string. All clauses from a single expression are AND-ed. + * + * Three operators (shared across `devices list`, `devices batch`, + * `events tail` / `mqtt-tail`): + * key=value — case-insensitive substring (exact for `category`) + * key~value — explicit case-insensitive substring + * key=/pattern/ — case-insensitive regex */ +export type FilterOp = 'eq' | 'sub' | 'regex'; + export interface FilterClause { - key: 'type' | 'family' | 'room' | 'category'; - op: '=' | '~='; - value: string; + key: string; + op: FilterOp; + raw: string; + regex?: RegExp; } export class FilterSyntaxError extends Error { @@ -17,49 +26,128 @@ export class FilterSyntaxError extends Error { } } -const VALID_KEYS: FilterClause['key'][] = ['type', 'family', 'room', 'category']; - /** - * Parse a filter expression like "type=Bot,family=Home" into discrete clauses. + * Parse a comma-separated filter expression into discrete clauses. * - * Grammar: - * expr := clause ("," clause)* - * clause := KEY OP VALUE - * KEY := type | family | room | category - * OP := "=" | "~=" - * VALUE := any non-empty string (no comma — split at the clause boundary) + * Grammar (per clause, recognition order): + * 1. key=/pattern/ → regex (case-insensitive); invalid regex throws. + * 2. key~value → substring (case-insensitive). + * 3. key=value → 'eq' op (substring; caller decides whether to treat + * as exact for specific keys via matchClause's + * `exactKeys` option). * - * Whitespace around keys / values is trimmed. Empty expressions return []. + * `allowedKeys` is command-specific: `devices list` uses + * {type,name,category,room}; `devices batch` uses {type,family,room,category}; + * `events tail` uses {deviceId,type}. */ -export function parseFilter(expr: string | undefined): FilterClause[] { +export function parseFilterExpr( + expr: string | undefined, + allowedKeys: readonly string[], +): FilterClause[] { if (!expr) return []; const parts = expr.split(',').map((p) => p.trim()).filter((p) => p.length > 0); const clauses: FilterClause[] = []; for (const part of parts) { - const m = /^([a-zA-Z_]+)\s*(~=|=)\s*(.+)$/.exec(part); - if (!m) { + const regexMatch = /^([^=~]+)=\/(.*)\/$/.exec(part); + const tildeIdx = part.indexOf('~'); + const eqIdx = part.indexOf('='); + + let key: string; + let op: FilterOp; + let raw: string; + let regex: RegExp | undefined; + + if (regexMatch) { + key = regexMatch[1].trim(); + op = 'regex'; + raw = regexMatch[2]; + try { + regex = new RegExp(raw, 'i'); + } catch (err) { + throw new FilterSyntaxError( + `Invalid regex in --filter "${part}": ${(err as Error).message}`, + ); + } + } else if (tildeIdx !== -1 && (eqIdx === -1 || tildeIdx < eqIdx)) { + key = part.slice(0, tildeIdx).trim(); + op = 'sub'; + raw = part.slice(tildeIdx + 1).trim(); + if (raw.startsWith('=')) { + throw new FilterSyntaxError( + `Invalid filter clause "${part}" — "~=" is no longer supported. Use "${key}~${raw.slice(1)}" instead.`, + ); + } + } else if (eqIdx !== -1) { + key = part.slice(0, eqIdx).trim(); + op = 'eq'; + raw = part.slice(eqIdx + 1).trim(); + } else { throw new FilterSyntaxError( - `Invalid filter clause "${part}" — expected "=" or "~="` + `Invalid filter clause "${part}" — expected "=", "~", or "=//"`, ); } - const key = m[1] as FilterClause['key']; - const op = m[2] as FilterClause['op']; - const value = m[3].trim(); - if (!VALID_KEYS.includes(key)) { - throw new FilterSyntaxError( - `Unknown filter key "${key}" — supported: ${VALID_KEYS.join(', ')}` - ); + + if (!key) { + throw new FilterSyntaxError(`Empty key in filter clause "${part}"`); } - if (!value) { + if (!raw) { throw new FilterSyntaxError(`Empty value for filter clause "${part}"`); } - clauses.push({ key, op, value }); + if (!allowedKeys.includes(key)) { + throw new FilterSyntaxError( + `Unknown filter key "${key}" — supported: ${allowedKeys.join(', ')}`, + ); + } + + clauses.push({ key, op, raw, regex }); } return clauses; } +/** + * Match a single candidate string against a clause. + * + * - `regex` → RegExp.test against the candidate (case-insensitive by construction). + * - `sub` → case-insensitive substring. + * - `eq` → case-insensitive substring, except for keys listed in + * `exactKeys`, which get case-insensitive exact comparison. + * Default `exactKeys` is `['category']` to preserve the existing + * list/batch behavior for that key. + */ +export function matchClause( + candidate: string | undefined, + clause: FilterClause, + options?: { exactKeys?: readonly string[] }, +): boolean { + if (candidate === undefined) return false; + if (clause.op === 'regex') { + return clause.regex!.test(candidate); + } + const cLower = candidate.toLowerCase(); + const vLower = clause.raw.toLowerCase(); + if (clause.op === 'sub') { + return cLower.includes(vLower); + } + const exactKeys = options?.exactKeys ?? ['category']; + if (exactKeys.includes(clause.key)) { + return cLower === vLower; + } + return cLower.includes(vLower); +} + +const BATCH_KEYS = ['type', 'family', 'room', 'category'] as const; + +/** + * Back-compat narrow signature: parses with the batch key set. Callers that + * need a different key set (list, events tail) should call parseFilterExpr + * directly. + */ +export function parseFilter(expr: string | undefined): FilterClause[] { + return parseFilterExpr(expr, BATCH_KEYS); +} + interface FilterableDevice { deviceId: string; type: string; @@ -72,7 +160,7 @@ interface FilterableDevice { function toFilterable( d: Device | InfraredDevice, isPhysical: boolean, - hubLocation?: Map + hubLocation?: Map, ): FilterableDevice { if (isPhysical) { const p = d as Device; @@ -95,35 +183,30 @@ function toFilterable( }; } -function matches(d: FilterableDevice, clause: FilterClause): boolean { - const candidate: string | undefined = - clause.key === 'type' - ? d.type - : clause.key === 'family' - ? d.family - : clause.key === 'room' - ? d.room - : d.category; - if (candidate === undefined) return false; - - if (clause.op === '=') return candidate.toLowerCase() === clause.value.toLowerCase(); - - // '~=' — case-insensitive substring match on the candidate. - return candidate.toLowerCase().includes(clause.value.toLowerCase()); +function candidateFor(d: FilterableDevice, key: string): string | undefined { + switch (key) { + case 'type': + return d.type; + case 'family': + return d.family; + case 'room': + return d.room; + case 'category': + return d.category; + default: + return undefined; + } } /** * Apply the parsed clauses to a mixed list of physical devices + IR remotes. - * Returns the deviceIds of the entries that satisfy every clause. - * - * `hubLocation` (optional) allows family/room filters to match IR remotes by - * the Hub-inherited location. + * Returns the filterable entries that satisfy every clause. */ export function applyFilter( clauses: FilterClause[], deviceList: Device[], infraredRemoteList: InfraredDevice[], - hubLocation?: Map + hubLocation?: Map, ): FilterableDevice[] { const candidates: FilterableDevice[] = [ ...deviceList.map((d) => toFilterable(d, true)), @@ -131,5 +214,7 @@ export function applyFilter( ]; if (clauses.length === 0) return candidates; - return candidates.filter((c) => clauses.every((clause) => matches(c, clause))); + return candidates.filter((c) => + clauses.every((clause) => matchClause(candidateFor(c, clause.key), clause)), + ); } diff --git a/tests/commands/events.test.ts b/tests/commands/events.test.ts index 0aabad23..cc29816d 100644 --- a/tests/commands/events.test.ts +++ b/tests/commands/events.test.ts @@ -6,6 +6,7 @@ import path from 'node:path'; import { once } from 'node:events'; import { AddressInfo } from 'node:net'; import { startReceiver, registerEventsCommand } from '../../src/commands/events.js'; +import type { FilterClause } from '../../src/utils/filter.js'; import { deviceHistoryStore } from '../../src/mcp/device-history.js'; import { runCli } from '../helpers/cli.js'; @@ -178,10 +179,11 @@ describe('events tail receiver', () => { it('marks events as unmatched when deviceId filter does not match', async () => { const port = await pickPort(); const received: Array<{ matched: boolean }> = []; + const filter: FilterClause[] = [{ key: 'deviceId', op: 'eq', raw: 'BOT1' }]; const server = startReceiver( port, '/', - { deviceId: 'BOT1' }, + filter, (ev) => received.push(ev as { matched: boolean }), ); await postJson(port, '/', { context: { deviceMac: 'BOT2', deviceType: 'Bot' } }); @@ -195,10 +197,11 @@ describe('events tail receiver', () => { it('type filter matches on context.deviceType', async () => { const port = await pickPort(); const received: Array<{ matched: boolean }> = []; + const filter: FilterClause[] = [{ key: 'type', op: 'eq', raw: 'WoMeter' }]; const server = startReceiver( port, '/', - { type: 'WoMeter' }, + filter, (ev) => received.push(ev as { matched: boolean }), ); await postJson(port, '/', { context: { deviceMac: 'X1', deviceType: 'Bot' } }); diff --git a/tests/utils/filter.test.ts b/tests/utils/filter.test.ts index 335d7fd9..8e379110 100644 --- a/tests/utils/filter.test.ts +++ b/tests/utils/filter.test.ts @@ -1,10 +1,18 @@ import { describe, it, expect } from 'vitest'; -import { parseFilter, applyFilter, FilterSyntaxError } from '../../src/utils/filter.js'; +import { + parseFilter, + parseFilterExpr, + matchClause, + applyFilter, + FilterSyntaxError, + type FilterClause, +} from '../../src/utils/filter.js'; import type { Device, InfraredDevice } from '../../src/lib/devices.js'; const devices: Device[] = [ { deviceId: 'BOT1', deviceName: 'Kitchen Bot', deviceType: 'Bot', familyName: 'Home', roomName: 'Kitchen', enableCloudService: true, hubDeviceId: 'HUB1' }, { deviceId: 'BOT2', deviceName: 'Office Bot', deviceType: 'Bot', familyName: 'Home', roomName: 'Office', enableCloudService: true, hubDeviceId: 'HUB1' }, + { deviceId: 'BOT3', deviceName: 'Garage Bot Plus', deviceType: 'Bot Plus', familyName: 'Home', roomName: 'Garage', enableCloudService: true, hubDeviceId: 'HUB1' }, { deviceId: 'LAMP', deviceName: 'Desk', deviceType: 'Color Bulb', familyName: 'Home', roomName: 'Office', enableCloudService: true, hubDeviceId: 'HUB1' }, { deviceId: 'METER', deviceName: 'Outside', deviceType: 'Meter', familyName: 'Cabin', roomName: 'Porch', enableCloudService: true, hubDeviceId: 'HUB2' }, ]; @@ -19,19 +27,41 @@ const hubLoc = new Map([ ['HUB2', { family: 'Cabin', room: 'Bedroom' }], ]); -describe('parseFilter', () => { +describe('parseFilter (batch-key default)', () => { it('returns [] for undefined / empty string', () => { expect(parseFilter(undefined)).toEqual([]); expect(parseFilter('')).toEqual([]); expect(parseFilter(' ')).toEqual([]); }); - it('parses a single exact clause', () => { - expect(parseFilter('type=Bot')).toEqual([{ key: 'type', op: '=', value: 'Bot' }]); + it('parses "key=value" as an eq clause (raw preserved)', () => { + expect(parseFilter('type=Bot')).toEqual([ + { key: 'type', op: 'eq', raw: 'Bot', regex: undefined }, + ]); }); - it('parses a substring (~=) clause', () => { - expect(parseFilter('type~=Light')).toEqual([{ key: 'type', op: '~=', value: 'Light' }]); + it('parses "key~value" as a sub clause', () => { + expect(parseFilter('type~Light')).toEqual([ + { key: 'type', op: 'sub', raw: 'Light', regex: undefined }, + ]); + }); + + it('parses "key=/pattern/" as a regex clause with case-insensitive RegExp', () => { + const [c] = parseFilter('type=/Bot.*/'); + expect(c.key).toBe('type'); + expect(c.op).toBe('regex'); + expect(c.raw).toBe('Bot.*'); + expect(c.regex?.source).toBe('Bot.*'); + expect(c.regex?.flags).toContain('i'); + }); + + it('rejects the legacy "~=" spelling with a helpful hint', () => { + expect(() => parseFilter('type~=Light')).toThrow(FilterSyntaxError); + expect(() => parseFilter('type~=Light')).toThrow(/~=.*no longer supported/); + }); + + it('rejects invalid regex with FilterSyntaxError', () => { + expect(() => parseFilter('type=/[/')).toThrow(FilterSyntaxError); }); it('parses multi-clause AND expressions', () => { @@ -43,7 +73,7 @@ describe('parseFilter', () => { it('trims whitespace around keys and values', () => { const [c] = parseFilter(' type = Bot Plus '); - expect(c).toEqual({ key: 'type', op: '=', value: 'Bot Plus' }); + expect(c).toEqual({ key: 'type', op: 'eq', raw: 'Bot Plus', regex: undefined }); }); it('rejects unknown keys', () => { @@ -59,26 +89,103 @@ describe('parseFilter', () => { }); }); +describe('parseFilterExpr with custom allowedKeys', () => { + it('accepts events-tail keys (deviceId, type)', () => { + const c = parseFilterExpr('deviceId=ABC,type~Bot', ['deviceId', 'type']); + expect(c).toHaveLength(2); + expect(c[0]).toEqual({ key: 'deviceId', op: 'eq', raw: 'ABC', regex: undefined }); + expect(c[1]).toEqual({ key: 'type', op: 'sub', raw: 'Bot', regex: undefined }); + }); + + it('rejects keys outside the allowed set', () => { + expect(() => parseFilterExpr('family=Home', ['deviceId', 'type'])).toThrow( + FilterSyntaxError, + ); + }); + + it('accepts list keys including "name"', () => { + const [c] = parseFilterExpr('name~office', ['type', 'name', 'category', 'room']); + expect(c).toEqual({ key: 'name', op: 'sub', raw: 'office', regex: undefined }); + }); +}); + +describe('matchClause', () => { + const sub = (key: string, raw: string): FilterClause => ({ key, op: 'sub', raw }); + const eq = (key: string, raw: string): FilterClause => ({ key, op: 'eq', raw }); + const rx = (key: string, src: string): FilterClause => ({ + key, + op: 'regex', + raw: src, + regex: new RegExp(src, 'i'), + }); + + it('sub is a case-insensitive substring match', () => { + expect(matchClause('Color Bulb', sub('type', 'color'))).toBe(true); + expect(matchClause('Color Bulb', sub('type', 'BULB'))).toBe(true); + expect(matchClause('Color Bulb', sub('type', 'neon'))).toBe(false); + }); + + it('eq is substring for non-exact keys', () => { + expect(matchClause('Bot Plus', eq('type', 'Bot'))).toBe(true); + expect(matchClause('Color Bulb', eq('type', 'neon'))).toBe(false); + }); + + it('eq is exact (case-insensitive) for "category" by default', () => { + expect(matchClause('physical', eq('category', 'physical'))).toBe(true); + expect(matchClause('physical', eq('category', 'phys'))).toBe(false); + expect(matchClause('IR', eq('category', 'ir'))).toBe(true); + }); + + it('regex.test against the raw candidate (not lowercased)', () => { + expect(matchClause('Bot Plus', rx('type', 'Bot.*'))).toBe(true); + expect(matchClause('Air Conditioner', rx('type', '^Air'))).toBe(true); + expect(matchClause('Air Conditioner', rx('type', 'conditioner'))).toBe(true); + expect(matchClause('TV', rx('type', 'conditioner'))).toBe(false); + }); + + it('undefined candidate never matches', () => { + expect(matchClause(undefined, sub('family', 'Home'))).toBe(false); + expect(matchClause(undefined, eq('category', 'physical'))).toBe(false); + }); + + it('custom exactKeys can make any key exact', () => { + expect( + matchClause('Bot Plus', eq('type', 'Bot'), { exactKeys: ['type'] }), + ).toBe(false); + expect( + matchClause('Bot', eq('type', 'Bot'), { exactKeys: ['type'] }), + ).toBe(true); + }); +}); + describe('applyFilter', () => { it('returns every candidate when the clause list is empty', () => { const all = applyFilter([], devices, irRemotes, hubLoc); expect(all.map((d) => d.deviceId).sort()).toEqual( - ['AC1', 'BOT1', 'BOT2', 'LAMP', 'METER', 'TV1'] + ['AC1', 'BOT1', 'BOT2', 'BOT3', 'LAMP', 'METER', 'TV1'], ); }); - it('filters by exact type on physical devices', () => { + it('type=Bot is now a substring match (was exact in <=2.5.0) — also hits Bot Plus', () => { const matched = applyFilter(parseFilter('type=Bot'), devices, irRemotes, hubLoc); - expect(matched.map((d) => d.deviceId).sort()).toEqual(['BOT1', 'BOT2']); + expect(matched.map((d) => d.deviceId).sort()).toEqual(['BOT1', 'BOT2', 'BOT3']); }); - it('substring match with ~= is case-insensitive', () => { - const matched = applyFilter(parseFilter('type~=light'), devices, irRemotes, hubLoc); - // "Color Bulb" doesn't contain "light", so only the IR remotes that do — none here. - // Let's check against a real substring. - const meter = applyFilter(parseFilter('type~=met'), devices, irRemotes, hubLoc); + it('substring match with ~ is case-insensitive', () => { + const meter = applyFilter(parseFilter('type~met'), devices, irRemotes, hubLoc); expect(meter.map((d) => d.deviceId)).toEqual(['METER']); - expect(matched).toEqual([]); // Color Bulb / Meter / Bot / TV / AC: none contain 'light' + const bulb = applyFilter(parseFilter('type~bulb'), devices, irRemotes, hubLoc); + expect(bulb.map((d) => d.deviceId)).toEqual(['LAMP']); + }); + + it('regex filter supports alternation', () => { + const matched = applyFilter( + parseFilter('type=/Bulb|Meter/'), + devices, + irRemotes, + hubLoc, + ); + expect(matched.map((d) => d.deviceId).sort()).toEqual(['LAMP', 'METER']); }); it('AND-joins multiple clauses', () => { @@ -86,7 +193,7 @@ describe('applyFilter', () => { parseFilter('type=Bot,room=Office'), devices, irRemotes, - hubLoc + hubLoc, ); expect(matched.map((d) => d.deviceId)).toEqual(['BOT2']); }); @@ -96,14 +203,19 @@ describe('applyFilter', () => { expect(matched.map((d) => d.deviceId).sort()).toEqual(['AC1', 'METER']); }); - it('filters by category=ir', () => { + it('filters by category=ir (exact, never substring)', () => { const matched = applyFilter(parseFilter('category=ir'), devices, irRemotes, hubLoc); expect(matched.map((d) => d.deviceId).sort()).toEqual(['AC1', 'TV1']); }); - it('filters by category=physical', () => { + it('filters by category=physical (exact)', () => { const matched = applyFilter(parseFilter('category=physical'), devices, irRemotes, hubLoc); - expect(matched.map((d) => d.deviceId).sort()).toEqual(['BOT1', 'BOT2', 'LAMP', 'METER']); + expect(matched.map((d) => d.deviceId).sort()).toEqual(['BOT1', 'BOT2', 'BOT3', 'LAMP', 'METER']); + }); + + it('category=phys (substring prefix) returns empty because category is exact', () => { + const matched = applyFilter(parseFilter('category=phys'), devices, irRemotes, hubLoc); + expect(matched).toEqual([]); }); it('returns empty when a clause has no matches', () => {