From 4dfe3464b2589aaa993997ad03ec2fd030606c9a Mon Sep 17 00:00:00 2001 From: Kaoru0822 Date: Tue, 20 Oct 2026 22:52:33 -0700 Subject: [PATCH] fix(signals): report rejected sigrok selectors as invalid input (#5570) sigrok-cli refusals of a caller-supplied decoder ID or annotation class were reported as execution_failure with "Run `rea doctor`" advice, which has no sigrok check. Recognize the observed sigrok-cli 0.7.2 refusals at the provider boundary only when the rejected name matches a caller selector, and report invalid_request naming the decoders or annotations argument, retaining the captured output and pointing to inspect_signal_decoders. Other non-zero exits stay provider failures. Fixes #5515 Co-authored-by: Claude Opus 5.5 --- src/signals/SigrokRecordingProvider.ts | 25 ++++++- src/signals/sigrokSelectorRejection.ts | 52 +++++++++++++++ .../signals/recordingProvider.test.ts | 66 +++++++++++++++++++ 3 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 src/signals/sigrokSelectorRejection.ts diff --git a/src/signals/SigrokRecordingProvider.ts b/src/signals/SigrokRecordingProvider.ts index 6b8918d576..05417d1af9 100644 --- a/src/signals/SigrokRecordingProvider.ts +++ b/src/signals/SigrokRecordingProvider.ts @@ -7,6 +7,10 @@ import { DecoderDiscoveryBudgetError, parseDecoderDiscovery, } from "./parseDecoderDiscovery.js"; +import { + parseSigrokSelectorRejection, + type SigrokSelectorRejection, +} from "./sigrokSelectorRejection.js"; import { BOUNDED_PRODUCER_LIMITS as LIMITS, prlimitArgumentVector, @@ -166,7 +170,12 @@ export class SigrokRecordingProvider implements SignalRecordingPort { root = await this.createRuntime(); this.#resources.retain(root, undefined); const snapshotPath = join(root.path, "recording"); - const execute = async (args: readonly string[]) => { + const execute = async ( + args: readonly string[], + rejectedSelector?: ( + stderr: string, + ) => SigrokSelectorRejection | undefined, + ) => { capturedOutput = undefined; const output = await runOwnedCommand( { @@ -222,6 +231,16 @@ export class SigrokRecordingProvider implements SignalRecordingPort { ); phase = previousPhase; resourcePath = "path" in input ? input.path : command; + const rejection = + output.exitCode === 0 + ? undefined + : rejectedSelector?.(output.stderr.text); + if (rejection !== undefined) + throw new AnalysisInputError( + operation, + { capturedOutput, userMessage: rejection.message }, + [rejection], + ); if (output.exitCode !== 0) throw new ProviderAdapterError( SIGROK_PROVIDER_IDENTITY.id, @@ -311,7 +330,9 @@ export class SigrokRecordingProvider implements SignalRecordingPort { ? [] : [`--protocol-decoder-annotations=${input.annotations}`]), ]; - const output = await execute(args); + const output = await execute(args, (stderr) => + parseSigrokSelectorRejection(stderr, input), + ); phase = "integrity"; resourcePath = snapshotPath; if ( diff --git a/src/signals/sigrokSelectorRejection.ts b/src/signals/sigrokSelectorRejection.ts new file mode 100644 index 0000000000..d37bfa10f4 --- /dev/null +++ b/src/signals/sigrokSelectorRejection.ts @@ -0,0 +1,52 @@ +import type { AnalysisInputIssue } from "../domain/analysisErrorCore.js"; +import type { DecodeSignalRecordingInput } from "../domain/signals/signalRecording.js"; + +export type SigrokSelectorRejection = AnalysisInputIssue & { + readonly message: string; +}; + +const rejectedSelector = ( + field: "decoders" | "annotations", + producerMessage: string, +): SigrokSelectorRejection => ({ + path: [field], + reason: "invalid_value", + message: `sigrok-cli rejected the ${field} selector: ${producerMessage} Use inspect_signal_decoders to list installed decoder IDs, or pass a decoder ID to show its options and annotation classes.`, +}); + +/** + * Recognize only observed sigrok-cli 0.7.2 selector refusals whose rejected + * name equals a caller-supplied selector; other failures stay unclassified. + */ +export const parseSigrokSelectorRejection = ( + stderr: string, + input: Pick, +): SigrokSelectorRejection | undefined => { + const lines = new Set(stderr.split("\n")); + // sigrok-cli stacks decoders with "," and appends options with ":". + for (const entry of input.decoders.split(",")) { + const id = entry.split(":")[0] ?? ""; + const producerMessage = `Failed to load protocol decoder ${id}.`; + // A missing module named exactly after the decoder separates an unknown ID + // from an installed decoder whose own imports or code fail to load. + if ( + id !== "" && + lines.has(producerMessage) && + lines.has( + `srd: ModuleNotFoundError: Failed to load decoder ${id}: import by name failed: No module named '${id}'`, + ) + ) + return rejectedSelector("decoders", producerMessage); + } + // Annotation selections are "decoder=class:class,decoder". + for (const entry of input.annotations?.split(",") ?? []) { + const [decoder = "", classes, ...rest] = entry.split("="); + if (decoder === "" || classes === undefined || rest.length > 0) continue; + for (const annotation of classes.split(":")) { + const producerMessage = `Annotation '${annotation}' not found for protocol decoder '${decoder}'.`; + if (annotation !== "" && lines.has(producerMessage)) + return rejectedSelector("annotations", producerMessage); + } + } + return undefined; +}; diff --git a/tests/boundary/signals/recordingProvider.test.ts b/tests/boundary/signals/recordingProvider.test.ts index 0a75aa5671..d044497f39 100644 --- a/tests/boundary/signals/recordingProvider.test.ts +++ b/tests/boundary/signals/recordingProvider.test.ts @@ -1,5 +1,6 @@ import { AnalysisResourceConstraintError } from "../../../src/domain/analysisErrorCore.js"; import { signalDecodersSchema } from "../../../src/domain/signals/signalDecoders.js"; +import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; import { OwnedCommandFailure } from "../../../src/process/OwnedCommand.js"; import { it, expect, onTestFinished } from "vitest"; import { writeFile, readdir, chmod, symlink } from "node:fs/promises"; @@ -28,6 +29,9 @@ const fixture = async (mode = "ok", failCleanup = false) => { if (process.argv.includes('--version')) { console.log(${JSON.stringify(mode)} === 'bad-version' ? 'sigrok-cli 0.7.1' : 'sigrok-cli 0.7.2\\nlibsigrokdecode 0.5.3'); console.error('version warning'); } else if (${JSON.stringify(mode)} === 'bad-version') console.log('unexpected version fallback'); else if (${JSON.stringify(mode)} === 'nonzero') { console.error('invalid decoder'); process.exitCode = 1; } +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)} === 'malformed-copy') { chmodSync('recording', 0o600); writeFileSync('recording', 'changed'); } else if (${JSON.stringify(mode)} === 'oversized-copy') { chmodSync('recording', 0o600); truncateSync('recording', 32 * 1024 * 1024 + 1); } else if (${JSON.stringify(mode)} === 'line-budget') process.stdout.write('ID: uart' + '\\n'.repeat(65536)); @@ -250,6 +254,68 @@ it.each([ }, ); +it.each([ + [ + "unknown-decoder", + { decoders: "uart:rx=RX,nosuchdecoder" }, + "decoders", + "Failed to load protocol decoder nosuchdecoder.", + ], + [ + "unknown-annotation", + { decoders: "uart", annotations: "uart=rx-data:nosuchclass" }, + "annotations", + "Annotation 'nosuchclass' not found for protocol decoder 'uart'.", + ], +] as const)( + "reports sigrok's %s refusal as invalid caller input", + async (mode, selection, field, refusal) => { + const f = await fixture(mode); + const result = await f.service.decode({ path: f.path, ...selection }); + if (result.ok) throw new Error("Expected selector refusal"); + expect(result.error._tag).toBe("AnalysisInputError"); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "invalid_request", + message: expect.stringContaining("inspect_signal_decoders"), + details: { + issues: [ + { + path: [field], + reason: "invalid_value", + message: expect.stringContaining(refusal), + }, + ], + captured_output: { + exit_code: 1, + stderr: expect.stringContaining(refusal), + }, + }, + }); + expect(result.error.cleanupIncomplete).toBe(false); + expect( + (await readdir(f.directory)).filter((name) => + name.startsWith("decoder-"), + ), + ).toEqual([]); + }, +); + +it.each([ + // An installed decoder whose own dependency is missing is an installation fault. + ["broken-decoder", { decoders: "uart" }], + // Refusals naming selectors the caller did not supply stay unclassified. + ["unknown-decoder", { decoders: "uart" }], + ["unknown-annotation", { decoders: "uart", annotations: "uart=rx-data" }], +] as const)( + "keeps %s outside caller selectors as a provider failure", + async (mode, selection) => { + const f = await fixture(mode); + const result = await f.service.decode({ path: f.path, ...selection }); + if (result.ok) throw new Error("Expected decoder failure"); + expect(result.error._tag).toBe("ProviderAdapterError"); + }, +); + it.each(["decode", "discovery"] as const)( "does not attribute the version probe's output to a %s command that fails to launch", async (operation) => {