[PR #5858] [MERGED] fix(signals): report unknown decoder help IDs as invalid input #7089

New issue

Closed

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

bsakaguchi commented

2026-11-01 21:45:17 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/signal-decoder-unknown-id

📝 Commits (2)

fe643f2 fix(signals): report unknown decoder help IDs as invalid input
6c4434e Merge remote-tracking branch 'origin/main' into fix/signal-decoder-unknown-id

📊 Changes

5 files changed (+179 additions, -35 deletions)

View changed files

📝 docs/signal-recordings.md (+3 -0)
📝 scripts/verify-signal-recording.mjs (+20 -5)
📝 src/signals/SigrokRecordingProvider.ts (+7 -10)
📝 src/signals/sigrokSelectorRejection.ts (+54 -18)
📝 tests/boundary/signals/recordingProvider.test.ts (+95 -2)

📄 Description

What broke: inspect_signal_decoders with an unknown decoder reported sigrok's explicit missing-decoder refusal as execution_failure with rea doctor advice, while decode_signal_recording already reported the same refusal as invalid_request.
Fix: the sigrok selector classifier now owns the unknown-ID match for both operations; help names decoder and points to catalog discovery, decoding keeps decoders/annotations.
Risk: low; only the exact two-line refusal naming the caller's ID changes class. Other non-zero exits stay execution_failure. Tests use a sigrok simulator; the real-sigrok CI lane now asserts the typed outcome.

Summary

Fixes #5849. Asking for help on a decoder ID that is not installed now returns invalid_request with an issue on ["decoder"]. The message tells the caller to omit decoder to list installed IDs. sigrok's output is kept in captured_output. The response now matches the one decode_signal_recording gives for the same refusal.

Problem and expected behavior

Discovery and decoding share one command executor in SigrokRecordingProvider, but only decoding passed the selector-refusal classifier (added in #5570). The --show help command therefore fell through to ProviderAdapterError → execution_failure + doctor advice, with no field-level correction. Expected: when sigrok refuses a caller-selected decoder ID that is absent, the response is an input error on the field that selected it. An installed decoder whose own dependency is missing remains a provider failure.

Change and scope

src/signals/sigrokSelectorRejection.ts is the one owner of this interpretation. It takes either selection shape ({decoders, annotations} for decoding, {decoder} for help). A single private matcher recognizes the unknown-ID refusal for both. Each field gets its own recovery text.
SigrokRecordingProvider.execute now takes the caller selection that a command loads, instead of a per-call classifier closure. Both producer commands pass their input. The --version probe passes nothing. The superseded closure parameter is removed.
The matched strings are unchanged: Failed to load protocol decoder <id>. plus srd: ModuleNotFoundError: Failed to load decoder <id>: import by name failed: No module named '<id>'. They match only when <id> equals the caller's selection.
Sibling check: these are the only two signal operations. The catalog listing (decoder: null) selects no ID, so it is never classified.
Not included: option/channel refusals and unknown input formats. Their real sigrok wording has not been observed, so they keep failing closed as execution_failure.
Scope decision: not applicable (error classification within an existing provider boundary).

Contract and boundary impact

Semantic owner and earliest changed stage: sigrok provider adapter (src/signals/sigrokSelectorRejection.ts), where producer 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 help refusal 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: pinned to sigrok-cli 0.7.2 strings already used for decoding; unmatched output keeps prior behavior.
Evidence, artifact, provenance, or reconstruction contract: unchanged; full stderr retained.
Target selection, authority, upstream behavior, and cleanup details: unchanged; tests confirm the private workspace is released.
Generated metadata, package, or installation impact: none. docs/signal-recordings.md gains three sentences.

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.

(Existing discovery already launches sigrok-cli. This PR does not change that.)

Evidence and regression coverage

Tests added/updated in tests/boundary/signals/recordingProvider.test.ts. They use the command simulator with real process ownership; this is not real sigrok.
- reports an unknown decoder ID as the rejected field of each MCP operation: goes through the production MCP harness and checks both tools. Each returns invalid_request with its own field, operation, recovery text and captured_output, and the workspace is released.
- reports an unknown decoder help selection as CLI invalid input: runs inspect-signal-decoders --decoder nosuchdecoder --format json and checks the same typed error.
- keeps %s outside caller selectors %j as a provider failure: adds help cases. They cover an installed decoder with a missing dependency, a refusal naming a different ID, and the catalog listing. All stay ProviderAdapterError.
scripts/verify-signal-recording.mjs (real sigrok in the Real signal recording workflow) now checks invalid_request and the issue path for decoders: "no_such_decoder" and decoder: "rea_no_such_decoder". Before, it only checked isError.
Base reproduction (executed): on f801b2fbf the issue's repro test passed as written, confirming execution_failure + doctor advice. Run against the base source, the two new tests failed with "code": "execution_failure" where invalid_request was expected. With the fix, the same repro fails its bug assertion and returns the output below.

User-visible MCP output, from the issue's repro with the fix. It is trimmed: stdout, truncated and signal are omitted.

Before:

{"code":"execution_failure","category":"execution_failure","message":"Analysis could not complete. Run `rea doctor` and review the reported details; repeating the unchanged request is not expected to succeed.","details":{"provider_id":"sigrok-recording","operation":"inspect_signal_decoders","diagnostics":{"exit_code":1,"stderr":"srd: ModuleNotFoundError: Failed to load decoder nosuchdecoder: import by name failed: No module named 'nosuchdecoder'\nFailed to load protocol decoder nosuchdecoder.\n"}}}

After:

{"code":"invalid_request","category":"invalid_input","message":"sigrok-cli rejected the decoder selector: Failed to load protocol decoder nosuchdecoder. Omit decoder to list installed decoder IDs, optionally filtered by query, then request help for one of those IDs.","recovery":{"kind":"inspect_input_contract","operation":"inspect_signal_decoders"},"details":{"operation":"inspect_signal_decoders","issues":[{"path":["decoder"],"reason":"invalid_value","message":"sigrok-cli rejected the decoder selector: …"}],"captured_output":{"exit_code":1,"stderr":"srd: ModuleNotFoundError: Failed to load decoder nosuchdecoder: …\nFailed to load protocol decoder nosuchdecoder.\n"}}}

Remaining proof gaps: which sigrok-cli finds nothing on this macOS host, so sigrok was simulated locally. One claim is source-derived only: sigrok-cli 0.7.2 loads -P decoders before it dispatches --show, so --show emits the same two-line refusal. The real-sigrok CI lane will confirm or refute this through the strengthened verifier assertion.

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

Issue repro (npm run test:local -- src/repro.test.ts, temporary file, since removed). On base it passed, confirming the bug. With the fix it fails its bug assertion and returns the invalid_request output above.
npm run test:focused -- tests/boundary/signals/recordingProvider.test.ts against base source: 2 failed (the new tests), 44 passed.
VITEST_MAX_WORKERS=2 npm run test:focused -- tests/boundary/signals/decoderDiscovery.test.ts src/process/LinuxResourceLimits.test.ts src/domain/signals/annotationTextWindow.test.ts tests/boundary/signals/recordingProvider.test.ts tests/boundary/signals/recordingDescriptorCleanup.test.ts tests/boundary/signals/resourceLimits.test.ts tests/boundary/mcp/signalRecordingMcp.test.ts tests/boundary/cli/signalCleanup.test.ts src/process/ArtifactAnalysisResources.test.ts: 97 passed, 9 skipped (Linux-only on macOS).
npm run check:fast: passed.
npm run format:check: passed.
npm run docs:check: passed.
node --check scripts/verify-signal-recording.mjs: passed. The verifier itself was not run locally because it needs Linux and real sigrok.

Compatibility, safety, and release

Breaking changes or migration steps: none. Only the error code for the matched refusal changes.
Real Hopper/Ghidra, browser, or OS coverage: no local real sigrok; macOS host only. Real sigrok coverage comes from the Real signal recording CI workflow.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new process, file or network behavior. The retained stderr was already retained before.

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/5858 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/21/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/signal-decoder-unknown-id` --- ### 📝 Commits (2) - [`fe643f2`](https://github.com/morluto/rea/commit/fe643f2eabd068e0f6d9719c9abe771980b44c8a) fix(signals): report unknown decoder help IDs as invalid input - [`6c4434e`](https://github.com/morluto/rea/commit/6c4434e2cb6d1fbd4aa5d572fb6ef8f336da4581) Merge remote-tracking branch 'origin/main' into fix/signal-decoder-unknown-id ### 📊 Changes **5 files changed** (+179 additions, -35 deletions) <details> <summary>View changed files</summary> 📝 `docs/signal-recordings.md` (+3 -0) 📝 `scripts/verify-signal-recording.mjs` (+20 -5) 📝 `src/signals/SigrokRecordingProvider.ts` (+7 -10) 📝 `src/signals/sigrokSelectorRejection.ts` (+54 -18) 📝 `tests/boundary/signals/recordingProvider.test.ts` (+95 -2) </details> ### 📄 Description **What broke:** `inspect_signal_decoders` with an unknown `decoder` reported sigrok's explicit missing-decoder refusal as `execution_failure` with `rea doctor` advice, while `decode_signal_recording` already reported the same refusal as `invalid_request`. **Fix:** the sigrok selector classifier now owns the unknown-ID match for both operations; help names `decoder` and points to catalog discovery, decoding keeps `decoders`/`annotations`. **Risk:** low; only the exact two-line refusal naming the caller's ID changes class. Other non-zero exits stay `execution_failure`. Tests use a sigrok simulator; the real-sigrok CI lane now asserts the typed outcome. ## Summary Fixes #5849. Asking for help on a decoder ID that is not installed now returns `invalid_request` with an issue on `["decoder"]`. The message tells the caller to omit `decoder` to list installed IDs. sigrok's output is kept in `captured_output`. The response now matches the one `decode_signal_recording` gives for the same refusal. ## Problem and expected behavior Discovery and decoding share one command executor in `SigrokRecordingProvider`, but only decoding passed the selector-refusal classifier (added in #5570). The `--show` help command therefore fell through to `ProviderAdapterError` → `execution_failure` + doctor advice, with no field-level correction. Expected: when sigrok refuses a caller-selected decoder ID that is absent, the response is an input error on the field that selected it. An installed decoder whose own dependency is missing remains a provider failure. ## Change and scope - `src/signals/sigrokSelectorRejection.ts` is the one owner of this interpretation. It takes either selection shape (`{decoders, annotations}` for decoding, `{decoder}` for help). A single private matcher recognizes the unknown-ID refusal for both. Each field gets its own recovery text. - `SigrokRecordingProvider.execute` now takes the caller selection that a command loads, instead of a per-call classifier closure. Both producer commands pass their input. The `--version` probe passes nothing. The superseded closure parameter is removed. - The matched strings are unchanged: `Failed to load protocol decoder <id>.` plus `srd: ModuleNotFoundError: Failed to load decoder <id>: import by name failed: No module named '<id>'`. They match only when `<id>` equals the caller's selection. - Sibling check: these are the only two signal operations. The catalog listing (`decoder: null`) selects no ID, so it is never classified. - Not included: option/channel refusals and unknown input formats. Their real sigrok wording has not been observed, so they keep failing closed as `execution_failure`. - Scope decision: not applicable (error classification within an existing provider boundary). ## Contract and boundary impact - Semantic owner and earliest changed stage: sigrok provider adapter (`src/signals/sigrokSelectorRejection.ts`), where producer 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 help refusal 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: pinned to sigrok-cli 0.7.2 strings already used for decoding; unmatched output keeps prior behavior. - Evidence, artifact, provenance, or reconstruction contract: unchanged; full stderr retained. - Target selection, authority, upstream behavior, and cleanup details: unchanged; tests confirm the private workspace is released. - Generated metadata, package, or installation impact: none. `docs/signal-recordings.md` gains three sentences. 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. (Existing discovery already launches sigrok-cli. This PR does not change that.) ## Evidence and regression coverage - Tests added/updated in `tests/boundary/signals/recordingProvider.test.ts`. They use the command simulator with real process ownership; this is not real sigrok. - `reports an unknown decoder ID as the rejected field of each MCP operation`: goes through the production MCP harness and checks both tools. Each returns `invalid_request` with its own field, operation, recovery text and `captured_output`, and the workspace is released. - `reports an unknown decoder help selection as CLI invalid input`: runs `inspect-signal-decoders --decoder nosuchdecoder --format json` and checks the same typed error. - `keeps %s outside caller selectors %j as a provider failure`: adds help cases. They cover an installed decoder with a missing dependency, a refusal naming a different ID, and the catalog listing. All stay `ProviderAdapterError`. - `scripts/verify-signal-recording.mjs` (real sigrok in the `Real signal recording` workflow) now checks `invalid_request` and the issue path for `decoders: "no_such_decoder"` and `decoder: "rea_no_such_decoder"`. Before, it only checked `isError`. - Base reproduction (executed): on `f801b2fbf` the issue's repro test passed as written, confirming `execution_failure` + doctor advice. Run against the base source, the two new tests failed with `"code": "execution_failure"` where `invalid_request` was expected. With the fix, the same repro fails its bug assertion and returns the output below. - User-visible MCP output, from the issue's repro with the fix. It is trimmed: stdout, truncated and signal are omitted. Before: ```json {"code":"execution_failure","category":"execution_failure","message":"Analysis could not complete. Run `rea doctor` and review the reported details; repeating the unchanged request is not expected to succeed.","details":{"provider_id":"sigrok-recording","operation":"inspect_signal_decoders","diagnostics":{"exit_code":1,"stderr":"srd: ModuleNotFoundError: Failed to load decoder nosuchdecoder: import by name failed: No module named 'nosuchdecoder'\nFailed to load protocol decoder nosuchdecoder.\n"}}} ``` After: ```json {"code":"invalid_request","category":"invalid_input","message":"sigrok-cli rejected the decoder selector: Failed to load protocol decoder nosuchdecoder. Omit decoder to list installed decoder IDs, optionally filtered by query, then request help for one of those IDs.","recovery":{"kind":"inspect_input_contract","operation":"inspect_signal_decoders"},"details":{"operation":"inspect_signal_decoders","issues":[{"path":["decoder"],"reason":"invalid_value","message":"sigrok-cli rejected the decoder selector: …"}],"captured_output":{"exit_code":1,"stderr":"srd: ModuleNotFoundError: Failed to load decoder nosuchdecoder: …\nFailed to load protocol decoder nosuchdecoder.\n"}}} ``` - Remaining proof gaps: `which sigrok-cli` finds nothing on this macOS host, so sigrok was simulated locally. One claim is source-derived only: sigrok-cli 0.7.2 loads `-P` decoders before it dispatches `--show`, so `--show` emits the same two-line refusal. The real-sigrok CI lane will confirm or refute this through the strengthened verifier assertion. 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 - Issue repro (`npm run test:local -- src/repro.test.ts`, temporary file, since removed). On base it passed, confirming the bug. With the fix it fails its bug assertion and returns the `invalid_request` output above. - `npm run test:focused -- tests/boundary/signals/recordingProvider.test.ts` against base source: 2 failed (the new tests), 44 passed. - `VITEST_MAX_WORKERS=2 npm run test:focused -- tests/boundary/signals/decoderDiscovery.test.ts src/process/LinuxResourceLimits.test.ts src/domain/signals/annotationTextWindow.test.ts tests/boundary/signals/recordingProvider.test.ts tests/boundary/signals/recordingDescriptorCleanup.test.ts tests/boundary/signals/resourceLimits.test.ts tests/boundary/mcp/signalRecordingMcp.test.ts tests/boundary/cli/signalCleanup.test.ts src/process/ArtifactAnalysisResources.test.ts`: 97 passed, 9 skipped (Linux-only on macOS). - `npm run check:fast`: passed. - `npm run format:check`: passed. - `npm run docs:check`: passed. - `node --check scripts/verify-signal-recording.mjs`: passed. The verifier itself was not run locally because it needs Linux and real sigrok. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only the error code for the matched refusal changes. - Real Hopper/Ghidra, browser, or OS coverage: no local real sigrok; macOS host only. Real sigrok coverage comes from the `Real signal recording` CI workflow. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new process, file or network behavior. The retained stderr was already retained before. ## 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 - [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. 🤖 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:45:18 +00:00