[PR #5854] [CLOSED] fix(signals): classify a decoder help refusal as invalid input #7086

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5854
Author: @XiaoBinGan
Created: 10/21/2026
Status: ❌ Closed

Base: main ← Head: fix/sigrok-decoder-help-refusal


📝 Commits (2)

  • 2e09288 fix(signals): classify a decoder help refusal as invalid input
  • bae9b0d Merge remote-tracking branch 'gh/main' into fix/sigrok-decoder-help-refusal

📊 Changes

4 files changed (+171 additions, -11 deletions)

View changed files

📝 src/signals/SigrokRecordingProvider.ts (+4 -1)
📝 src/signals/sigrokSelectorRejection.ts (+43 -10)
📝 tests/boundary/signals/recordingProvider.test.ts (+39 -0)
➕ tests/boundary/signals/sigrokSelectorRejection.test.ts (+85 -0)

📄 Description

Summary

inspect_signal_decoders launches sigrok-cli --show for the requested decoder but never passed a rejection parser to execute, so sigrok's unknown-ID refusal surfaced as a provider execution failure with rea doctor guidance instead of an invalid_request naming the decoder field.

Fixes #5849

Problem and expected behavior

  • Smallest trigger: inspect_signal_decoders with {"decoder": "nosuchdecoder"} against an installation whose producer refuses that ID. The producer exits 1 with Failed to load protocol decoder nosuchdecoder. and the matching srd: ModuleNotFoundError line, and the operation returned a provider execution failure.
  • The same refusal is already classified on decode_signal_recording, where parseSigrokSelectorRejection turns it into an input issue on decoders and points at decoder discovery. Only the help path was missing the parser.
  • Violated invariant: a refusal that names exactly the selector the caller supplied is an input problem, not a tooling fault; docs/tool-design.md requires the input issue to identify the rejected field.
  • Expected: the help refusal for the requested decoder becomes invalid_request with issues[0].path = ["decoder"], keeps the producer's stderr as captured output, and points at catalog discovery. Refusals for other IDs, catalog requests, and installed decoders whose own imports fail stay provider failures.

Change and scope

  • src/signals/sigrokSelectorRejection.ts: the missing-decoder recognition is factored into missingDecoderRefusal and reused; a new parseSigrokDecoderHelpRejection(stderr, decoder) classifies the refusal only when it names the decoder the caller asked help for.
  • src/signals/SigrokRecordingProvider.ts: the inspect_signal_decoders branch passes that parser to execute; the decode path, catalog listing and version probe are untouched.
  • Tests: tests/boundary/signals/sigrokSelectorRejection.test.ts (new, parser cases) and two cases in tests/boundary/signals/recordingProvider.test.ts (help refusal becomes AnalysisInputError; a refusal for another decoder stays a provider failure).
  • Not included: no change to the classifier's existing rule that refusals naming selectors the caller did not supply stay unclassified; no change to producer invocation, limits or workspace ownership.

Contract and boundary impact

  • Semantic owner and earliest changed stage: sigrok selector-refusal classification at the producer boundary.
  • Contribution scope fit and linked maintainer decision: not applicable - an ordinary fix inside an existing boundary.
  • CLI and MCP/tool-catalog contract: inspect_signal_decoders now returns invalid_request with a decoder issue path instead of an execution failure for an unknown decoder ID.
  • Provider, bridge, target-format, or platform compatibility: Linux sigrok provider only; no producer argument or protocol change.
  • Evidence, artifact, provenance, or reconstruction contract: the producer's stderr stays in captured_output; the failure classification is the only change.
  • Target selection, authority, upstream behavior, and cleanup details: unchanged; no extra process is launched and workspace cleanup keeps its existing path.
  • Generated metadata, package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).

  • 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.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/signals/sigrokSelectorRejection.test.ts (new, 8 cases), tests/boundary/signals/recordingProvider.test.ts (+2 cases).
  • Base reproduction or other evidence: with the parser wiring reverted, the new help case fails with expected 'ProviderAdapterError' to be 'AnalysisInputError'; with the change it returns code: "invalid_request", details.issues[0].path = ["decoder"], and captured_output.exit_code = 1 retaining the producer stderr. The decode-path cases and their existing "unclassified" expectations are unchanged.
  • User-visible CLI/MCP output (if applicable): an unknown decoder ID passed to inspect_signal_decoders now reports the decoder field and decoder discovery instead of directing the caller to rea doctor.
  • Remaining proof gaps: the producer is an authored simulator emitting sigrok 0.7.2's observed refusal lines, not a real sigrok installation; no real installation was exercised.

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

  • npx vitest run tests/boundary/signals/sigrokSelectorRejection.test.ts tests/boundary/signals/recordingProvider.test.ts - 51 passed.
  • npx vitest run tests/boundary/signals tests/boundary/mcp/signalRecordingMcp.test.ts - 58 passed, 9 skipped.
  • npm run format:check - all matched files use the correct format.
  • npm run verify:test-discovery - all 1904 test files discovered exactly once.
  • npm run typecheck - no diagnostic in the changed files (the remaining errors in this checkout are pre-existing protobuf-module resolution failures).
  • npm run lint - 0 errors, module boundaries checked.

Compatibility, safety, and release

  • Breaking changes or migration steps: a client that treated an unknown decoder ID on the help path as a tooling failure now receives invalid_request; the decode path is unchanged.
  • Real sigrok coverage: none claimed.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none; no producer argument, limit or containment change.

Author readiness

Human owner: @XiaoBinGan

  • 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.

🔄 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/5854 **Author:** [@XiaoBinGan](https://github.com/XiaoBinGan) **Created:** 10/21/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/sigrok-decoder-help-refusal` --- ### 📝 Commits (2) - [`2e09288`](https://github.com/morluto/rea/commit/2e09288cf9d2cccc83f2691de867478f84934e82) fix(signals): classify a decoder help refusal as invalid input - [`bae9b0d`](https://github.com/morluto/rea/commit/bae9b0def62511c4ce787750b25e304358de5f95) Merge remote-tracking branch 'gh/main' into fix/sigrok-decoder-help-refusal ### 📊 Changes **4 files changed** (+171 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `src/signals/SigrokRecordingProvider.ts` (+4 -1) 📝 `src/signals/sigrokSelectorRejection.ts` (+43 -10) 📝 `tests/boundary/signals/recordingProvider.test.ts` (+39 -0) ➕ `tests/boundary/signals/sigrokSelectorRejection.test.ts` (+85 -0) </details> ### 📄 Description ## Summary `inspect_signal_decoders` launches `sigrok-cli --show` for the requested decoder but never passed a rejection parser to `execute`, so sigrok's unknown-ID refusal surfaced as a provider execution failure with `rea doctor` guidance instead of an `invalid_request` naming the `decoder` field. Fixes #5849 ## Problem and expected behavior - Smallest trigger: `inspect_signal_decoders` with `{"decoder": "nosuchdecoder"}` against an installation whose producer refuses that ID. The producer exits 1 with `Failed to load protocol decoder nosuchdecoder.` and the matching `srd: ModuleNotFoundError` line, and the operation returned a provider execution failure. - The same refusal is already classified on `decode_signal_recording`, where `parseSigrokSelectorRejection` turns it into an input issue on `decoders` and points at decoder discovery. Only the help path was missing the parser. - Violated invariant: a refusal that names exactly the selector the caller supplied is an input problem, not a tooling fault; `docs/tool-design.md` requires the input issue to identify the rejected field. - Expected: the help refusal for the requested decoder becomes `invalid_request` with `issues[0].path = ["decoder"]`, keeps the producer's stderr as captured output, and points at catalog discovery. Refusals for other IDs, catalog requests, and installed decoders whose own imports fail stay provider failures. ## Change and scope - `src/signals/sigrokSelectorRejection.ts`: the missing-decoder recognition is factored into `missingDecoderRefusal` and reused; a new `parseSigrokDecoderHelpRejection(stderr, decoder)` classifies the refusal only when it names the decoder the caller asked help for. - `src/signals/SigrokRecordingProvider.ts`: the `inspect_signal_decoders` branch passes that parser to `execute`; the decode path, catalog listing and version probe are untouched. - Tests: `tests/boundary/signals/sigrokSelectorRejection.test.ts` (new, parser cases) and two cases in `tests/boundary/signals/recordingProvider.test.ts` (help refusal becomes `AnalysisInputError`; a refusal for another decoder stays a provider failure). - Not included: no change to the classifier's existing rule that refusals naming selectors the caller did not supply stay unclassified; no change to producer invocation, limits or workspace ownership. ## Contract and boundary impact - Semantic owner and earliest changed stage: sigrok selector-refusal classification at the producer boundary. - Contribution scope fit and linked maintainer decision: not applicable - an ordinary fix inside an existing boundary. - CLI and MCP/tool-catalog contract: `inspect_signal_decoders` now returns `invalid_request` with a `decoder` issue path instead of an execution failure for an unknown decoder ID. - Provider, bridge, target-format, or platform compatibility: Linux sigrok provider only; no producer argument or protocol change. - Evidence, artifact, provenance, or reconstruction contract: the producer's stderr stays in `captured_output`; the failure classification is the only change. - Target selection, authority, upstream behavior, and cleanup details: unchanged; no extra process is launched and workspace cleanup keeps its existing path. - Generated metadata, package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). - [ ] 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. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/signals/sigrokSelectorRejection.test.ts` (new, 8 cases), `tests/boundary/signals/recordingProvider.test.ts` (+2 cases). - Base reproduction or other evidence: with the parser wiring reverted, the new help case fails with `expected 'ProviderAdapterError' to be 'AnalysisInputError'`; with the change it returns `code: "invalid_request"`, `details.issues[0].path = ["decoder"]`, and `captured_output.exit_code = 1` retaining the producer stderr. The decode-path cases and their existing "unclassified" expectations are unchanged. - User-visible CLI/MCP output (if applicable): an unknown decoder ID passed to `inspect_signal_decoders` now reports the `decoder` field and decoder discovery instead of directing the caller to `rea doctor`. - Remaining proof gaps: the producer is an authored simulator emitting sigrok 0.7.2's observed refusal lines, not a real sigrok installation; no real installation was exercised. 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 - `npx vitest run tests/boundary/signals/sigrokSelectorRejection.test.ts tests/boundary/signals/recordingProvider.test.ts` - 51 passed. - `npx vitest run tests/boundary/signals tests/boundary/mcp/signalRecordingMcp.test.ts` - 58 passed, 9 skipped. - `npm run format:check` - all matched files use the correct format. - `npm run verify:test-discovery` - all 1904 test files discovered exactly once. - `npm run typecheck` - no diagnostic in the changed files (the remaining errors in this checkout are pre-existing protobuf-module resolution failures). - `npm run lint` - 0 errors, module boundaries checked. ## Compatibility, safety, and release - Breaking changes or migration steps: a client that treated an unknown decoder ID on the help path as a tooling failure now receives `invalid_request`; the decode path is unchanged. - Real sigrok coverage: none claimed. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; no producer argument, limit or containment change. ## Author readiness Human owner: @XiaoBinGan - [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 - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:45:17 +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#7086
No description provided.