[PR #2754] [MERGED] fix(session): preserve investigation records after failed target opens #5365

Closed
opened 2026-10-31 01:06:03 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2754
Author: @Adkid-Zephyr
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/session-preserve-records-on-open-failure


📝 Commits (2)

  • f603947 fix(session): preserve investigation records after failed target opens
  • 41c0ce5 chore(ci): merge current main verification fixes

📊 Changes

3 files changed (+192 additions, -4 deletions)

View changed files

📝 docs/mcp-contracts.md (+5 -3)
📝 src/application/binary/BinarySession.ts (+3 -1)
➕ tests/boundary/mcp/sessionEvidenceRecovery.test.ts (+184 -0)

📄 Description

Summary

Preserve the investigation's Evidence and Unknown history when open_binary fails to retire the previous provider. Both target switching and same-target restart currently turn a bundle with 4 records/2 revisions into an empty bundle. Fixes #2753.

Follow-up to the unresolved P1 finding on #1776; original finding credited there. Implemented and verified with OpenAI Codex assistance.

Problem and expected behavior

A provider cleanup failure is returned correctly, but BinarySession.#open also calls clearSessionRecords(). A subsequent export cannot recover the lost investigation. Target opens must preserve records; explicit close_binary remains the operation that clears them.

Change and scope

Use the existing clearSnapshot() owner to discard the retired provider's replay cache without clearing the investigation ledger. Keep the pending cleanup owner, unavailable target, retry gate, and explicit-close behavior. Clarify this lifetime in the MCP guide.

Contract and boundary impact

  • Semantic owner and earliest changed stage: BinarySession.#open, after failed retirement.
  • Contribution scope fit and linked maintainer decision: ordinary reliability fix within existing lifecycle boundaries; advance scope decision not applicable.
  • CLI and MCP/tool-catalog contract: no schema or catalog changes; previously returned Evidence references remain usable after a failed open.
  • Provider, bridge, target-format, or platform compatibility: provider-neutral; no adapter/protocol changes.
  • Evidence, artifact, provenance, or reconstruction contract: preserve exact Evidence, Unknown revisions and bundle metadata; invalidate only the retired provider cache.
  • Target selection, authority, upstream behavior, and cleanup details: existing selection and cleanup unchanged; replacement remains blocked until cleanup succeeds.
  • Generated metadata, package, or installation impact: none.

Expected effects exercised by the regression workflow (production adds no new target interaction):

  • Read existing files or captured data (test-owned fixtures).
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Production adds no target interaction. Tests also write temporary fixtures/exports and launch REA's JavaScript analysis worker; they never execute the analyzed JavaScript or a native target.

Evidence and regression coverage

  • Two public MCP boundary regressions exercise a target switch and same-target restart through the production provider composition, with controlled provider failure/health.

  • Exact final tests on main ab02f3c8d665ead35391c94db74cd0fb0cfed0ed: 2 failed, observing all 4 records and both Unknown revisions disappear. With the fix: 2 passed.

  • Check repeated failed cleanup, exact bundle preservation and file export, successful retry, fresh provider queries instead of stale cache replay, retained JavaScript Evidence views, and explicit close clearing.

  • User-visible failure remains cleanup_incomplete; get_evidence_bundle and export retain the original records instead of empty collections.

  • Proof gaps: provider cleanup/health are fixtures. No new real Hopper/Ghidra/IDA failure or native Windows/Linux run; no incidence-rate claim.

  • 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

Pinned Node 24.18.0/npm 11.16.0 on macOS arm64, final head f603947 rebased onto main 2995ec6:

  • npm run test:focused -- tests/boundary/mcp/sessionEvidenceRecovery.test.ts tests/boundary/mcp/sessionMcp.test.ts tests/boundary/mcp/retainedApplicationEvidenceMcp.test.ts tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts tests/composition/analysis-sessions/binarySession.test.ts src/application/binary/BinarySessionLiveness.test.ts tests/boundary/filesystem/directAnalysisSnapshotReplay.test.ts — 76 passed, 7 files.
  • npm run check — passed after shortening the new test helper; the initial run caught its function-length lint error.
  • npm run docs:check — passed, including 2 consistency tests.
  • npm run verify:test-discovery — all 1,055 files discovered once.
  • git diff --check — passed.

Full deterministic suite not run locally; remote CI is separate. verify:package passed while scouting the untouched baseline; packaging checks were not rerun for this lifecycle-only diff.

Compatibility, safety, and release

  • Breaking changes or migration steps: none; explicit close keeps its existing lifetime.
  • Real Hopper/Ghidra, browser, or OS coverage: macOS tests with actual static JavaScript analysis; native provider failure behavior unverified.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no security research or new authority; cleanup ownership and startup ordering remain covered.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved; no new capability needs a scope decision.
  • One focused outcome and a Conventional Commit title.
  • Related issue linked and prior finding credited.
  • Tests cover observable behavior and meaningful failure paths.
  • Owning documentation updated; schemas/generated metadata unchanged.
  • Representative MCP behavior and verification limits described.
  • Final diff checked for secrets, unrelated cleanup, and unsupported claims.

🔄 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/2754 **Author:** [@Adkid-Zephyr](https://github.com/Adkid-Zephyr) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/session-preserve-records-on-open-failure` --- ### 📝 Commits (2) - [`f603947`](https://github.com/morluto/rea/commit/f603947cc8b09c2e0848e626d36b3ffe9e349b3c) fix(session): preserve investigation records after failed target opens - [`41c0ce5`](https://github.com/morluto/rea/commit/41c0ce5f51668c5b03ce4470dbbf26946d360248) chore(ci): merge current main verification fixes ### 📊 Changes **3 files changed** (+192 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `docs/mcp-contracts.md` (+5 -3) 📝 `src/application/binary/BinarySession.ts` (+3 -1) ➕ `tests/boundary/mcp/sessionEvidenceRecovery.test.ts` (+184 -0) </details> ### 📄 Description ## Summary Preserve the investigation's Evidence and Unknown history when `open_binary` fails to retire the previous provider. Both target switching and same-target restart currently turn a bundle with 4 records/2 revisions into an empty bundle. Fixes #2753. Follow-up to the [unresolved P1 finding on #1776](https://github.com/morluto/rea/pull/1776#discussion_r4242283747); original finding credited there. Implemented and verified with OpenAI Codex assistance. ## Problem and expected behavior A provider cleanup failure is returned correctly, but `BinarySession.#open` also calls `clearSessionRecords()`. A subsequent export cannot recover the lost investigation. Target opens must preserve records; explicit `close_binary` remains the operation that clears them. ## Change and scope Use the existing `clearSnapshot()` owner to discard the retired provider's replay cache without clearing the investigation ledger. Keep the pending cleanup owner, unavailable target, retry gate, and explicit-close behavior. Clarify this lifetime in the MCP guide. ## Contract and boundary impact - Semantic owner and earliest changed stage: `BinarySession.#open`, after failed retirement. - Contribution scope fit and linked maintainer decision: ordinary reliability fix within existing lifecycle boundaries; advance scope decision not applicable. - CLI and MCP/tool-catalog contract: no schema or catalog changes; previously returned Evidence references remain usable after a failed open. - Provider, bridge, target-format, or platform compatibility: provider-neutral; no adapter/protocol changes. - Evidence, artifact, provenance, or reconstruction contract: preserve exact Evidence, Unknown revisions and bundle metadata; invalidate only the retired provider cache. - Target selection, authority, upstream behavior, and cleanup details: existing selection and cleanup unchanged; replacement remains blocked until cleanup succeeds. - Generated metadata, package, or installation impact: none. Expected effects exercised by the regression workflow (production adds no new target interaction): - [x] Read existing files or captured data (test-owned fixtures). - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [x] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. Production adds no target interaction. Tests also write temporary fixtures/exports and launch REA's JavaScript analysis worker; they never execute the analyzed JavaScript or a native target. ## Evidence and regression coverage - Two public MCP boundary regressions exercise a target switch and same-target restart through the production provider composition, with controlled provider failure/health. - Exact final tests on main `ab02f3c8d665ead35391c94db74cd0fb0cfed0ed`: **2 failed**, observing all 4 records and both Unknown revisions disappear. With the fix: **2 passed**. - Check repeated failed cleanup, exact bundle preservation and file export, successful retry, fresh provider queries instead of stale cache replay, retained JavaScript Evidence views, and explicit close clearing. - User-visible failure remains `cleanup_incomplete`; `get_evidence_bundle` and export retain the original records instead of empty collections. - Proof gaps: provider cleanup/health are fixtures. No new real Hopper/Ghidra/IDA failure or native Windows/Linux run; no incidence-rate claim. - [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 Pinned Node 24.18.0/npm 11.16.0 on macOS arm64, final head `f603947` rebased onto main `2995ec6`: - `npm run test:focused -- tests/boundary/mcp/sessionEvidenceRecovery.test.ts tests/boundary/mcp/sessionMcp.test.ts tests/boundary/mcp/retainedApplicationEvidenceMcp.test.ts tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts tests/composition/analysis-sessions/binarySession.test.ts src/application/binary/BinarySessionLiveness.test.ts tests/boundary/filesystem/directAnalysisSnapshotReplay.test.ts` — **76 passed, 7 files**. - `npm run check` — passed after shortening the new test helper; the initial run caught its function-length lint error. - `npm run docs:check` — passed, including 2 consistency tests. - `npm run verify:test-discovery` — all 1,055 files discovered once. - `git diff --check` — passed. Full deterministic suite not run locally; remote CI is separate. `verify:package` passed while scouting the untouched baseline; packaging checks were not rerun for this lifecycle-only diff. ## Compatibility, safety, and release - Breaking changes or migration steps: none; explicit close keeps its existing lifetime. - Real Hopper/Ghidra, browser, or OS coverage: macOS tests with actual static JavaScript analysis; native provider failure behavior unverified. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no security research or new authority; cleanup ownership and startup ordering remain covered. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved; no new capability needs a scope decision. - [x] One focused outcome and a Conventional Commit title. - [x] Related issue linked and prior finding credited. - [x] Tests cover observable behavior and meaningful failure paths. - [x] Owning documentation updated; schemas/generated metadata unchanged. - [x] Representative MCP behavior and verification limits described. - [x] Final diff checked for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:06:03 +00:00
Sign in to join this conversation.
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#5365
No description provided.