From 48cd33af186fd50111503ed6aff2296436286bc6 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:40:48 +0800 Subject: [PATCH] fix(signals): reject missing sigrok channels Recognize the exact selected-channel diagnostic even when sigrok-cli exits successfully, while preserving partial producer output in the invalid-input details. Cover both public JSON adapters and add the missing-channel case to the Linux real-tool verifier. Fixes #6631 --- docs/signal-recordings.md | 7 +- scripts/verify-signal-recording.mjs | 30 +++++++- src/signals/SigrokRecordingProvider.ts | 4 +- src/signals/sigrokSelectorRejection.ts | 32 ++++++++ .../signals/recordingProvider.test.ts | 75 +++++++++++++++++++ 5 files changed, 144 insertions(+), 4 deletions(-) diff --git a/docs/signal-recordings.md b/docs/signal-recordings.md index 9fd45b7385..89c6384ae0 100644 --- a/docs/signal-recordings.md +++ b/docs/signal-recordings.md @@ -31,6 +31,10 @@ The corresponding MCP request is: `decoders` uses sigrok's decoder stack and option/channel syntax. Channel names must match the recording; REA does not guess wiring, baud rate or protocol. +When sigrok reports that a selected channel name is missing, REA returns +`invalid_request` for `decoders` even if sigrok continues with fallback +annotations and exits successfully. The diagnostic and any partial stdout remain +in `captured_output`; unrelated producer warnings do not change request status. Omitting `input_format` requests sigrok's autodetection. REA uses an immutable private copy whose filename retains the selected recording's final suffix, so filename-based format detection receives that hint without exposing the @@ -95,7 +99,8 @@ REA_SIGROK_COMMAND=/absolute/path/to/sigrok-cli node scripts/verify-signal-recor The verifier synthesizes a 9600-baud 8N1 UART VCD carrying `REA`, then checks actual CLI/MCP parity, advertised schemas, stored Evidence, source hashes, -annotation filtering/autodetection, invalid decoder/input failures and cleanup. +annotation filtering/autodetection, invalid decoder/input failures, missing +channel rejection with retained partial output, and cleanup. An optional positional REA entrypoint verifies an installed package. The real-tool lane requires Linux, sigrok-cli 0.7.2 and libsigrokdecode. A separate kernel-limit regression checks the inherited soft/hard limits and verifies an diff --git a/scripts/verify-signal-recording.mjs b/scripts/verify-signal-recording.mjs index e093d0aa14..7559109504 100644 --- a/scripts/verify-signal-recording.mjs +++ b/scripts/verify-signal-recording.mjs @@ -40,7 +40,12 @@ const environment = { }; const client = new Client({ name: "signal-recording-verifier", version: "1" }); // Real sigrok refusals of an unknown decoder ID must name the caller's field. -const assertRejectedSelector = (response, operation, field) => { +const assertRejectedSelector = ( + response, + operation, + field, + expectedExitCode = 1, +) => { assert.equal(response.isError, true); const text = response.content.find((item) => item.type === "text")?.text; const { error } = JSON.parse(text ?? "{}"); @@ -50,7 +55,8 @@ const assertRejectedSelector = (response, operation, field) => { error.details.issues.map((issue) => issue.path), [[field]], ); - assert.equal(error.details.captured_output.exit_code, 1); + assert.equal(error.details.captured_output.exit_code, expectedExitCode); + return error; }; const transport = new StdioClientTransport({ command: process.execPath, @@ -174,6 +180,26 @@ try { assert.equal(await readFile(path, "utf8"), source); cases++; } + const missingChannel = await client.callTool({ + name: advertised.name, + arguments: { + path, + decoders: "uart:rx=REA_NO_SUCH_CHANNEL:baudrate=9600", + input_format: "vcd", + }, + }); + const channelError = assertRejectedSelector( + missingChannel, + advertised.name, + "decoders", + 0, + ); + assert.match( + channelError.details.captured_output.stderr, + /cli: No channel with name "REA_NO_SUCH_CHANNEL" found\./u, + ); + assert.match(channelError.details.captured_output.stdout, /uart-1:/u); + cases++; const pagedLines = lines.slice(0, 7); let pagedTick = 1000; for (const byte of Array.from({ length: 120 }, (_, i) => i)) { diff --git a/src/signals/SigrokRecordingProvider.ts b/src/signals/SigrokRecordingProvider.ts index b5e978b273..08d3cd6e1f 100644 --- a/src/signals/SigrokRecordingProvider.ts +++ b/src/signals/SigrokRecordingProvider.ts @@ -242,8 +242,10 @@ export class SigrokRecordingProvider implements SignalRecordingPort { ); phase = previousPhase; resourcePath = "path" in input ? input.path : command; + // Some selected-channel refusals are diagnostics, not process failures: + // sigrok can emit fallback annotations and still exit successfully. const rejection = - output.exitCode === 0 || selection === undefined + selection === undefined ? undefined : parseSigrokSelectorRejection(output.stderr.text, selection); if (rejection !== undefined) diff --git a/src/signals/sigrokSelectorRejection.ts b/src/signals/sigrokSelectorRejection.ts index 42d21840c4..3caa64aeb4 100644 --- a/src/signals/sigrokSelectorRejection.ts +++ b/src/signals/sigrokSelectorRejection.ts @@ -25,6 +25,8 @@ const DECODE_RECOVERY = "Use inspect_signal_decoders to list installed decoder IDs, or pass a decoder ID to show its options and annotation classes."; const HELP_RECOVERY = "Omit decoder to list installed decoder IDs, optionally filtered by query, then request help for one of those IDs."; +const CHANNEL_RECOVERY = + "Choose a channel present in the recording; inspect_signal_decoders shows the selected decoder's channel IDs."; /** * sigrok-cli loads selected decoders before decoding or showing help, so an @@ -46,6 +48,30 @@ const unknownDecoderRefusal = ( : undefined; }; +/** + * The sigrok-cli 0.7.2 channel mapper reports a missing target as a plain + * stderr line, even though it continues with the other channel mappings. It + * does not include the decoder channel key, so compare the reported target + * with exact values from the caller's assignments rather than searching the + * selector text for a substring. + */ +const missingSelectedChannelRefusal = ( + lines: ReadonlySet, + decoders: string, +): string | undefined => { + const selectedValues = new Set(); + for (const decoder of decoders.split(",")) + for (const assignment of decoder.split(":").slice(1)) { + const separator = assignment.indexOf("="); + if (separator > 0) selectedValues.add(assignment.slice(separator + 1)); + } + for (const channel of selectedValues) { + const producerMessage = `No channel with name "${channel}" found.`; + if (lines.has(`cli: ${producerMessage}`)) return producerMessage; + } + return undefined; +}; + /** * Recognize only observed sigrok-cli 0.7.2 selector refusals whose rejected * name equals a caller-supplied selector; other failures stay unclassified. @@ -70,6 +96,12 @@ export const parseSigrokSelectorRejection = ( if (refusal !== undefined) return rejectedSelector("decoders", refusal, DECODE_RECOVERY); } + const missingChannel = missingSelectedChannelRefusal( + lines, + selection.decoders, + ); + if (missingChannel !== undefined) + return rejectedSelector("decoders", missingChannel, CHANNEL_RECOVERY); // Annotation selections are "decoder=class:class,decoder". for (const entry of selection.annotations?.split(",") ?? []) { const [decoder = "", classes, ...rest] = entry.split("="); diff --git a/tests/boundary/signals/recordingProvider.test.ts b/tests/boundary/signals/recordingProvider.test.ts index b94dfc34fe..69ecbf33fb 100644 --- a/tests/boundary/signals/recordingProvider.test.ts +++ b/tests/boundary/signals/recordingProvider.test.ts @@ -45,6 +45,8 @@ else if (${JSON.stringify(mode)} === 'nonzero') { console.error('invalid decoder else if (${JSON.stringify(mode)} === 'unknown-decoder') { console.error("srd: ModuleNotFoundError: Failed to load decoder nosuchdecoder: import by name failed: No module named 'nosuchdecoder'\\nFailed to load protocol decoder nosuchdecoder."); process.exitCode = 1; } else if (${JSON.stringify(mode)} === 'broken-decoder') { console.error("srd: ModuleNotFoundError: Failed to load decoder uart: import by name failed: No module named 'serial'\\nFailed to load protocol decoder uart."); process.exitCode = 1; } else if (${JSON.stringify(mode)} === 'unknown-annotation') { console.error("Annotation 'nosuchclass' not found for protocol decoder 'uart'."); process.exitCode = 1; } +else if (${JSON.stringify(mode)} === 'unknown-channel') { process.stdout.write('1000-1105 uart-1: Start bit\\n'); console.error('cli: No channel with name "NONEXISTENT" found.'); } +else if (${JSON.stringify(mode)} === 'other-channel-warning') { process.stdout.write('1000-1105 uart-1: Start bit\\n'); console.error('cli: No channel with name "OTHER" found.'); } else if (${JSON.stringify(mode)} === 'malformed-copy') { chmodSync(${JSON.stringify(snapshotName)}, 0o600); writeFileSync(${JSON.stringify(snapshotName)}, 'changed'); } else if (${JSON.stringify(mode)} === 'oversized-copy') { chmodSync(${JSON.stringify(snapshotName)}, 0o600); truncateSync(${JSON.stringify(snapshotName)}, 32 * 1024 * 1024 + 1); } else if (${JSON.stringify(mode)} === 'line-budget') process.stdout.write('ID: uart' + '\\n'.repeat(65536)); @@ -334,6 +336,79 @@ it.each([ }, ); +it("projects sigrok's zero-exit missing-channel refusal through CLI and MCP with partial output", async () => { + const f = await fixture("unknown-channel"); + const refusal = 'No channel with name "NONEXISTENT" found.'; + const message = `sigrok-cli rejected the decoders selector: ${refusal}`; + + const harness = await createApplicationMcpHarness({ + serverOptions: { environment: {}, signalRecording: f.service }, + }); + onTestFinished(harness.close); + const { error } = parseMcpToolError( + await harness.client.callTool({ + name: "decode_signal_recording", + arguments: { + path: f.path, + decoders: "uart:rx=NONEXISTENT:baudrate=9600", + }, + }), + ); + expect(error).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + message: expect.stringContaining(message), + details: { + issues: [{ path: ["decoders"], reason: "invalid_value" }], + captured_output: { + exit_code: 0, + stdout: "1000-1105 uart-1: Start bit\n", + stderr: expect.stringContaining(refusal), + }, + }, + }); + + const cli = Cli.create("signal-channel-refusal-test", { sync: false }); + registerSignalCommands(cli, silentLogger, {}, f.service); + let stdout = ""; + await cli.serve( + [ + "decode-signal-recording", + f.path, + "--decoders", + "uart:rx=NONEXISTENT:baudrate=9600", + "--format", + "json", + ], + { + stdout: (text) => { + stdout += text; + }, + exit: () => {}, + }, + ); + expect(JSON.parse(stdout)).toMatchObject({ + code: "invalid_request", + details: { + issues: [{ path: ["decoders"], reason: "invalid_value" }], + captured_output: { + exit_code: 0, + stdout: "1000-1105 uart-1: Start bit\n", + stderr: expect.stringContaining(refusal), + }, + }, + }); +}); + +it("does not match channel refusals by substring", async () => { + const f = await fixture("other-channel-warning"); + const result = await f.service.decode({ + path: f.path, + decoders: "uart:rx=RX:baudrate=9600", + }); + expect(result.ok).toBe(true); +}); + it("reports an unknown decoder ID as the rejected field of each MCP operation", async () => { const f = await fixture("unknown-decoder"); const harness = await createApplicationMcpHarness({