morluto/rea

Fork 0

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

[PR #5247] [CLOSED] fix(cli): keep debug-with-rizin success status after confirmed fallback cleanup #6836

New issue

Closed

opened 2026-11-01 21:43:24 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:43:24 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5247
Author: @akushonkamen
Created: 10/19/2026
Status: ❌ Closed

Base: main ← Head: fix/5090-rizin-fallback-exit-code

📝 Commits (1)

420350c fix(cli): keep debug-with-rizin success status after confirmed fallback cleanup

📊 Changes

2 files changed (+186 additions, -16 deletions)

View changed files

📝 src/cli/reverseEngineeringCommands.ts (+33 -16)
📝 tests/boundary/cli/reverseEngineeringCommands.test.ts (+153 -0)

📄 Description

Summary

A debug-with-rizin one-shot session whose initial per-session close reported incomplete cleanup exited with status 1 even when the fallback closeAll() then completed the full cleanup, so automation reading only the status treated a completed invocation as failed. This aligns the Rizin handler with the adjacent GDB handler's already-correct cleanupFailed pattern.

Fixes #5090

Problem and expected behavior

Smallest trigger: run debug-with-rizin, the command succeeds, manager.close(sessionId) resolves with an incomplete-cleanup error, and manager.closeAll() then succeeds. Violated invariant: the CLI exit status must reflect the final outcome — 0 for a completed operation whose cleanup is confirmed, 1 for operations that could not complete (docs/cli.md status table; the lifecycle contract resolves cleanup uncertainty after confirmed release). The handler assigned process.exitCode = 1 immediately on the first close error and never revised it after successful fallback. The maintainer confirmed this root cause in #5090.

Change and scope

src/cli/reverseEngineeringCommands.ts, Rizin finally block only: mirror the GDB handler — track let cleanupFailed, set it when the initial close fails (now also catching a thrown close instead of letting it escape the finally), reset it to false when the fallback closeAll() succeeds, set it to true if closeAll() throws, and assign process.exitCode = 1 once at the end only if cleanup remained unresolved. Intentionally not included: any change to Evidence handling, logging content, GDB behavior, or manager internals.

Contribution scope: ordinary fix within existing boundaries — required maintainer scope decision: not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: CLI adapter (debug-with-rizin handler), cleanup/cancellation/lifecycle stage after command output is collected.
Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable.
CLI and MCP/tool-catalog contract: exit status only; command arguments, options, output, and log lines unchanged. Restores the status semantics already documented in docs/cli.md.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none; Evidence is returned unchanged.
Target selection, authority, upstream behavior, and cleanup details: cleanup bookkeeping only. Regression tests are fully mocked (RizinDebugSessionManager spies); no real Rizin process or target was executed, and the natural frequency of a transient close failure remains unknown (as reported in #5090).
Generated metadata, package, or installation impact: none.

Expected effects: the change alters only exit-status bookkeeping after the command has already run; the tests launch no process and perform no target interaction.

Evidence and regression coverage

Tests added or updated: three regression tests in tests/boundary/cli/reverseEngineeringCommands.test.ts (mocked manager, no real Rizin): incomplete initial close + successful fallback → Evidence returned, closeAll called once, cleanup uncertainty resolved, process.exitCode stays 0; successful initial close + successful fallback → stays 0; incomplete initial close + thrown fallback → stays 1 (failure preserved).
Base reproduction or other evidence: executed. Before the fix the new test failed with AssertionError: expected 1 to be +0 on this same worktree at base 3ea0d9489, matching the reporter's probe output {"firstClose":"incomplete","fallback":"resolved","returnedOriginalEvidence":true,"exitCode":1} from #5090. After the fix all 10 tests in the file pass. The regression test fails on the affected base revision by construction (it was run there first, red, then made green).
User-visible CLI/MCP output (if applicable): exit status of debug-with-rizin when fallback cleanup confirms release is now 0 instead of 1.
Remaining proof gaps: no real-Rizin/Linux execution and no packaged-CLI subprocess run; natural-frequency data remains NOT_ENOUGH_INFO per the issue.
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

timeout 120 npx vitest run tests/boundary/cli/reverseEngineeringCommands.test.ts --maxWorkers=1 --no-file-parallelism — before the fix: 1 failed (expected 1 to be +0 at the exit-status assertion), 9 passed; after the fix: 10 passed.
timeout 300 npx vitest run tests/boundary/cli/reverseEngineeringCommands.test.ts tests/boundary/cli/gdbStartupCleanupCli.test.ts --maxWorkers=1 --no-file-parallelism — 18 passed (both files exercising this source; GDB path unaffected).
npx oxlint src/cli/reverseEngineeringCommands.ts tests/boundary/cli/reverseEngineeringCommands.test.ts — 0 errors, 2 warnings, both pre-existing on the unmodified clone (max-parameters on registerReverseEngineeringCommands).
npx tsc --noEmit — 4 errors, all openpgp module-resolution errors in files untouched by this change; reproduced identically on the clean base revision in this environment (node_modules predates the openpgp dependency added in 70cd4cbfb), so they are unrelated pre-existing environment issues.
timeout 600 npx vitest run --project cli-boundary — 86 files fail in this worktree because built-CLI fixture tests (tests/fixtures/cliJsonProcess.ts spawning scripts/rea.mjs) require dist/ build artifacts that are not built here; verified pre-existing by stashing this change and re-running tests/boundary/cli/accessibilityCaptureComparison.test.ts on the clean tree (7/7 fail with the same missing-build error). No in-process test failure was introduced.
CI note: the initial push failed the Validate author declaration/PR readiness gates because the PR was opened non-draft with the Author readiness section unfilled; AI agents must leave those declarations to the human owner per the PR template, so the PR was converted to draft and the gate now reports pending. A separate CI failure (Reverse tools quality → tests/boundary/mcp/reverseEngineeringEnvironmentMcp.test.ts > snapshots an explicit provider environment when the server is created, expected true not to be true at the isError assertion) was investigated and is a pre-existing flake: on this exact head commit the same test was run 4 times locally with 2 failures and 2 passes, and the identical failure occurs on main runs at the base commit; this PR's files (src/cli/reverseEngineeringCommands.ts, tests/boundary/cli/reverseEngineeringCommands.test.ts) are not imported by that test.

Compatibility, safety, and release

Breaking changes or migration steps: none; the only behavior change restores the documented success status for the fallback-cleanup-success case.
Real Hopper/Ghidra, browser, or OS coverage: none; mocked CLI boundary tests only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none.

Author readiness

Human owner: @akushonkamen

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.

_{🔄 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/5247 **Author:** [@akushonkamen](https://github.com/akushonkamen) **Created:** 10/19/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/5090-rizin-fallback-exit-code` --- ### 📝 Commits (1) - [`420350c`](https://github.com/morluto/rea/commit/420350cb9b4f39e6077adac8fb36070970fd2089) fix(cli): keep debug-with-rizin success status after confirmed fallback cleanup ### 📊 Changes **2 files changed** (+186 additions, -16 deletions) <details> <summary>View changed files</summary> 📝 `src/cli/reverseEngineeringCommands.ts` (+33 -16) 📝 `tests/boundary/cli/reverseEngineeringCommands.test.ts` (+153 -0) </details> ### 📄 Description ## Summary A `debug-with-rizin` one-shot session whose initial per-session `close` reported incomplete cleanup exited with status `1` even when the fallback `closeAll()` then completed the full cleanup, so automation reading only the status treated a completed invocation as failed. This aligns the Rizin handler with the adjacent GDB handler's already-correct `cleanupFailed` pattern. Fixes #5090 ## Problem and expected behavior Smallest trigger: run `debug-with-rizin`, the command succeeds, `manager.close(sessionId)` resolves with an incomplete-cleanup error, and `manager.closeAll()` then succeeds. Violated invariant: the CLI exit status must reflect the final outcome — `0` for a completed operation whose cleanup is confirmed, `1` for operations that could not complete (docs/cli.md status table; the lifecycle contract resolves cleanup uncertainty after confirmed release). The handler assigned `process.exitCode = 1` immediately on the first close error and never revised it after successful fallback. The maintainer confirmed this root cause in #5090. ## Change and scope `src/cli/reverseEngineeringCommands.ts`, Rizin `finally` block only: mirror the GDB handler — track `let cleanupFailed`, set it when the initial `close` fails (now also catching a thrown `close` instead of letting it escape the `finally`), reset it to `false` when the fallback `closeAll()` succeeds, set it to `true` if `closeAll()` throws, and assign `process.exitCode = 1` once at the end only if cleanup remained unresolved. Intentionally not included: any change to Evidence handling, logging content, GDB behavior, or manager internals. Contribution scope: ordinary fix within existing boundaries — required maintainer scope decision: not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: CLI adapter (`debug-with-rizin` handler), cleanup/cancellation/lifecycle stage after command output is collected. - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: exit status only; command arguments, options, output, and log lines unchanged. Restores the status semantics already documented in docs/cli.md. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none; Evidence is returned unchanged. - Target selection, authority, upstream behavior, and cleanup details: cleanup bookkeeping only. Regression tests are fully mocked (`RizinDebugSessionManager` spies); no real Rizin process or target was executed, and the natural frequency of a transient close failure remains unknown (as reported in #5090). - Generated metadata, package, or installation impact: none. Expected effects: the change alters only exit-status bookkeeping after the command has already run; the tests launch no process and perform no target interaction. ## Evidence and regression coverage - Tests added or updated: three regression tests in `tests/boundary/cli/reverseEngineeringCommands.test.ts` (mocked manager, no real Rizin): incomplete initial close + successful fallback → Evidence returned, `closeAll` called once, cleanup uncertainty resolved, `process.exitCode` stays `0`; successful initial close + successful fallback → stays `0`; incomplete initial close + thrown fallback → stays `1` (failure preserved). - Base reproduction or other evidence: executed. Before the fix the new test failed with `AssertionError: expected 1 to be +0` on this same worktree at base `3ea0d9489`, matching the reporter's probe output `{"firstClose":"incomplete","fallback":"resolved","returnedOriginalEvidence":true,"exitCode":1}` from #5090. After the fix all 10 tests in the file pass. The regression test fails on the affected base revision by construction (it was run there first, red, then made green). - User-visible CLI/MCP output (if applicable): exit status of `debug-with-rizin` when fallback cleanup confirms release is now `0` instead of `1`. - Remaining proof gaps: no real-Rizin/Linux execution and no packaged-CLI subprocess run; natural-frequency data remains NOT_ENOUGH_INFO per the issue. - [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 - `timeout 120 npx vitest run tests/boundary/cli/reverseEngineeringCommands.test.ts --maxWorkers=1 --no-file-parallelism` — before the fix: 1 failed (`expected 1 to be +0` at the exit-status assertion), 9 passed; after the fix: 10 passed. - `timeout 300 npx vitest run tests/boundary/cli/reverseEngineeringCommands.test.ts tests/boundary/cli/gdbStartupCleanupCli.test.ts --maxWorkers=1 --no-file-parallelism` — 18 passed (both files exercising this source; GDB path unaffected). - `npx oxlint src/cli/reverseEngineeringCommands.ts tests/boundary/cli/reverseEngineeringCommands.test.ts` — 0 errors, 2 warnings, both pre-existing on the unmodified clone (`max-parameters` on `registerReverseEngineeringCommands`). - `npx tsc --noEmit` — 4 errors, all `openpgp` module-resolution errors in files untouched by this change; reproduced identically on the clean base revision in this environment (`node_modules` predates the `openpgp` dependency added in 70cd4cbfb), so they are unrelated pre-existing environment issues. - `timeout 600 npx vitest run --project cli-boundary` — 86 files fail in this worktree because built-CLI fixture tests (`tests/fixtures/cliJsonProcess.ts` spawning `scripts/rea.mjs`) require `dist/` build artifacts that are not built here; verified pre-existing by stashing this change and re-running `tests/boundary/cli/accessibilityCaptureComparison.test.ts` on the clean tree (7/7 fail with the same missing-build error). No in-process test failure was introduced. - CI note: the initial push failed the `Validate author declaration`/`PR readiness` gates because the PR was opened non-draft with the Author readiness section unfilled; AI agents must leave those declarations to the human owner per the PR template, so the PR was converted to draft and the gate now reports pending. A separate CI failure (`Reverse tools quality` → `tests/boundary/mcp/reverseEngineeringEnvironmentMcp.test.ts > snapshots an explicit provider environment when the server is created`, `expected true not to be true` at the `isError` assertion) was investigated and is a pre-existing flake: on this exact head commit the same test was run 4 times locally with 2 failures and 2 passes, and the identical failure occurs on `main` runs at the base commit; this PR's files (`src/cli/reverseEngineeringCommands.ts`, `tests/boundary/cli/reverseEngineeringCommands.test.ts`) are not imported by that test. ## Compatibility, safety, and release - Breaking changes or migration steps: none; the only behavior change restores the documented success status for the fallback-cleanup-success case. - Real Hopper/Ghidra, browser, or OS coverage: none; mocked CLI boundary tests only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none. ## Author readiness  Human owner: @akushonkamen - [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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:43:24 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns