morluto/rea

Fork 0

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

[PR #1845] [MERGED] fix(ida): retain private input after ambiguous open #4853

New issue

Closed

opened 2026-10-31 00:56:02 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:56:02 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1845
Author: @jhuang-tw
Created: 10/11/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/ida-reaped-worker-cleanup-1710

📝 Commits (1)

08694c7 fix(ida): retain private input after unverified open

📊 Changes

4 files changed (+103 additions, -18 deletions)

View changed files

📝 docs/ida-provider.md (+2 -0)
📝 src/ida/IdaProtocolValues.ts (+8 -0)
📝 src/ida/IdaSessionClient.test.ts (+80 -18)
📝 src/ida/IdaSessionClient.ts (+13 -0)

📄 Description

Summary

Guard private headless IDA workspace cleanup when idb_open returned, but its actual owned-worker identity was never verified. This is a standalone REA admission/ownership fix using the existing upstream MCP protocol. It does not depend on unmerged ida-pro-mcp #551.

Problem: REA marks #openCompleted = true as soon as an idb_open tool reply arrives. A positive or malformed reply can then fail session/path validation, or the first inventory may not show the private worker. If a later ordinary idb_list is empty, the old close() path can delete the private input despite never verifying whether the original worker is still using it. An empty registry is not a process-termination certificate.

Change: After an attempted and completed open whose reply is not an explicit producer error, refuse private-workspace cleanup if no active, owned session for the exact private session ID and input path can be found. Return cleanup_incomplete and retain the connection/workspace. A later close() can retry; once a matching owned worker is actually observed, ordinary idb_close({database, save:false}) and existing post-close inventory checking apply. Track explicit idb_open rejection separately so existing completed rejection without a worker retains its prior cleanup behavior. No close is sent for an untrusted session ID.

Why this is independent

The original Draft of PR #1845 consumed an unmerged upstream release_id / strict-discovery protocol and was too dependent on upstream acceptance. REA maintainers have already merged commit 12fa8fd for Issue #1710 and closed that issue. This revised PR supersedes the original Draft scope rather than duplicating the existing reaped-worker recovery.

There is no new release_id, expected_input_path, strict_discovery, dependency, external version pin, worker-kill shortcut, or changes to Ghidra, Hopper, core workflow or other providers. Only four files change:

src/ida/IdaProtocolValues.ts: parse the explicit completed open-error response without confusing it with a successful open.
src/ida/IdaSessionClient.ts: keep the private owned resource when open identity is unresolved.
src/ida/IdaSessionClient.test.ts: reproductions and retry/regression coverage.
docs/ida-provider.md: spell out this cleanup ownership boundary.

Reproduction and regression evidence

Three new source-owned, production-adapter connection-seam cases failed against unchanged REA main before the fix, because close() succeeded and removed the private workspace when the first inventory was empty:

A success reply with the requested private session/input followed by an incomplete inventory.
A success reply with foreign session and input identity.
A malformed success reply lacking required input identity.

With this patch, all three PASS: cleanup remains incomplete and the private input is retained; once the fixture subsequently shows the actual requested owned worker, the next close sends only save:false to that exact private session and removes the workspace after verification. A fourth test verifies that a completed explicit producer rejection preserves the existing no-worker cleanup path.

Verification on the latest locally rebased candidate (commit 08694c7937f04f58dca5ce4315ab0929115641aa, based on upstream main 94153f1 at test time):

npm run check: PASS (TypeScript, lint, format, dependency checks, architecture guards, metadata/knip).
npm run docs:check: PASS.
New focused regression selection: 4 passed.
Five-file focused IDA selection: 69 passed, 5 failed locally on Windows; all five existing negative-cleanup fixture tests failed during teardown with EBUSY deleting deliberately retained private-DACL workspaces. They are not claimed passing and were not bypassed.
npm run verify:windows-native:artifact: PASS, using the CI-built native control from the earlier source-identical candidate; no DACL/security bypass.
Final-head GitHub Actions: PASS. CI run 38272664838 completed successfully on 08694c7; all 6/6 test shards passed, coverage/timing aggregation passed, Static checks, Docs Generation, Build package, Windows native artifact build, and Linux native package baseline passed. Other unneeded CI lanes were skipped by repository change classification.

Boundaries and rollout

This PR does not claim to solve all of #1710. Specifically, the existing legacy Session not found + empty idb_list recovery on an already admitted Worker is unchanged; this PR cannot prove termination from that inventory. An explicit idb_open error remains a producer assertion, not proof about arbitrary still-running processes. Supervisor restarts, orphaned children and global process isolation remain outside this patch.

Licensed real IDA Engine tests have not been run for this independent change. The new tests validate the adapter's existing MCP boundary and safe retry behavior, not end-to-end licensed-engine release. Maintainer review is requested before merging.

History transparency: the former #551-dependent candidate was saved locally at backup/rea-1845-551-20261012 (01cee40b51e2db1130fec4e157541bd2631447bb) before updating the same PR branch with an exact --force-with-lease. No other PR or repository main was modified.

_{🔄 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/1845 **Author:** [@jhuang-tw](https://github.com/jhuang-tw) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/ida-reaped-worker-cleanup-1710` --- ### 📝 Commits (1) - [`08694c7`](https://github.com/morluto/rea/commit/08694c7937f04f58dca5ce4315ab0929115641aa) fix(ida): retain private input after unverified open ### 📊 Changes **4 files changed** (+103 additions, -18 deletions) <details> <summary>View changed files</summary> 📝 `docs/ida-provider.md` (+2 -0) 📝 `src/ida/IdaProtocolValues.ts` (+8 -0) 📝 `src/ida/IdaSessionClient.test.ts` (+80 -18) 📝 `src/ida/IdaSessionClient.ts` (+13 -0) </details> ### 📄 Description ## Summary Guard private headless IDA workspace cleanup when `idb_open` **returned**, but its actual owned-worker identity was never verified. This is a **standalone REA admission/ownership fix** using the existing upstream MCP protocol. It does **not** depend on unmerged [ida-pro-mcp #551](https://github.com/mrexodia/ida-pro-mcp/pull/551). **Problem:** REA marks `#openCompleted = true` as soon as an `idb_open` tool reply arrives. A positive or malformed reply can then fail session/path validation, or the first inventory may not show the private worker. If a later ordinary `idb_list` is empty, the old `close()` path can delete the private input despite never verifying whether the original worker is still using it. An empty registry is not a process-termination certificate. **Change:** After an attempted and completed open whose reply is **not an explicit producer error**, refuse private-workspace cleanup if no active, owned session for the exact private session ID and input path can be found. Return `cleanup_incomplete` and retain the connection/workspace. A later `close()` can retry; once a matching owned worker is actually observed, ordinary `idb_close({database, save:false})` and existing post-close inventory checking apply. Track explicit `idb_open` rejection separately so existing completed rejection without a worker retains its prior cleanup behavior. No close is sent for an untrusted session ID. ## Why this is independent The original Draft of PR #1845 consumed an unmerged upstream `release_id` / strict-discovery protocol and was too dependent on upstream acceptance. REA maintainers have already merged commit [12fa8fd](https://github.com/morluto/rea/commit/12fa8fd9f986dbd4ff58165c3ec71a46dc8a8a3b) for Issue #1710 and closed that issue. This revised PR **supersedes the original Draft scope** rather than duplicating the existing reaped-worker recovery. There is **no new `release_id`, `expected_input_path`, `strict_discovery`, dependency, external version pin, worker-kill shortcut, or changes to Ghidra, Hopper, core workflow or other providers**. Only four files change: - `src/ida/IdaProtocolValues.ts`: parse the explicit completed open-error response without confusing it with a successful open. - `src/ida/IdaSessionClient.ts`: keep the private owned resource when open identity is unresolved. - `src/ida/IdaSessionClient.test.ts`: reproductions and retry/regression coverage. - `docs/ida-provider.md`: spell out this cleanup ownership boundary. ## Reproduction and regression evidence Three new source-owned, production-adapter connection-seam cases **failed against unchanged REA main before the fix**, because `close()` succeeded and removed the private workspace when the first inventory was empty: 1. A success reply with the requested private session/input followed by an incomplete inventory. 2. A success reply with foreign session and input identity. 3. A malformed success reply lacking required input identity. With this patch, all three **PASS**: cleanup remains incomplete and the private input is retained; once the fixture subsequently shows the actual requested **owned** worker, the next close sends only `save:false` to that exact private session and removes the workspace after verification. A fourth test verifies that a completed explicit producer rejection preserves the existing no-worker cleanup path. Verification on the **latest locally rebased** candidate (commit `08694c7937f04f58dca5ce4315ab0929115641aa`, based on upstream main `94153f1` at test time): - `npm run check`: **PASS** (TypeScript, lint, format, dependency checks, architecture guards, metadata/knip). - `npm run docs:check`: **PASS**. - New focused regression selection: **4 passed**. - Five-file focused IDA selection: **69 passed, 5 failed** locally on Windows; all five existing negative-cleanup fixture tests failed during teardown with `EBUSY` deleting deliberately retained private-DACL workspaces. They are **not claimed passing** and were not bypassed. - `npm run verify:windows-native:artifact`: **PASS**, using the CI-built native control from the earlier source-identical candidate; no DACL/security bypass. - **Final-head GitHub Actions: PASS.** [CI run 38272664838](https://github.com/morluto/rea/actions/runs/38272664838) **completed successfully** on `08694c7`; all 6/6 test shards passed, coverage/timing aggregation passed, Static checks, Docs Generation, Build package, Windows native artifact build, and Linux native package baseline passed. Other unneeded CI lanes were skipped by repository change classification. ## Boundaries and rollout This PR **does not claim to solve all of #1710**. Specifically, the existing legacy `Session not found` + empty `idb_list` recovery on an **already admitted** Worker is unchanged; this PR cannot prove termination from that inventory. An explicit `idb_open` error remains a producer assertion, not proof about arbitrary still-running processes. Supervisor restarts, orphaned children and global process isolation remain outside this patch. Licensed real IDA Engine tests have **not** been run for this independent change. The new tests validate the adapter's existing MCP boundary and safe retry behavior, not end-to-end licensed-engine release. Maintainer review is requested before merging. **History transparency:** the former #551-dependent candidate was saved locally at `backup/rea-1845-551-20261012` (`01cee40b51e2db1130fec4e157541bd2631447bb`) before updating the same PR branch with an exact `--force-with-lease`. No other PR or repository main was modified. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:56:02 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:14:32 +00:00

[PR #4863] [CLOSED] fix(javascript): preserve ambient effects in Map endpoint proofs #6448

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

No description provided.

Rows
Columns