[PR #5570] [MERGED] fix(signals): report rejected sigrok selectors as invalid input #6969

Closed
opened 2026-11-01 21:44:23 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5570
Author: @kaoru0822-kitauji
Created: 10/20/2026
Status: ✅ Merged
Merged: 10/21/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/sigrok-selector-errors


📝 Commits (2)

  • 34ef008 fix(signals): report rejected sigrok selectors as invalid input
  • 748a996 Merge remote-tracking branch 'origin/main' into fix/sigrok-selector-errors

📊 Changes

3 files changed (+141 additions, -2 deletions)

View changed files

📝 src/signals/SigrokRecordingProvider.ts (+23 -2)
➕ src/signals/sigrokSelectorRejection.ts (+52 -0)
📝 tests/boundary/signals/recordingProvider.test.ts (+66 -0)

📄 Description

What broke: a misspelled sigrok decoder ID or annotation class came back as execution_failure with "run rea doctor" advice (#5515).
Fix: the sigrok adapter recognizes the two pinned sigrok-cli 0.7.2 refusals, only when they name a caller-supplied selector, and reports invalid_request on that argument.
Risk: low; unmatched stderr keeps failing closed as execution_failure. Not verified against a real sigrok-cli.

Summary

Fixes #5515. When sigrok-cli rejects a decoder ID or annotation class that the caller supplied, decode_signal_recording now returns invalid_request instead of execution_failure. Before this change, the error told callers to "Run rea doctor", but doctor has no sigrok check. The new error names the decoders or annotations argument, keeps sigrok's stderr and points to inspect_signal_decoders.

Problem and expected behavior

SigrokRecordingProvider turned every non-zero sigrok-cli exit into a generic ProviderAdapterError. A typo such as --decoders nosuchdecoder or --annotations uart=nosuchclass therefore produced execution_failure with doctor advice. The fix is to correct the selector, but the error never said so. Expected behavior: a rejected caller selector is reported as an input error on the field that caused it. Failures that come from the installation stay execution_failure.

Change and scope

  • New src/signals/sigrokSelectorRejection.ts recognizes only the two sigrok-cli 0.7.2 refusal formats quoted in the issue, and only when the rejected name equals a selector the caller supplied:
    • Failed to load protocol decoder <id>. is matched only together with the libsigrokdecode line srd: ModuleNotFoundError: Failed to load decoder <id>: import by name failed: No module named '<id>'. This separates an unknown decoder ID from an installed decoder that fails because one of its own dependencies is missing, which is an installation problem.
    • Annotation '<class>' not found for protocol decoder '<decoder>'. is matched only when that decoder=class pair appears in the caller's annotations.
  • SigrokRecordingProvider runs this check only for the decode command, and only on a non-zero exit after the existing integrity checks. A match raises AnalysisInputError with one issue (path: ["decoders"|"annotations"], reason: "invalid_value"), a user message pointing to inspect_signal_decoders, and the retained capturedOutput (stdout, stderr, exit code). Every other non-zero exit still raises ProviderAdapterError.
  • Not included: refusals of decoder options or channels, Protocol decoder '<x>' not found. for annotations naming a decoder that isn't in the stack, and a sigrok readiness check in rea doctor. I couldn't observe those message formats against a real sigrok-cli, so they keep failing closed as execution_failure.
  • Scope decision: not applicable. This is an error-classification fix within an existing provider boundary.

Contract and boundary impact

  • Semantic owner and earliest changed stage: the sigrok provider adapter (src/signals/), where sigrok's stderr is interpreted.
  • Contribution scope fit and linked maintainer decision: not applicable (ordinary fix).
  • CLI and MCP/tool-catalog contract: no schema change. For the matched refusals only, the error code changes from execution_failure to invalid_request, with details.issues and details.captured_output.
  • Provider, bridge, target-format, or platform compatibility: matching is pinned to the sigrok-cli 0.7.2 strings already required by the version gate. Messages that don't match keep the previous behavior.
  • Evidence, artifact, provenance, or reconstruction contract: unchanged. The full producer stderr is kept in captured_output.
  • Target selection, authority, upstream behavior, and cleanup details: unchanged. The tests confirm the private workspace is still released.
  • Generated metadata, package, or installation impact: none.

Expected effects:

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

(The existing decode path already reads the selected recording and launches sigrok-cli. This PR doesn't change either.)

Evidence and regression coverage

  • Tests added or updated, in tests/boundary/signals/recordingProvider.test.ts (command-boundary simulator with real process ownership, not real sigrok):
    • reports sigrok's unknown-decoder / unknown-annotation refusal as invalid caller input: the projected error is invalid_request, the message mentions inspect_signal_decoders, details.issues[0].path is ["decoders"] or ["annotations"], captured_output keeps exit code 1 and the sigrok stderr, and the private workspace is released.
    • keeps <mode> outside caller selectors as a provider failure: these cases stay ProviderAdapterError. They cover an installed decoder with a missing dependency module, and refusals that name a selector the caller did not supply.
  • Base reproduction: none run locally, because sigrok-cli is not installed on the macOS host (which sigrok-cli finds nothing). The refusal strings come from the reporter's observed sigrok-cli 0.7.2 output in #5515. Reasoning from the source: on the base revision the two new "invalid caller input" tests would fail, because every non-zero exit became ProviderAdapterError.
  • User-visible output, derived from the test projection:
    {"code":"invalid_request","message":"sigrok-cli rejected the decoders selector: Failed to load protocol decoder nosuchdecoder. Use inspect_signal_decoders to list installed decoder IDs, or pass a decoder ID to show its options and annotation classes.", "details":{"issues":[{"path":["decoders"],"reason":"invalid_value",...}],"captured_output":{...,"exit_code":1}}}
  • Remaining proof gaps: not verified against a real sigrok-cli 0.7.2 on Linux. Decoder option and channel refusals and the doctor readiness check are not covered.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npm run test:focused -- tests/boundary/signals/recordingProvider.test.ts: 41 passed.
  • npm run test:focused -- tests/boundary/signals/recordingDescriptorCleanup.test.ts tests/boundary/signals/resourceLimits.test.ts tests/boundary/mcp/signalRecordingMcp.test.ts: 4 passed, 9 skipped (the skipped tests are Linux-only on this macOS host).
  • npm run check:fast (typecheck, lint): passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Only the error code for the matched selector refusals changes.
  • Real Hopper/Ghidra, browser, or OS coverage: no real sigrok coverage, macOS host only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new process, file or network behavior. The stderr retained in the error was already retained before this change.

Author readiness

Human owner: @kaoru0822-kitauji

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code


🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/5570 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/20/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/sigrok-selector-errors` --- ### 📝 Commits (2) - [`34ef008`](https://github.com/morluto/rea/commit/34ef008a27fb9be6101bf898d72327e958103563) fix(signals): report rejected sigrok selectors as invalid input - [`748a996`](https://github.com/morluto/rea/commit/748a996123e00918f30f9f2a8826173818349ca7) Merge remote-tracking branch 'origin/main' into fix/sigrok-selector-errors ### 📊 Changes **3 files changed** (+141 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/signals/SigrokRecordingProvider.ts` (+23 -2) ➕ `src/signals/sigrokSelectorRejection.ts` (+52 -0) 📝 `tests/boundary/signals/recordingProvider.test.ts` (+66 -0) </details> ### 📄 Description **What broke:** a misspelled sigrok decoder ID or annotation class came back as `execution_failure` with "run rea doctor" advice (#5515). **Fix:** the sigrok adapter recognizes the two pinned sigrok-cli 0.7.2 refusals, only when they name a caller-supplied selector, and reports `invalid_request` on that argument. **Risk:** low; unmatched stderr keeps failing closed as `execution_failure`. Not verified against a real sigrok-cli. ## Summary Fixes #5515. When sigrok-cli rejects a decoder ID or annotation class that the caller supplied, `decode_signal_recording` now returns `invalid_request` instead of `execution_failure`. Before this change, the error told callers to "Run `rea doctor`", but doctor has no sigrok check. The new error names the `decoders` or `annotations` argument, keeps sigrok's stderr and points to `inspect_signal_decoders`. ## Problem and expected behavior `SigrokRecordingProvider` turned every non-zero sigrok-cli exit into a generic `ProviderAdapterError`. A typo such as `--decoders nosuchdecoder` or `--annotations uart=nosuchclass` therefore produced `execution_failure` with doctor advice. The fix is to correct the selector, but the error never said so. Expected behavior: a rejected caller selector is reported as an input error on the field that caused it. Failures that come from the installation stay `execution_failure`. ## Change and scope - New `src/signals/sigrokSelectorRejection.ts` recognizes only the two sigrok-cli 0.7.2 refusal formats quoted in the issue, and only when the rejected name equals a selector the caller supplied: - `Failed to load protocol decoder <id>.` is matched only together with the libsigrokdecode line `srd: ModuleNotFoundError: Failed to load decoder <id>: import by name failed: No module named '<id>'`. This separates an unknown decoder ID from an installed decoder that fails because one of its own dependencies is missing, which is an installation problem. - `Annotation '<class>' not found for protocol decoder '<decoder>'.` is matched only when that `decoder=class` pair appears in the caller's `annotations`. - `SigrokRecordingProvider` runs this check only for the decode command, and only on a non-zero exit after the existing integrity checks. A match raises `AnalysisInputError` with one issue (`path: ["decoders"|"annotations"]`, `reason: "invalid_value"`), a user message pointing to `inspect_signal_decoders`, and the retained `capturedOutput` (stdout, stderr, exit code). Every other non-zero exit still raises `ProviderAdapterError`. - Not included: refusals of decoder options or channels, `Protocol decoder '<x>' not found.` for annotations naming a decoder that isn't in the stack, and a sigrok readiness check in `rea doctor`. I couldn't observe those message formats against a real sigrok-cli, so they keep failing closed as `execution_failure`. - Scope decision: not applicable. This is an error-classification fix within an existing provider boundary. ## Contract and boundary impact - Semantic owner and earliest changed stage: the sigrok provider adapter (`src/signals/`), where sigrok's stderr is interpreted. - Contribution scope fit and linked maintainer decision: not applicable (ordinary fix). - CLI and MCP/tool-catalog contract: no schema change. For the matched refusals only, the error code changes from `execution_failure` to `invalid_request`, with `details.issues` and `details.captured_output`. - Provider, bridge, target-format, or platform compatibility: matching is pinned to the sigrok-cli 0.7.2 strings already required by the version gate. Messages that don't match keep the previous behavior. - Evidence, artifact, provenance, or reconstruction contract: unchanged. The full producer stderr is kept in `captured_output`. - Target selection, authority, upstream behavior, and cleanup details: unchanged. The tests confirm the private workspace is still released. - Generated metadata, package, or installation impact: none. Expected effects: - [x] Read existing files or captured data. - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. (The existing decode path already reads the selected recording and launches sigrok-cli. This PR doesn't change either.) ## Evidence and regression coverage - Tests added or updated, in `tests/boundary/signals/recordingProvider.test.ts` (command-boundary simulator with real process ownership, not real sigrok): - `reports sigrok's unknown-decoder / unknown-annotation refusal as invalid caller input`: the projected error is `invalid_request`, the message mentions `inspect_signal_decoders`, `details.issues[0].path` is `["decoders"]` or `["annotations"]`, `captured_output` keeps exit code 1 and the sigrok stderr, and the private workspace is released. - `keeps <mode> outside caller selectors as a provider failure`: these cases stay `ProviderAdapterError`. They cover an installed decoder with a missing dependency module, and refusals that name a selector the caller did not supply. - Base reproduction: none run locally, because sigrok-cli is not installed on the macOS host (`which sigrok-cli` finds nothing). The refusal strings come from the reporter's observed sigrok-cli 0.7.2 output in #5515. Reasoning from the source: on the base revision the two new "invalid caller input" tests would fail, because every non-zero exit became `ProviderAdapterError`. - User-visible output, derived from the test projection: `{"code":"invalid_request","message":"sigrok-cli rejected the decoders selector: Failed to load protocol decoder nosuchdecoder. Use inspect_signal_decoders to list installed decoder IDs, or pass a decoder ID to show its options and annotation classes.", "details":{"issues":[{"path":["decoders"],"reason":"invalid_value",...}],"captured_output":{...,"exit_code":1}}}` - Remaining proof gaps: not verified against a real sigrok-cli 0.7.2 on Linux. Decoder option and channel refusals and the doctor readiness check are not covered. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:focused -- tests/boundary/signals/recordingProvider.test.ts`: 41 passed. - `npm run test:focused -- tests/boundary/signals/recordingDescriptorCleanup.test.ts tests/boundary/signals/resourceLimits.test.ts tests/boundary/mcp/signalRecordingMcp.test.ts`: 4 passed, 9 skipped (the skipped tests are Linux-only on this macOS host). - `npm run check:fast` (typecheck, lint): passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only the error code for the matched selector refusals changes. - Real Hopper/Ghidra, browser, or OS coverage: no real sigrok coverage, macOS host only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new process, file or network behavior. The stderr retained in the error was already retained before this change. ## Author readiness Human owner: @kaoru0822-kitauji - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:44:23 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#6969
No description provided.