morluto/rea

Fork 0

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

[GH-ISSUE #5090] [Bug] debug-with-rizin retains failure exit status after fallback cleanup succeeds #3128

New issue

Closed

opened 2026-10-31 00:36:57 +00:00 by bsakaguchi · 4 comments

bsakaguchi commented

2026-10-31 00:36:57 +00:00

Owner

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

Before submitting

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

Area

CLI

Observed behavior

A mocked production CLI-handler probe returns the original command Evidence and completes fallback cleanup, but debug-with-rizin leaves process.exitCode at 1 when the initial per-session close returned an incomplete-cleanup error. The control with a successful initial close leaves it at 0.

This executes the registered handler with mocked manager methods. No real Rizin process, target or packaged CLI subprocess was executed.

Reproduction

In a checkout of the source revision below with locked dependencies installed, save this probe as tests/boundary/cli/rizinCleanupProbe.test.ts:

import pino from 'pino';
import { it, expect, vi } from 'vitest';
import { registerReverseEngineeringCommands } from '../../../src/cli/reverseEngineeringCommands.js';
import { RizinDebugSessionManager } from '../../../src/rizin/RizinDebugSessionManager.js';
import { AnalysisOutputError } from '../../../src/domain/analysisErrorCore.js';
import { createEvidence } from '../../../src/domain/evidence.js';
import { ok, err } from '../../../src/domain/result.js';

it('observes the registered Rizin command after successful fallback cleanup', async () => {
  const saved = process.exitCode;
  const evidence = createEvidence(undefined, {id:'rizin.debugger',name:'Rizin debugger',version:null}, {
    predicateType:'rea.debugger.command-observation', operation:'rizin_debug_command',
    parameters:{command:'dr'}, result:{output:'fixture',completion_status:'complete'}, rawResult:{output:'fixture'}
  });
  try {
    for (const initialFailure of [false, true]) {
      process.exitCode = 0;
      vi.spyOn(RizinDebugSessionManager.prototype,'start').mockResolvedValue(ok({session_id:'fixture-session',backend:null}));
      vi.spyOn(RizinDebugSessionManager.prototype,'execute').mockResolvedValue(ok({evidence,result:{command:'dr',output:'fixture',output_scope:'command_and_interleaved_session_output',output_truncated:false,completion_status:'complete',backend:null}}));
      const failure = new AnalysisOutputError('close_rizin_debug_session','synthetic incomplete first stop',{cleanup:{reason:'synthetic stop pending',resources:['fixture-session']}});
      vi.spyOn(RizinDebugSessionManager.prototype,'close').mockResolvedValue(initialFailure ? err(failure) : ok({target_state:'unknown'}));
      const fallback = vi.spyOn(RizinDebugSessionManager.prototype,'closeAll').mockImplementation(async()=> { failure.resolveCleanup(); });
      let run: any;
      const cli: any = {command(name:string, definition:any) { if(name==='debug-with-rizin') run=definition.run; return cli; }};
      registerReverseEngineeringCommands(cli,pino({enabled:false}),{});
      const result = await run({args:{path:'fixture.bin',command:'dr'},options:{inferiorTty:'/dev/pts/7'}});
      expect(result).toBe(evidence);
      expect(fallback).toHaveBeenCalledOnce();
      expect(failure.cleanupIncomplete).toBe(false);
      console.log(JSON.stringify({firstClose:initialFailure?'incomplete':'complete',fallback:'resolved',returnedOriginalEvidence:result===evidence,exitCode:process.exitCode}));
      expect(process.exitCode).toBe(initialFailure ? 1 : 0);
      vi.restoreAllMocks();
    }
  } finally { vi.restoreAllMocks(); process.exitCode=saved; }
});

Run with a disposable HOME:

WORK=$(mktemp -d)
HOME="$WORK" node node_modules/vitest/vitest.mjs run --project cli-boundary tests/boundary/cli/rizinCleanupProbe.test.ts --maxWorkers=1 --no-file-parallelism

One executed test passed, characterizing both cases once. Remove the probe and its disposable HOME afterward. Assertions observe the current defect rather than assert the proposed correction.

Expected behavior

For a successful command with confirmed fallback cleanup and no independent operation failure or cancellation, retain success status. The CLI status table assigns 0 to completed operations and 1 to operations that could not complete. The lifecycle contract resolves current cleanup uncertainty after confirmed release while retaining completed observations and historical diagnostics. The GDB one-shot handler already clears its cleanup flag after successful fallback.

Impact and scope

The demonstrated effect is a misleading failure status despite returned command Evidence and resolved fallback cleanup. Automation using status alone can treat a completed invocation as failed. No Evidence loss or remaining resource was demonstrated. Real-provider frequency and a general caller-side workaround are NOT_ENOUGH_INFO; the probe establishes the CLI adapter boundary only.

REA version

Source 3537e58c1ab29309f06eabf50bc52adba9817a0c, package manifest 6.7.0. Relevant source is unchanged from published stable tag rea-agents-6.7.0; the published package and stable runtime were not executed.

Runtime and platform

Node.js v26.12.0; macOS 27.0.1; arm64; locked Vitest 5.0.3. This run did not use the repository's Node 24.18.0 pin.

Target and artifact context

Synthetic successful command Evidence and mocked manager lifecycle outcomes. No artifact was opened and no target or provider executable was launched. The example TTY and artifact names are unused mock inputs.

Interface

CLI

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed in the focused test:

{"firstClose":"complete","fallback":"resolved","returnedOriginalEvidence":true,"exitCode":0}
{"firstClose":"incomplete","fallback":"resolved","returnedOriginalEvidence":true,"exitCode":1}

The Rizin handler sets failure status before fallback and does not revise it after fallback succeeds.

SOURCE-ONLY reachability corroboration: close retains a session after an incomplete stop. closeAll retries that session through the shared cleanup owner. The stop wrapper resolves retained cleanup errors after a confirmed stop. A bounded stream settlement wait can be incomplete before later settlement. No natural timing incident was reproduced.

Typed outcome or recovery expected

Keep the original successful Evidence, preserve historical cleanup diagnostics, and report success when final cleanup is confirmed. Preserve nonzero status for independently failed operations, cancellation or unresolved cleanup.

Analysis or suspected cause

The CLI writes process.exitCode = 1 immediately on the first close error. Successful fallback resolves cleanup ownership but cannot revise this separate status assignment. The adjacent GDB handler defers the status decision until fallback finishes.

Additional context

Searched the supplied complete open-and-closed issue snapshot. Related lifecycle audit #4227, CDP cleanup marker issue #4467, and web signal exit-status issue #1126 concern different triggers and outcomes. No matching report found. Real Rizin/Linux execution, packaged subprocess exit and the frequency of transient cleanup failure remain NOT_ENOUGH_INFO.

Originally created by @coygeek on GitHub (Oct 18, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5090 ## 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 CLI ## Observed behavior A mocked production CLI-handler probe returns the original command Evidence and completes fallback cleanup, but `debug-with-rizin` leaves `process.exitCode` at `1` when the initial per-session close returned an incomplete-cleanup error. The control with a successful initial close leaves it at `0`. This executes the registered handler with mocked manager methods. No real Rizin process, target or packaged CLI subprocess was executed. ## Reproduction In a checkout of the source revision below with locked dependencies installed, save this probe as `tests/boundary/cli/rizinCleanupProbe.test.ts`: ```ts import pino from 'pino'; import { it, expect, vi } from 'vitest'; import { registerReverseEngineeringCommands } from '../../../src/cli/reverseEngineeringCommands.js'; import { RizinDebugSessionManager } from '../../../src/rizin/RizinDebugSessionManager.js'; import { AnalysisOutputError } from '../../../src/domain/analysisErrorCore.js'; import { createEvidence } from '../../../src/domain/evidence.js'; import { ok, err } from '../../../src/domain/result.js'; it('observes the registered Rizin command after successful fallback cleanup', async () => { const saved = process.exitCode; const evidence = createEvidence(undefined, {id:'rizin.debugger',name:'Rizin debugger',version:null}, { predicateType:'rea.debugger.command-observation', operation:'rizin_debug_command', parameters:{command:'dr'}, result:{output:'fixture',completion_status:'complete'}, rawResult:{output:'fixture'} }); try { for (const initialFailure of [false, true]) { process.exitCode = 0; vi.spyOn(RizinDebugSessionManager.prototype,'start').mockResolvedValue(ok({session_id:'fixture-session',backend:null})); vi.spyOn(RizinDebugSessionManager.prototype,'execute').mockResolvedValue(ok({evidence,result:{command:'dr',output:'fixture',output_scope:'command_and_interleaved_session_output',output_truncated:false,completion_status:'complete',backend:null}})); const failure = new AnalysisOutputError('close_rizin_debug_session','synthetic incomplete first stop',{cleanup:{reason:'synthetic stop pending',resources:['fixture-session']}}); vi.spyOn(RizinDebugSessionManager.prototype,'close').mockResolvedValue(initialFailure ? err(failure) : ok({target_state:'unknown'})); const fallback = vi.spyOn(RizinDebugSessionManager.prototype,'closeAll').mockImplementation(async()=> { failure.resolveCleanup(); }); let run: any; const cli: any = {command(name:string, definition:any) { if(name==='debug-with-rizin') run=definition.run; return cli; }}; registerReverseEngineeringCommands(cli,pino({enabled:false}),{}); const result = await run({args:{path:'fixture.bin',command:'dr'},options:{inferiorTty:'/dev/pts/7'}}); expect(result).toBe(evidence); expect(fallback).toHaveBeenCalledOnce(); expect(failure.cleanupIncomplete).toBe(false); console.log(JSON.stringify({firstClose:initialFailure?'incomplete':'complete',fallback:'resolved',returnedOriginalEvidence:result===evidence,exitCode:process.exitCode})); expect(process.exitCode).toBe(initialFailure ? 1 : 0); vi.restoreAllMocks(); } } finally { vi.restoreAllMocks(); process.exitCode=saved; } }); ``` Run with a disposable HOME: ```sh WORK=$(mktemp -d) HOME="$WORK" node node_modules/vitest/vitest.mjs run --project cli-boundary tests/boundary/cli/rizinCleanupProbe.test.ts --maxWorkers=1 --no-file-parallelism ``` One executed test passed, characterizing both cases once. Remove the probe and its disposable HOME afterward. Assertions observe the current defect rather than assert the proposed correction. ## Expected behavior For a successful command with confirmed fallback cleanup and no independent operation failure or cancellation, retain success status. The [CLI status table](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/docs/cli.md#L341-L345) assigns `0` to completed operations and `1` to operations that could not complete. The [lifecycle contract](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/docs/tool-design.md#L119-L135) resolves current cleanup uncertainty after confirmed release while retaining completed observations and historical diagnostics. The [GDB one-shot handler](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/cli/reverseEngineeringCommands.ts#L238-L282) already clears its cleanup flag after successful fallback. ## Impact and scope The demonstrated effect is a misleading failure status despite returned command Evidence and resolved fallback cleanup. Automation using status alone can treat a completed invocation as failed. No Evidence loss or remaining resource was demonstrated. Real-provider frequency and a general caller-side workaround are NOT_ENOUGH_INFO; the probe establishes the CLI adapter boundary only. ## REA version Source `3537e58c1ab29309f06eabf50bc52adba9817a0c`, package manifest 6.7.0. Relevant source is unchanged from published stable tag `rea-agents-6.7.0`; the published package and stable runtime were not executed. ## Runtime and platform Node.js v26.12.0; macOS 27.0.1; arm64; locked Vitest 5.0.3. This run did not use the repository's Node 24.18.0 pin. ## Target and artifact context Synthetic successful command Evidence and mocked manager lifecycle outcomes. No artifact was opened and no target or provider executable was launched. The example TTY and artifact names are unused mock inputs. ## Interface CLI ## Earliest failure stage Cleanup, cancellation, or lifecycle ## Relevant output or evidence Observed in the focused test: ```json {"firstClose":"complete","fallback":"resolved","returnedOriginalEvidence":true,"exitCode":0} {"firstClose":"incomplete","fallback":"resolved","returnedOriginalEvidence":true,"exitCode":1} ``` The [Rizin handler](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/cli/reverseEngineeringCommands.ts#L335-L363) sets failure status before fallback and does not revise it after fallback succeeds. SOURCE-ONLY reachability corroboration: [close](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/rizin/RizinDebugSessionManager.ts#L922-L941) retains a session after an incomplete stop. [closeAll](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/rizin/RizinDebugSessionManager.ts#L962-L973) retries that session through the [shared cleanup owner](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/process/ownedDebuggerSession.ts#L278-L311). The [stop wrapper](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/rizin/RizinDebugSessionManager.ts#L1115-L1136) resolves retained cleanup errors after a confirmed stop. A bounded [stream settlement wait](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/process/ownedDebuggerSession.ts#L145-L181) can be incomplete before later settlement. No natural timing incident was reproduced. ## Typed outcome or recovery expected Keep the original successful Evidence, preserve historical cleanup diagnostics, and report success when final cleanup is confirmed. Preserve nonzero status for independently failed operations, cancellation or unresolved cleanup. ## Analysis or suspected cause The CLI writes `process.exitCode = 1` immediately on the first close error. Successful fallback resolves cleanup ownership but cannot revise this separate status assignment. The adjacent GDB handler defers the status decision until fallback finishes. ## Additional context Searched the supplied complete open-and-closed issue snapshot. Related [lifecycle audit #4227](https://github.com/morluto/rea/issues/4227), [CDP cleanup marker issue #4467](https://github.com/morluto/rea/issues/4467), and [web signal exit-status issue #1126](https://github.com/morluto/rea/issues/1126) concern different triggers and outcomes. No matching report found. Real Rizin/Linux execution, packaged subprocess exit and the frequency of transient cleanup failure remain NOT_ENOUGH_INFO.

bsakaguchi closed this issue

2026-10-31 00:36:57 +00:00

bsakaguchi commented

2026-10-31 00:36:57 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 19, 2026):

Agreed — this is a stale exit-status bug: the initial close failure sets process.exitCode, and the fallback success path never resets it. The exit code should reflect the final outcome, so either the fallback-success branch explicitly clears the code, or the handler only assigns a non-zero code when the whole cleanup ends in failure.

Worth adding the same regression you already have in the probe: assert exitCode === 0 after a failed initial close followed by successful fallback cleanup, since this kind of state leaks easily across handler refactors.

@XinwangTao commented on GitHub (Oct 19, 2026): Agreed — this is a stale exit-status bug: the initial close failure sets process.exitCode, and the fallback success path never resets it. The exit code should reflect the final outcome, so either the fallback-success branch explicitly clears the code, or the handler only assigns a non-zero code when the whole cleanup ends in failure. Worth adding the same regression you already have in the probe: assert exitCode === 0 after a failed initial close followed by successful fallback cleanup, since this kind of state leaks easily across handler refactors.

bsakaguchi commented

2026-10-31 00:36:58 +00:00

Author

Owner

@akushonkamen commented on GitHub (Oct 19, 2026):

I'd like to take this one — the maintainer-confirmed root cause (initial close failure sets process.exitCode = 1 and the fallback closeAll() success path never resets it) matches what I see in src/cli/reverseEngineeringCommands.ts. Will follow up with a minimal fix aligning the rizin path with the existing GDB cleanup pattern, plus a regression test, shortly.

@akushonkamen commented on GitHub (Oct 19, 2026): I'd like to take this one — the maintainer-confirmed root cause (initial close failure sets `process.exitCode = 1` and the fallback `closeAll()` success path never resets it) matches what I see in `src/cli/reverseEngineeringCommands.ts`. Will follow up with a minimal fix aligning the rizin path with the existing GDB cleanup pattern, plus a regression test, shortly.

bsakaguchi commented

2026-10-31 00:36:58 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 20, 2026):

Nice — aligning the rizin path with the GDB pattern is the right call, and the Draft #5247 scope (minimal fix + regression) matches what this issue needs. One implementation note: prefer the deferred-assignment approach (only set a non-zero exitCode after the whole cleanup settles) over clearing the code after fallback — it keeps the status decision in one place and avoids the same leak if another fallback path is added later. The regression asserting exitCode === 0 after failed-first-close + successful fallback will guard that.

@XinwangTao commented on GitHub (Oct 20, 2026): Nice — aligning the rizin path with the GDB pattern is the right call, and the Draft #5247 scope (minimal fix + regression) matches what this issue needs. One implementation note: prefer the deferred-assignment approach (only set a non-zero exitCode after the whole cleanup settles) over clearing the code after fallback — it keeps the status decision in one place and avoids the same leak if another fallback path is added later. The regression asserting exitCode === 0 after failed-first-close + successful fallback will guard that.

bsakaguchi commented

2026-10-31 00:36:58 +00:00

Author

Owner

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

Checked the reported fix on main at d87e73f2db.

The Rizin one-shot handler now logs an initial per-session close error without assigning failure exit status. The retained-owner fallback closeAll is authoritative: only its rejection sets exitCode 1. Thus a successful command plus confirmed fallback cleanup retains success, without clearing independent failures or cancellation.

Implementation: reverseEngineeringCommands.ts, reverseEngineeringCommands.test.ts.

The relevant regression checks in reverseEngineeringCommands.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. The Rizin one-shot handler now logs an initial per-session close error without assigning failure exit status. The retained-owner fallback closeAll is authoritative: only its rejection sets exitCode 1. Thus a successful command plus confirmed fallback cleanup retains success, without clearing independent failures or cancellation. Implementation: [reverseEngineeringCommands.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/cli/reverseEngineeringCommands.ts#L446), [reverseEngineeringCommands.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/cli/reverseEngineeringCommands.test.ts#L387). The relevant regression checks in [reverseEngineeringCommands.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/cli/reverseEngineeringCommands.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 01:08:12 +00:00

[PR #3144] [CLOSED] fix(javascript): admit candidate endpoint interpretations at the worker boundary #5641

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#3128

No description provided.

Rows
Columns