morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #5984] [Bug] Sigrok version-probe failures bypass configuration error guidance #3653

New issue

Closed

opened 2026-10-31 00:42:44 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 00:42:44 +00:00

Owner

Originally created by @coygeek on GitHub (Oct 21, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/5984

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

Provider or capability discovery

Observed behavior

A configured sigrok command that exits nonzero during --version is reported as execution_failure with generic rea doctor advice. It does not receive the sigrok configuration guidance used for an unsupported version banner. The standard details.captured_output field is absent, although stdout, stderr and the exit code survive under details.diagnostics.

Reproduced through the production in-memory MCP harness for both inspect_signal_decoders and decode_signal_recording, using a command simulator. With identical stdout/stderr, changing only the probe exit from 0 to 1 changes the result from capability_unavailable with configuration guidance to execution_failure without it. No real sigrok installation was used.

Reproduction

In a checkout of the revision below with locked dependencies installed, add this focused test as tests/boundary/mcp/sigrokVersionProbe.test.ts:

import { it, expect, onTestFinished } from "vitest";
import { chmod, readdir, writeFile } from "node:fs/promises";
import { join } from "node:path";
import { SigrokRecordingProvider } from "../../../src/signals/SigrokRecordingProvider.js";
import { SignalRecordingService } from "../../../src/application/signals/SignalRecordingService.js";
import { PrivateRuntimeRoot } from "../../../src/process/PrivateRuntimeRoot.js";
import { spawnOwnedProviderProcess } from "../../../src/process/ProviderProcess.js";
import { readStableArtifact } from "../../../src/artifacts/readStableArtifact.js";
import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js";
import { createApplicationMcpHarness } from "../../fixtures/applicationMcpHarness.js";
import { parseMcpToolError } from "../../fixtures/mcpToolError.js";

it.each([0, 1])("compares unsupported version probes with exit %s", async (exitCode) => {
  const directory = await createTestTempDirectory("rea-sigrok-probe-");
  const script = join(directory, "producer.mjs");
  await writeFile(script, `
if (!process.argv.includes('--version')) throw Error('unexpected producer command');
console.log('sigrok-cli 0.7.1');
console.error('selected installation is unusable');
process.exitCode = ${exitCode};
`);
  await chmod(script, 0o700);
  const calls: string[][] = [];
  const provider = new SigrokRecordingProvider(
    { ...process.env, REA_SIGROK_COMMAND: script, REA_SIGROK_PRLIMIT_COMMAND: script },
    async (input) => {
      calls.push([...input.arguments]);
      return spawnOwnedProviderProcess({ ...input, command: process.execPath,
        arguments: [script, ...input.arguments], expectedCommand: process.execPath });
    },
    () => PrivateRuntimeRoot.create({ parent: directory, prefix: "decoder-" }),
    "linux", readStableArtifact, async (limits) => limits,
  );
  onTestFinished(() => provider.close());
  const harness = await createApplicationMcpHarness({ serverOptions: {
    environment: {}, signalRecording: new SignalRecordingService(provider),
  } });
  onTestFinished(harness.close);
  for (const name of ["inspect_signal_decoders", "decode_signal_recording"]) {
    const response = await harness.client.callTool({
      name, arguments: name === "inspect_signal_decoders" ? {} : {
        path: join(directory, "unopened-recording.vcd"), decoders: "uart",
      },
    });
    const error = parseMcpToolError(response).error;
    expect(error.details?.operation).toBe(name);
    if (exitCode === 1) {
      expect(error).toMatchObject({ code: "execution_failure", details: {
        diagnostics: { exit_code: 1, stdout: "sigrok-cli 0.7.1\n",
          stderr: "selected installation is unusable\n" },
      } });
      expect(error.message).toContain("Run `rea doctor`");
      expect(error.details?.captured_output).toBeUndefined();
      expect(JSON.stringify(error)).not.toContain("REA_SIGROK_COMMAND");
    } else {
      expect(error).toMatchObject({ code: "capability_unavailable", details: {
        captured_output: { exit_code: 0, stdout: "sigrok-cli 0.7.1\n",
          stderr: "selected installation is unusable\n" },
      } });
      expect(error.message).toContain("REA_SIGROK_COMMAND");
      expect(error.message).toContain("REA_SIGROK_PRLIMIT_COMMAND");
    }
    console.log(JSON.stringify({ probe_exit: exitCode, operation: name, error }));
  }
  expect(calls).toHaveLength(2);
  expect(calls.every((args) => args.includes("--version"))).toBe(true);
  expect((await readdir(directory)).filter((name) => name.startsWith("decoder-"))).toEqual([]);
  await harness.client.ping();
});

Run it, then remove the temporary test:

probe_root="$(mktemp -d)"
mkdir -p "$probe_root/runtime"
TMPDIR="$probe_root/runtime" node node_modules/vitest/vitest.mjs run \
  tests/boundary/mcp/sigrokVersionProbe.test.ts --maxWorkers=1
rm tests/boundary/mcp/sigrokVersionProbe.test.ts

Executed once: 1 test file, 2 tests passed, covering four MCP calls. The assertions above describe the current incorrect behavior. The simulator follows the existing signal boundary tests: it injects Linux admission and a limit reader, then launches an owned Node process in place of sigrok/prlimit. It exercises service validation, executable identity checks, command exit collection, MCP projection, and workspace cleanup. It does not exercise Linux resource enforcement or real sigrok behavior.

Expected behavior

A completed version probe that cannot establish a usable sigrok installation should follow the provider's configuration-failure path: capability_unavailable, the existing guidance naming REA_SIGROK_COMMAND and REA_SIGROK_PRLIMIT_COMMAND, and bounded captured command output.

This follows the caller-supplied prerequisites and the provider's configuration classifier. The executed exit-zero unsupported-banner control already produces this result. Timeout, cancellation, signal, integrity, and producer-phase failures need to retain their own applicable error semantics.

Impact and scope

P2: a bounded typed-outcome and recovery defect for Linux users whose selected executable runs but fails its version probe. Callers cannot distinguish an unavailable installation from an adapter execution failure by the returned code, and command output is missing from its standard captured-output field. Manual diagnosis remains possible through the retained diagnostics and documented configuration settings.

Both tested tools failed before any producer command. The decode path did not open the nonexistent recording fixture. Workspaces were removed and the MCP session answered a subsequent ping. No artifact mutation or security boundary crossing was observed. Frequency across real sigrok installations is unknown.

REA version

rea-agents 6.7.0; source revision 5241597594.

Runtime and platform

Node.js v26.12.0; macOS 27.0.1 arm64. Production in-memory MCP with Linux platform/limit-reader injection and a Node command simulator.

Target and artifact context

No active binary target or hardware. Decoder discovery is target-free. The decode request names a nonexistent synthetic recording path; the version probe fails before artifact access.

Interface

MCP

Earliest failure stage

Setup or provider admission

Relevant output or evidence

The executed test produced these outcomes for both operations. Trimmed example for discovery:

{
  "probe_exit": 1,
  "error": {
    "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.",
    "retryable": false,
    "details": {
      "provider_id": "sigrok-recording",
      "operation": "inspect_signal_decoders",
      "diagnostics": {
        "exit_code": 1,
        "stdout": "sigrok-cli 0.7.1\n",
        "stderr": "selected installation is unusable\n"
      }
    }
  }
}

For exit 0 with the same stdout/stderr, the observed result was capability_unavailable, category: "unsupported_provider", retryable: false, and details.captured_output containing stdout, stderr, exit_code: 0, truncated: false, and signal: null. Its message and remediation said:

Expected sigrok-cli 0.7.2 with libsigrokdecode; inspect the selected installation. On Linux, configure sigrok-cli 0.7.2 through REA_SIGROK_COMMAND and util-linux prlimit through REA_SIGROK_PRLIMIT_COMMAND (default /usr/bin/prlimit).

Source trace at the tested revision:

The MCP registrations reach the shared signal service and provider.
The nonzero-exit branch throws ProviderAdapterError before the version-banner check.
The typed-error early return bypasses the configuration classifier. The projection mapping produces the differing public codes.

Typed outcome or recovery expected

For this completed nonzero version probe, AnalysisCapabilityUnavailableError projected as capability_unavailable, configuration guidance identifying the selected tool and limiter settings, and captured_output retaining the probe's bounded stdout/stderr and exit status.

Analysis or suspected cause

Source-derived: the shared command executor chooses ProviderAdapterError without considering the current phase. The failure handler preserves this typed error before it can consult the configuration phase. A plain error from the exit-zero banner check reaches that phase-specific path instead. The early branch also omits the already collected capturedOutput when constructing its typed error.

Additional context

The closest reports concern rejected decoder/annotation selectors (#5515, #5849), subsequently addressed by #5570 and #5858; #5854 also targets decoder help. They run after a successful version probe and require an input error rather than a configuration error. #2997 concerns attributing a successful probe's output to a later command that fails to launch; it explicitly excludes classification. #5730 concerns unsupported-host availability inventory. None addresses this completed nonzero version probe.

Checked upstream main at 25e1a24a3e. Nearby selector plumbing and cleanup projection changed, but the version-probe call and offending classification branches remain unchanged. This freshness check was source comparison; no runtime run against that newer revision was performed.

NOT_ENOUGH_INFO: first affected revision and frequency in real installations were not established. No fix or restoration was tested.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5984 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area Provider or capability discovery ### Observed behavior A configured sigrok command that exits nonzero during `--version` is reported as `execution_failure` with generic `rea doctor` advice. It does not receive the sigrok configuration guidance used for an unsupported version banner. The standard `details.captured_output` field is absent, although stdout, stderr and the exit code survive under `details.diagnostics`. Reproduced through the production in-memory MCP harness for both `inspect_signal_decoders` and `decode_signal_recording`, using a command simulator. With identical stdout/stderr, changing only the probe exit from 0 to 1 changes the result from `capability_unavailable` with configuration guidance to `execution_failure` without it. No real sigrok installation was used. ### Reproduction In a checkout of the revision below with locked dependencies installed, add this focused test as `tests/boundary/mcp/sigrokVersionProbe.test.ts`: ```ts import { it, expect, onTestFinished } from "vitest"; import { chmod, readdir, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { SigrokRecordingProvider } from "../../../src/signals/SigrokRecordingProvider.js"; import { SignalRecordingService } from "../../../src/application/signals/SignalRecordingService.js"; import { PrivateRuntimeRoot } from "../../../src/process/PrivateRuntimeRoot.js"; import { spawnOwnedProviderProcess } from "../../../src/process/ProviderProcess.js"; import { readStableArtifact } from "../../../src/artifacts/readStableArtifact.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { createApplicationMcpHarness } from "../../fixtures/applicationMcpHarness.js"; import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; it.each([0, 1])("compares unsupported version probes with exit %s", async (exitCode) => { const directory = await createTestTempDirectory("rea-sigrok-probe-"); const script = join(directory, "producer.mjs"); await writeFile(script, ` if (!process.argv.includes('--version')) throw Error('unexpected producer command'); console.log('sigrok-cli 0.7.1'); console.error('selected installation is unusable'); process.exitCode = ${exitCode}; `); await chmod(script, 0o700); const calls: string[][] = []; const provider = new SigrokRecordingProvider( { ...process.env, REA_SIGROK_COMMAND: script, REA_SIGROK_PRLIMIT_COMMAND: script }, async (input) => { calls.push([...input.arguments]); return spawnOwnedProviderProcess({ ...input, command: process.execPath, arguments: [script, ...input.arguments], expectedCommand: process.execPath }); }, () => PrivateRuntimeRoot.create({ parent: directory, prefix: "decoder-" }), "linux", readStableArtifact, async (limits) => limits, ); onTestFinished(() => provider.close()); const harness = await createApplicationMcpHarness({ serverOptions: { environment: {}, signalRecording: new SignalRecordingService(provider), } }); onTestFinished(harness.close); for (const name of ["inspect_signal_decoders", "decode_signal_recording"]) { const response = await harness.client.callTool({ name, arguments: name === "inspect_signal_decoders" ? {} : { path: join(directory, "unopened-recording.vcd"), decoders: "uart", }, }); const error = parseMcpToolError(response).error; expect(error.details?.operation).toBe(name); if (exitCode === 1) { expect(error).toMatchObject({ code: "execution_failure", details: { diagnostics: { exit_code: 1, stdout: "sigrok-cli 0.7.1\n", stderr: "selected installation is unusable\n" }, } }); expect(error.message).toContain("Run `rea doctor`"); expect(error.details?.captured_output).toBeUndefined(); expect(JSON.stringify(error)).not.toContain("REA_SIGROK_COMMAND"); } else { expect(error).toMatchObject({ code: "capability_unavailable", details: { captured_output: { exit_code: 0, stdout: "sigrok-cli 0.7.1\n", stderr: "selected installation is unusable\n" }, } }); expect(error.message).toContain("REA_SIGROK_COMMAND"); expect(error.message).toContain("REA_SIGROK_PRLIMIT_COMMAND"); } console.log(JSON.stringify({ probe_exit: exitCode, operation: name, error })); } expect(calls).toHaveLength(2); expect(calls.every((args) => args.includes("--version"))).toBe(true); expect((await readdir(directory)).filter((name) => name.startsWith("decoder-"))).toEqual([]); await harness.client.ping(); }); ``` Run it, then remove the temporary test: ```sh probe_root="$(mktemp -d)" mkdir -p "$probe_root/runtime" TMPDIR="$probe_root/runtime" node node_modules/vitest/vitest.mjs run \ tests/boundary/mcp/sigrokVersionProbe.test.ts --maxWorkers=1 rm tests/boundary/mcp/sigrokVersionProbe.test.ts ``` Executed once: 1 test file, 2 tests passed, covering four MCP calls. The assertions above describe the current incorrect behavior. The simulator follows the existing signal boundary tests: it injects Linux admission and a limit reader, then launches an owned Node process in place of sigrok/prlimit. It exercises service validation, executable identity checks, command exit collection, MCP projection, and workspace cleanup. It does not exercise Linux resource enforcement or real sigrok behavior. ### Expected behavior A completed version probe that cannot establish a usable sigrok installation should follow the provider's configuration-failure path: `capability_unavailable`, the existing guidance naming `REA_SIGROK_COMMAND` and `REA_SIGROK_PRLIMIT_COMMAND`, and bounded captured command output. This follows the [caller-supplied prerequisites](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/signal-recordings.md#L3-L9) and the [provider's configuration classifier](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/signals/SigrokRecordingProvider.ts#L526-L538). The executed exit-zero unsupported-banner control already produces this result. Timeout, cancellation, signal, integrity, and producer-phase failures need to retain their own applicable error semantics. ### Impact and scope P2: a bounded typed-outcome and recovery defect for Linux users whose selected executable runs but fails its version probe. Callers cannot distinguish an unavailable installation from an adapter execution failure by the returned code, and command output is missing from its standard captured-output field. Manual diagnosis remains possible through the retained diagnostics and documented configuration settings. Both tested tools failed before any producer command. The decode path did not open the nonexistent recording fixture. Workspaces were removed and the MCP session answered a subsequent ping. No artifact mutation or security boundary crossing was observed. Frequency across real sigrok installations is unknown. ### REA version rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e. ### Runtime and platform Node.js v26.12.0; macOS 27.0.1 arm64. Production in-memory MCP with Linux platform/limit-reader injection and a Node command simulator. ### Target and artifact context No active binary target or hardware. Decoder discovery is target-free. The decode request names a nonexistent synthetic recording path; the version probe fails before artifact access. ### Interface MCP ### Earliest failure stage Setup or provider admission ### Relevant output or evidence The executed test produced these outcomes for both operations. Trimmed example for discovery: ```json { "probe_exit": 1, "error": { "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.", "retryable": false, "details": { "provider_id": "sigrok-recording", "operation": "inspect_signal_decoders", "diagnostics": { "exit_code": 1, "stdout": "sigrok-cli 0.7.1\n", "stderr": "selected installation is unusable\n" } } } } ``` For exit 0 with the same stdout/stderr, the observed result was `capability_unavailable`, `category: "unsupported_provider"`, `retryable: false`, and `details.captured_output` containing stdout, stderr, `exit_code: 0`, `truncated: false`, and `signal: null`. Its message and remediation said: ```text Expected sigrok-cli 0.7.2 with libsigrokdecode; inspect the selected installation. On Linux, configure sigrok-cli 0.7.2 through REA_SIGROK_COMMAND and util-linux prlimit through REA_SIGROK_PRLIMIT_COMMAND (default /usr/bin/prlimit). ``` Source trace at the tested revision: - The [MCP registrations](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/server/registerSignalTools.ts#L20-L43) reach the shared signal service and provider. - The [nonzero-exit branch](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/signals/SigrokRecordingProvider.ts#L244-L255) throws `ProviderAdapterError` before the [version-banner check](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/signals/SigrokRecordingProvider.ts#L258-L266). - The [typed-error early return](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/signals/SigrokRecordingProvider.ts#L429) bypasses the configuration classifier. The [projection mapping](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/domain/analysisErrorProjection.ts#L242-L262) produces the differing public codes. ### Typed outcome or recovery expected For this completed nonzero version probe, `AnalysisCapabilityUnavailableError` projected as `capability_unavailable`, configuration guidance identifying the selected tool and limiter settings, and `captured_output` retaining the probe's bounded stdout/stderr and exit status. ### Analysis or suspected cause Source-derived: the shared command executor chooses `ProviderAdapterError` without considering the current phase. The failure handler preserves this typed error before it can consult the configuration phase. A plain error from the exit-zero banner check reaches that phase-specific path instead. The early branch also omits the already collected `capturedOutput` when constructing its typed error. ### Additional context The closest reports concern rejected decoder/annotation selectors (#5515, #5849), subsequently addressed by #5570 and #5858; #5854 also targets decoder help. They run after a successful version probe and require an input error rather than a configuration error. #2997 concerns attributing a successful probe's output to a later command that fails to launch; it explicitly excludes classification. #5730 concerns unsupported-host availability inventory. None addresses this completed nonzero version probe. Checked upstream main at 25e1a24a3ed3671a0148757555f18d16c29d936a. Nearby selector plumbing and cleanup projection changed, but the version-probe call and offending classification branches remain unchanged. This freshness check was source comparison; no runtime run against that newer revision was performed. NOT_ENOUGH_INFO: first affected revision and frequency in real installations were not established. No fix or restoration was tested.

bsakaguchi closed this issue

2026-10-31 00:42:44 +00:00

bsakaguchi commented

2026-10-31 00:42:45 +00:00

Author

Owner

@coygeek commented on GitHub (Oct 24, 2026):

Checked the reported fix on main at d87e73f2db.

A completed nonzero configuration-phase probe now throws into the phase-aware configuration mapper before the general ProviderAdapterError branch. Captured output is passed to that mapper. The focused service regression tests both discovery and decode, requiring capability_unavailable, REA_SIGROK_COMMAND guidance and captured exit/stdout/stderr; real sigrok and Linux containment remain outside this verification.

Implementation: SigrokRecordingProvider.ts, recordingProvider.test.ts.

The relevant regression checks in recordingProvider.test.ts passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

@coygeek commented on GitHub (Oct 24, 2026): Checked the reported fix on `main` at d87e73f2db50be72d33a4b3c7199af35518b441c. A completed nonzero configuration-phase probe now throws into the phase-aware configuration mapper before the general ProviderAdapterError branch. Captured output is passed to that mapper. The focused service regression tests both discovery and decode, requiring capability_unavailable, REA_SIGROK_COMMAND guidance and captured exit/stdout/stderr; real sigrok and Linux containment remain outside this verification. Implementation: [SigrokRecordingProvider.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/signals/SigrokRecordingProvider.ts#L247), [recordingProvider.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/signals/recordingProvider.test.ts#L1396). The relevant regression checks in [recordingProvider.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/signals/recordingProvider.test.ts) passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

bsakaguchi referenced this issue

2026-10-31 00:45:35 +00:00

[GH-ISSUE #6569] [Bug] Model summaries report no chat template for named-only GGUF templates #3958

bsakaguchi referenced this issue

2026-11-01 21:10:57 +00:00

[GH-ISSUE #3796] [Tracker] Open feature requests by area (October 2026) #1471