morluto/rea

Fork 0

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

[PR #4077] [MERGED] fix(ghidra): consume inspected configuration directory identity #6042

New issue

Closed

opened 2026-10-31 01:11:25 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:11:25 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4077
Author: @jeonjw85
Created: 10/15/2026
Status: ✅ Merged
Merged: 10/16/2026
Merged by: @morluto

Base: main ← Head: fix/ghidra-configuration-identity

📝 Commits (3)

6e2bc46 fix(ghidra): consume inspected configuration directory identity
4c740e1 docs(ghidra): clarify startup failure side-effect boundary
db822d4 Merge origin/main into fix/ghidra-configuration-identity

📊 Changes

16 files changed (+255 additions, -57 deletions)

View changed files

📝 docs/installation.md (+15 -0)
📝 scripts/verify-real-ghidra-windows.mjs (+1 -0)
📝 scripts/verify-real-ghidra.mjs (+1 -0)
📝 src/ghidra/GhidraInstallation.test.ts (+80 -0)
📝 src/ghidra/GhidraInstallation.ts (+4 -0)
📝 src/ghidra/GhidraInstallationPolicy.ts (+24 -0)
📝 src/ghidra/GhidraLaunchFailure.test.ts (+2 -0)
📝 src/ghidra/GhidraLauncher.ts (+8 -29)
📝 src/ghidra/GhidraLauncherEnvironment.test.ts (+1 -0)
📝 src/ghidra/GhidraProvider.behavior.test.ts (+24 -0)
📝 src/ghidra/GhidraProviderClient.ts (+4 -0)
📝 src/ghidra/GhidraReleasePolicy.ts (+17 -1)
📝 tests/boundary/filesystem/ghidraPspExtension.test.ts (+1 -0)
📝 tests/boundary/providers/ghidra/ghidraClient.test.ts (+1 -0)
📝 tests/boundary/providers/ghidra/ghidraJavaLaunch.test.ts (+64 -1)
📝 tests/boundary/providers/ghidra/ghidraLauncher.test.ts (+8 -26)

📄 Description

Summary

Split the remaining Ghidra configuration-directory work out of #2293 onto main 47b4628ba, as requested in https://github.com/morluto/rea/pull/2293#issuecomment-6186266496. The original CLI failure-envelope fix for #2273 is already delivered; this follow-up contains no CLI output, README translation, or unrelated test-group changes.

Problem and expected behavior

Installation inspection already owns the parsed Ghidra/application.properties, but the launcher rereads that file and independently reconstructs the release directory. Missing or malformed identity falls back to the installation basename, which is not evidence of Ghidra's release identity. A second read can also disagree with the inspected facts.

The launcher should consume the inspected release coordinate and never turn unknown identity into an assumed directory name.

Change and scope

Derive configurationDirectoryName: string | null in the existing properties parser. Preserve Java properties semantics, normalize release/version spelling as Ghidra's ApplicationIdentifier/ApplicationVersion do (including 12.1.0 → 12.1), retain raw providerVersion, and accept only a safe single path component.
Carry that observation through installation inspection and provider prerequisites into the launcher. Remove the launcher's properties reread and installation-basename fallback.
Reject unresolved identity before the launcher's home/cache/config/temp preparation or Java startup, retaining the reason in the existing launch-error cause. The client may already have allocated runtime/endpoint resources and a target snapshot; existing startup-failure cleanup still applies. Existing installation version/JDK admission checks remain unchanged.
Update direct verifier/test callers and the existing installation guide. Preserve cancellation, process ownership, private runtime creation, and cleanup behavior.

Contract and boundary impact

Semantic owner: GhidraInstallationPolicy interprets release properties once; installation inspection owns the resulting observation. The provider and launcher consume it without reparsing.
Configuration ownership: the coordinate names only the release subdirectory inside REA's session-private configuration area. It neither selects the installation nor authorizes writes to Ghidra's installation or an existing user profile. Runtime ownership and cleanup remain with the existing private-runtime/process owners.
Scope decision: not applicable; a focused correction within the existing BYO Ghidra provider, not a new capability.
CLI/MCP, bridge, Evidence, target selection, packages: no schema, catalog, bridge protocol, target-mutation authority, dependency, or release-metadata changes. Missing/unsafe release metadata now prevents startup instead of guessing a directory; restore the selected release's original properties.
Compatibility limits: official supported Ghidra release layout; customized application identifiers and development layouts are not newly supported. No performance improvement is claimed or measured.

Evidence and regression coverage

Baseline: 148 existing source tests passed before edits.
Red: 15 added regressions failed on the base. The filesystem regression created the wrong release directory from changed properties; the unresolved-identity regression observed an incorrectly successful launch.
Independent source review identified an inherited zero-patch version-spelling defect. Five further regressions failed before its correction; follow-up review found the finding addressed.
Green on the final revision: eight affected source/boundary test files passed, 292 tests passed, 3 Windows-only tests skipped. Coverage includes canonical version spelling with raw identity retained, parser safety/unknowns, renamed installations, provider-to-launcher wiring, actual runtime-directory preparation, launch failures, cancellation, and rollback ownership.
Simulated Java/provider fixtures establish boundary behavior only, not real Ghidra behavior.

Validation performed

Node 24.18.0 / npm 11.16.0 on macOS arm64, using the pinned toolchain through npx:

npm ci — succeeded; no dependency or lockfile changes.
npm run test:focused -- src/ghidra/GhidraInstallation.test.ts src/ghidra/GhidraProvider.behavior.test.ts src/ghidra/GhidraLaunchFailure.test.ts src/ghidra/GhidraLauncherEnvironment.test.ts tests/boundary/providers/ghidra/ghidraJavaLaunch.test.ts tests/boundary/providers/ghidra/ghidraLauncher.test.ts tests/boundary/providers/ghidra/ghidraClient.test.ts tests/boundary/filesystem/ghidraPspExtension.test.ts — final revision: 292 passed; 3 Windows-only skips.
npm run check — passed typecheck, formatting, knip, metadata, module boundaries, architecture guards; lint had 0 errors and existing warnings.
npm run docs:check — passed generated-document/license checks and all 5 onboarding consistency tests.
npm run verify:test-discovery — discovered all 1,331 test files exactly once.
git diff --check — passed.
node scripts/verify-real-ghidra.mjs — blocked at its installation preflight because GHIDRA_INSTALL_DIR is unset; no real engine run occurred.

GitHub CI run 38720524519 passed for implementation commit 6e2bc46688bd4664f30a4d1a6ff71879d8291b44: static/source checks, all five test shards, docs/build, portable tests, and native package lanes. The Windows capability lane passed 36 files / 465 tests (25 skipped), including all 25 ghidraLauncher.test.ts tests. These are fixture-backed boundary tests, not a real Ghidra engine run.

Documentation-only head 4c740e1fa66e7045c46cd5f5fc60fc2011d59ed8 clarifies the startup side-effect boundary without changing the implementation. The focused suite (292 passed, 3 Windows-only skips), npm run check, and npm run docs:check were rerun successfully with it; source review found no Important/Critical issue. Its CI run passed, including the required CI check, all five test shards, and the Windows capability/package lane. Optional skipped lanes do not establish additional provider coverage.

Earlier local full-suite run and reproduced baseline DMG failures

npm test — before the final version-canonicalization correction, 15,175 passed / 3 failed / 341 skipped (1,331 files; 1,538 s). The failures are the three native DMG real verification diagnostics cases in tests/boundary/providers/native/nativeDmgArtifactReader.test.ts: format-refusal, target-access I/O, and checksum-integrity classification each received unavailable instead of the expected category. All three reproduced identically using npm exec -- vitest run --project boundary tests/boundary/providers/native/nativeDmgArtifactReader.test.ts -t 'native DMG real verification diagnostics' in a pristine git archive snapshot of main 47b4628ba with the same installed dependencies. No unrelated DMG changes were made. The final revision was rebuilt and rechecked with the focused suite above; the full suite was not rerun after that last correction.

Compatibility, safety, and release

CI and fixture-backed tests passed; real Ghidra engine execution was not performed.

Author readiness

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/4077 **Author:** [@jeonjw85](https://github.com/jeonjw85) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/16/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/ghidra-configuration-identity` --- ### 📝 Commits (3) - [`6e2bc46`](https://github.com/morluto/rea/commit/6e2bc46688bd4664f30a4d1a6ff71879d8291b44) fix(ghidra): consume inspected configuration directory identity - [`4c740e1`](https://github.com/morluto/rea/commit/4c740e1fa66e7045c46cd5f5fc60fc2011d59ed8) docs(ghidra): clarify startup failure side-effect boundary - [`db822d4`](https://github.com/morluto/rea/commit/db822d41bef58ca81c6ff7d52eee082562677c0e) Merge origin/main into fix/ghidra-configuration-identity ### 📊 Changes **16 files changed** (+255 additions, -57 deletions) <details> <summary>View changed files</summary> 📝 `docs/installation.md` (+15 -0) 📝 `scripts/verify-real-ghidra-windows.mjs` (+1 -0) 📝 `scripts/verify-real-ghidra.mjs` (+1 -0) 📝 `src/ghidra/GhidraInstallation.test.ts` (+80 -0) 📝 `src/ghidra/GhidraInstallation.ts` (+4 -0) 📝 `src/ghidra/GhidraInstallationPolicy.ts` (+24 -0) 📝 `src/ghidra/GhidraLaunchFailure.test.ts` (+2 -0) 📝 `src/ghidra/GhidraLauncher.ts` (+8 -29) 📝 `src/ghidra/GhidraLauncherEnvironment.test.ts` (+1 -0) 📝 `src/ghidra/GhidraProvider.behavior.test.ts` (+24 -0) 📝 `src/ghidra/GhidraProviderClient.ts` (+4 -0) 📝 `src/ghidra/GhidraReleasePolicy.ts` (+17 -1) 📝 `tests/boundary/filesystem/ghidraPspExtension.test.ts` (+1 -0) 📝 `tests/boundary/providers/ghidra/ghidraClient.test.ts` (+1 -0) 📝 `tests/boundary/providers/ghidra/ghidraJavaLaunch.test.ts` (+64 -1) 📝 `tests/boundary/providers/ghidra/ghidraLauncher.test.ts` (+8 -26) </details> ### 📄 Description ## Summary Split the remaining Ghidra configuration-directory work out of #2293 onto main `47b4628ba`, as requested in https://github.com/morluto/rea/pull/2293#issuecomment-6186266496. The original CLI failure-envelope fix for #2273 is already delivered; this follow-up contains no CLI output, README translation, or unrelated test-group changes. ## Problem and expected behavior Installation inspection already owns the parsed `Ghidra/application.properties`, but the launcher rereads that file and independently reconstructs the release directory. Missing or malformed identity falls back to the installation basename, which is not evidence of Ghidra's release identity. A second read can also disagree with the inspected facts. The launcher should consume the inspected release coordinate and never turn unknown identity into an assumed directory name. ## Change and scope - Derive `configurationDirectoryName: string | null` in the existing properties parser. Preserve Java properties semantics, normalize release/version spelling as Ghidra's `ApplicationIdentifier`/`ApplicationVersion` do (including `12.1.0` → `12.1`), retain raw `providerVersion`, and accept only a safe single path component. - Carry that observation through installation inspection and provider prerequisites into the launcher. Remove the launcher's properties reread and installation-basename fallback. - Reject unresolved identity before the launcher's home/cache/config/temp preparation or Java startup, retaining the reason in the existing launch-error cause. The client may already have allocated runtime/endpoint resources and a target snapshot; existing startup-failure cleanup still applies. Existing installation version/JDK admission checks remain unchanged. - Update direct verifier/test callers and the existing installation guide. Preserve cancellation, process ownership, private runtime creation, and cleanup behavior. ## Contract and boundary impact - **Semantic owner:** `GhidraInstallationPolicy` interprets release properties once; installation inspection owns the resulting observation. The provider and launcher consume it without reparsing. - **Configuration ownership:** the coordinate names only the release subdirectory inside REA's session-private configuration area. It neither selects the installation nor authorizes writes to Ghidra's installation or an existing user profile. Runtime ownership and cleanup remain with the existing private-runtime/process owners. - **Scope decision:** not applicable; a focused correction within the existing BYO Ghidra provider, not a new capability. - **CLI/MCP, bridge, Evidence, target selection, packages:** no schema, catalog, bridge protocol, target-mutation authority, dependency, or release-metadata changes. Missing/unsafe release metadata now prevents startup instead of guessing a directory; restore the selected release's original properties. - **Compatibility limits:** official supported Ghidra release layout; customized application identifiers and development layouts are not newly supported. No performance improvement is claimed or measured. ## Evidence and regression coverage - Baseline: 148 existing source tests passed before edits. - Red: 15 added regressions failed on the base. The filesystem regression created the wrong release directory from changed properties; the unresolved-identity regression observed an incorrectly successful launch. - Independent source review identified an inherited zero-patch version-spelling defect. Five further regressions failed before its correction; follow-up review found the finding addressed. - Green on the final revision: eight affected source/boundary test files passed, **292 tests passed, 3 Windows-only tests skipped**. Coverage includes canonical version spelling with raw identity retained, parser safety/unknowns, renamed installations, provider-to-launcher wiring, actual runtime-directory preparation, launch failures, cancellation, and rollback ownership. - Simulated Java/provider fixtures establish boundary behavior only, not real Ghidra behavior. ## Validation performed Node 24.18.0 / npm 11.16.0 on macOS arm64, using the pinned toolchain through `npx`: - `npm ci` — succeeded; no dependency or lockfile changes. - `npm run test:focused -- src/ghidra/GhidraInstallation.test.ts src/ghidra/GhidraProvider.behavior.test.ts src/ghidra/GhidraLaunchFailure.test.ts src/ghidra/GhidraLauncherEnvironment.test.ts tests/boundary/providers/ghidra/ghidraJavaLaunch.test.ts tests/boundary/providers/ghidra/ghidraLauncher.test.ts tests/boundary/providers/ghidra/ghidraClient.test.ts tests/boundary/filesystem/ghidraPspExtension.test.ts` — final revision: 292 passed; 3 Windows-only skips. - `npm run check` — passed typecheck, formatting, knip, metadata, module boundaries, architecture guards; lint had 0 errors and existing warnings. - `npm run docs:check` — passed generated-document/license checks and all 5 onboarding consistency tests. - `npm run verify:test-discovery` — discovered all 1,331 test files exactly once. - `git diff --check` — passed. - `node scripts/verify-real-ghidra.mjs` — blocked at its installation preflight because `GHIDRA_INSTALL_DIR` is unset; no real engine run occurred. [GitHub CI run 38720524519](https://github.com/morluto/rea/actions/runs/38720524519) passed for implementation commit `6e2bc46688bd4664f30a4d1a6ff71879d8291b44`: static/source checks, all five test shards, docs/build, portable tests, and native package lanes. The Windows capability lane passed 36 files / 465 tests (25 skipped), including all 25 `ghidraLauncher.test.ts` tests. These are fixture-backed boundary tests, not a real Ghidra engine run. Documentation-only head `4c740e1fa66e7045c46cd5f5fc60fc2011d59ed8` clarifies the startup side-effect boundary without changing the implementation. The focused suite (292 passed, 3 Windows-only skips), `npm run check`, and `npm run docs:check` were rerun successfully with it; source review found no Important/Critical issue. [Its CI run](https://github.com/morluto/rea/actions/runs/38723131662) passed, including the required CI check, all five test shards, and the Windows capability/package lane. Optional skipped lanes do not establish additional provider coverage. <details> <summary>Earlier local full-suite run and reproduced baseline DMG failures</summary> - `npm test` — **before the final version-canonicalization correction**, 15,175 passed / 3 failed / 341 skipped (1,331 files; 1,538 s). The failures are the three `native DMG real verification diagnostics` cases in `tests/boundary/providers/native/nativeDmgArtifactReader.test.ts`: format-refusal, target-access I/O, and checksum-integrity classification each received `unavailable` instead of the expected category. All three reproduced identically using `npm exec -- vitest run --project boundary tests/boundary/providers/native/nativeDmgArtifactReader.test.ts -t 'native DMG real verification diagnostics'` in a pristine `git archive` snapshot of main `47b4628ba` with the same installed dependencies. No unrelated DMG changes were made. The final revision was rebuilt and rechecked with the focused suite above; the full suite was not rerun after that last correction. </details> ## Compatibility, safety, and release CI and fixture-backed tests passed; real Ghidra engine execution was not performed. ## Author readiness - [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-10-31 01:11:25 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:45:48 +00:00

[PR #6143] [MERGED] fix(cancellation): preserve caller abort reasons through late finalization #7158

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

No description provided.

Rows
Columns