morluto/rea

Fork 0

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

[PR #1960] [CLOSED] fix(reverse)!: retain cleanup and session ownership for local inspection #4935

New issue

Closed

opened 2026-10-31 00:57:08 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1960
Author: @dandgabr
Created: 10/11/2026
Status: ❌ Closed

Base: main ← Head: codex/pr1279-local-inspection

📝 Commits (10+)

c6bea2e feat(reverse): add local inspection commands
6fee7c1 fix(reverse): retain evidence across interrupted commands
6470387 fix(reverse): retain evidence for interrupted providers
92142b7 fix(reverse): address inspection review findings
4b3a668 fix(reverse): address remaining PR 1960 findings
56554f3 fix(reverse): account for unrestricted provider execution
25c217c Merge remote-tracking branch 'refs/remotes/upstream/main' into codex/pr1279-local-inspection
5ded4a7 fix(reverse): preserve admission and output provenance
b873324 Merge remote-tracking branch 'refs/remotes/morluto/main' into codex/pr1279-local-inspection
53bfd05 merge: update local inspection fixes with current main

📊 Changes

59 files changed (+4383 additions, -478 deletions)

View changed files

📝 .jscpd.json (+1 -1)
📝 docs/reverse-engineering-tools.md (+28 -0)
📝 scripts/lib/reverse-tools-mutation-targets.mjs (+13 -10)
📝 src/application/binary/BinarySession.ts (+155 -63)
📝 src/application/binary/BinarySessionLiveness.test.ts (+43 -0)
📝 src/application/binary/BinarySessionOpen.ts (+5 -0)
📝 src/application/binary/BinarySessionPort.ts (+15 -23)
📝 src/application/reverse/ReverseEngineeringCommandPort.ts (+20 -2)
📝 src/application/reverse/ReverseEngineeringService.ts (+267 -45)
📝 src/browser/WebBundleAnalysisWorker.test.ts (+3 -2)
📝 src/cli/reverseEngineeringCommands.ts (+56 -9)
📝 src/cliJsonPaths.test.ts (+3 -1)
📝 src/composition/resolveWindowsProviderExecutable.ts (+359 -13)
📝 src/composition/reverseEngineering.ts (+68 -23)
📝 src/contracts/reverseEngineeringToolContracts.ts (+1 -1)
📝 src/domain/analysisErrorBase.ts (+1 -0)
📝 src/domain/analysisErrorCleanup.test.ts (+13 -0)
📝 src/domain/analysisView/analysisView.ts (+2 -2)
📝 src/domain/analysisView/commandObservationView.ts (+5 -1)
📝 src/domain/commandObservation.ts (+8 -0)

...and 39 more files

📄 Description

Problem and outcome

Local inspection commands already exist on main; this PR fixes cleanup ownership, session ordering, provider lookup, and Evidence preservation around them. Incomplete writers remain owned and block replacement work until their actual owner confirms cleanup.

Changes

Keep retained process owners through session close and repeated CLI commands; retry cleanup before admitting another command, and reject overlapping CLI runs with retryable capacity guidance.
Settle reverse cleanup inside the admitted close transition before deciding writer completion; preserve independent session/snapshot failures, snapshot receipts, and every unresolved session/process/reverse cleanup owner.
Resolve Windows executables asynchronously across the complete selected PATH, with cancellation and a startup deadline. Refuse required token sweeps on hosts without an ownership baseline, with actionable guidance.
Preserve snapshots when Rizin preparation rejects missing/non-regular input; invalidate immediately before attempted dispatch. Retained writers still invalidate and retire analysis. Fixed analysis sources withhold mutating Rizin because they cannot refresh the binding.
Preserve direct ownership-preparation remediation, caller-selected commands, absolute artifact paths, and one authoritative output representation. Keep open/close ordering and their cleanup failures attributable.
Select all explicitly owned cancellation checkpoints in the mutation configuration while preserving missing-target and singular-ambiguity failures.

Validation performed

Pinned Node 24.18.0 / npm 11.16.0; merged upstream/main at 9af0b6113 while preserving branch history.
npm run test:focused -- <17 affected files>: 247 passed, 2 platform skips. Includes CLI lifecycle/demangle budgets, public MCP/session workflows, process supervision, service and lookup regressions, and mutation-configuration validation.
Additional retained-client mutation-barrier regression: 12/12 in binarySessionCleanupRetry.test.ts; removing the owner retry guard makes the new test fail, then restoring it passes.
npm run check:fast, npm run check, and npm run docs:check: passed.
Follow-up 66670714b, 9be820125, and a9db1775a: 104/104 tests passed across seven affected files; check:fast and check passed again. RED/GREEN covers stale cleanup/timeout close refusals, CLI retry advice, mixed owners, and typed cleanup throws/rejections. A synchronous-throw mutation failed on duplicate retry, then passed after restoration. Snapshot receipt/refusal and queued-open ordering controls passed; independent adapter review passed 17 additional owner-combination cases.
Real Linux GNU objdump 2.46.1 and Rizin 0.7.4: public CLI and stdio MCP inspections of a self-owned compiled ELF, four successful Evidence results with complete output, raw_result: null, and unchanged pre/post artifact digests; MCP close succeeded.

Not verified: Windows/macOS/Android runtime, real Ghidra, complete coverage/mutation campaigns, or performance/token/CPU/process-family-memory benchmarks. Simulated ownership/probe cases establish boundary behavior, not those host runtimes. CI for the updated head remains separately observable.

Compatibility and scope

Breaking: embedded createServer({ kind: "fixed", analysis }) servers no longer expose execute_rizin_command; use a session-backed source to provide mutation retirement/invalidation. Android/Termux hosts without ownership baselines refuse these one-shot providers rather than silently omit descendant cleanup.

No new execution authority is added. The maintainer scope/authority decision in #3656 remains open and is required before integration; this PR does not establish that approval. GDB, persistent Rizin, and Cutter feature work is outside this correction.

Author readiness

Human owner: @dandgabr

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

Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
Required checks pass for the final head.

_{🔄 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/1960 **Author:** [@dandgabr](https://github.com/dandgabr) **Created:** 10/11/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `codex/pr1279-local-inspection` --- ### 📝 Commits (10+) - [`c6bea2e`](https://github.com/morluto/rea/commit/c6bea2e2b0c8000dff3ff2cc0668869273de00cf) feat(reverse): add local inspection commands - [`6fee7c1`](https://github.com/morluto/rea/commit/6fee7c12a3da3561d5d39710a21b0a6bf5fe00a0) fix(reverse): retain evidence across interrupted commands - [`6470387`](https://github.com/morluto/rea/commit/64703879f38554c2d3c456a0148c227cbee4c8c9) fix(reverse): retain evidence for interrupted providers - [`92142b7`](https://github.com/morluto/rea/commit/92142b74a1aec79327281c4d91c31f15699a4425) fix(reverse): address inspection review findings - [`4b3a668`](https://github.com/morluto/rea/commit/4b3a6683b86d11235e68d8c2d47a988b9c33e8f8) fix(reverse): address remaining PR 1960 findings - [`56554f3`](https://github.com/morluto/rea/commit/56554f3cbca4c9e3a615abd2ac2a4f21321698fb) fix(reverse): account for unrestricted provider execution - [`25c217c`](https://github.com/morluto/rea/commit/25c217c9c6306d99c1e57020e26702d120391d75) Merge remote-tracking branch 'refs/remotes/upstream/main' into codex/pr1279-local-inspection - [`5ded4a7`](https://github.com/morluto/rea/commit/5ded4a78a8f67bcb6238dd0151fab41d28059217) fix(reverse): preserve admission and output provenance - [`b873324`](https://github.com/morluto/rea/commit/b87332435e9d079aed2fcad54fbac580b0eb80ba) Merge remote-tracking branch 'refs/remotes/morluto/main' into codex/pr1279-local-inspection - [`53bfd05`](https://github.com/morluto/rea/commit/53bfd05ca31ee84f44ce233f69fe43e0270ebbaa) merge: update local inspection fixes with current main ### 📊 Changes **59 files changed** (+4383 additions, -478 deletions) <details> <summary>View changed files</summary> 📝 `.jscpd.json` (+1 -1) 📝 `docs/reverse-engineering-tools.md` (+28 -0) 📝 `scripts/lib/reverse-tools-mutation-targets.mjs` (+13 -10) 📝 `src/application/binary/BinarySession.ts` (+155 -63) 📝 `src/application/binary/BinarySessionLiveness.test.ts` (+43 -0) 📝 `src/application/binary/BinarySessionOpen.ts` (+5 -0) 📝 `src/application/binary/BinarySessionPort.ts` (+15 -23) 📝 `src/application/reverse/ReverseEngineeringCommandPort.ts` (+20 -2) 📝 `src/application/reverse/ReverseEngineeringService.ts` (+267 -45) 📝 `src/browser/WebBundleAnalysisWorker.test.ts` (+3 -2) 📝 `src/cli/reverseEngineeringCommands.ts` (+56 -9) 📝 `src/cliJsonPaths.test.ts` (+3 -1) 📝 `src/composition/resolveWindowsProviderExecutable.ts` (+359 -13) 📝 `src/composition/reverseEngineering.ts` (+68 -23) 📝 `src/contracts/reverseEngineeringToolContracts.ts` (+1 -1) 📝 `src/domain/analysisErrorBase.ts` (+1 -0) 📝 `src/domain/analysisErrorCleanup.test.ts` (+13 -0) 📝 `src/domain/analysisView/analysisView.ts` (+2 -2) 📝 `src/domain/analysisView/commandObservationView.ts` (+5 -1) 📝 `src/domain/commandObservation.ts` (+8 -0) _...and 39 more files_ </details> ### 📄 Description ## Problem and outcome Local inspection commands already exist on `main`; this PR fixes cleanup ownership, session ordering, provider lookup, and Evidence preservation around them. Incomplete writers remain owned and block replacement work until their actual owner confirms cleanup. ## Changes - Keep retained process owners through session close and repeated CLI commands; retry cleanup before admitting another command, and reject overlapping CLI runs with retryable capacity guidance. - Settle reverse cleanup inside the admitted close transition before deciding writer completion; preserve independent session/snapshot failures, snapshot receipts, and every unresolved session/process/reverse cleanup owner. - Resolve Windows executables asynchronously across the complete selected `PATH`, with cancellation and a startup deadline. Refuse required token sweeps on hosts without an ownership baseline, with actionable guidance. - Preserve snapshots when Rizin preparation rejects missing/non-regular input; invalidate immediately before attempted dispatch. Retained writers still invalidate and retire analysis. Fixed analysis sources withhold mutating Rizin because they cannot refresh the binding. - Preserve direct ownership-preparation remediation, caller-selected commands, absolute artifact paths, and one authoritative output representation. Keep open/close ordering and their cleanup failures attributable. - Select all explicitly owned cancellation checkpoints in the mutation configuration while preserving missing-target and singular-ambiguity failures. ## Validation performed - Pinned Node `24.18.0` / npm `11.16.0`; merged `upstream/main` at `9af0b6113` while preserving branch history. - `npm run test:focused -- <17 affected files>`: **247 passed, 2 platform skips**. Includes CLI lifecycle/demangle budgets, public MCP/session workflows, process supervision, service and lookup regressions, and mutation-configuration validation. - Additional retained-client mutation-barrier regression: **12/12** in `binarySessionCleanupRetry.test.ts`; removing the owner retry guard makes the new test fail, then restoring it passes. - `npm run check:fast`, `npm run check`, and `npm run docs:check`: passed. - Follow-up `66670714b`, `9be820125`, and `a9db1775a`: **104/104 tests passed across seven affected files**; `check:fast` and `check` passed again. RED/GREEN covers stale cleanup/timeout close refusals, CLI retry advice, mixed owners, and typed cleanup throws/rejections. A synchronous-throw mutation failed on duplicate retry, then passed after restoration. Snapshot receipt/refusal and queued-open ordering controls passed; independent adapter review passed 17 additional owner-combination cases. - Real Linux GNU objdump `2.46.1` and Rizin `0.7.4`: public CLI and stdio MCP inspections of a self-owned compiled ELF, four successful Evidence results with complete output, `raw_result: null`, and unchanged pre/post artifact digests; MCP close succeeded. Not verified: Windows/macOS/Android runtime, real Ghidra, complete coverage/mutation campaigns, or performance/token/CPU/process-family-memory benchmarks. Simulated ownership/probe cases establish boundary behavior, not those host runtimes. CI for the updated head remains separately observable. ## Compatibility and scope **Breaking:** embedded `createServer({ kind: "fixed", analysis })` servers no longer expose `execute_rizin_command`; use a session-backed source to provide mutation retirement/invalidation. Android/Termux hosts without ownership baselines refuse these one-shot providers rather than silently omit descendant cleanup. No new execution authority is added. The maintainer scope/authority decision in [#3656](https://github.com/morluto/rea/issues/3656) remains open and is required before integration; this PR does not establish that approval. GDB, persistent Rizin, and Cutter feature work is outside this correction. ## Author readiness Human owner: @dandgabr - [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 - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] Required checks pass for the final head. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:42:16 +00:00

[PR #4935] [CLOSED] fix(setup): read client TOML integers losslessly #6680

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

No description provided.

Rows
Columns