[PR #4544] [CLOSED] fix(ghidra): own Allegrex PSP descriptor close failures (#4521) #6296

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4544
Author: @DeryFerd
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/4521-allegrex-descriptor-ownership


📝 Commits (1)

  • b9e57b0 fix(ghidra): own Allegrex PSP descriptor close failures

📊 Changes

2 files changed (+366 additions, -89 deletions)

View changed files

📝 src/ghidra/GhidraPspExtension.ts (+150 -89)
➕ tests/boundary/filesystem/ghidraPspDescriptorOwnership.test.ts (+216 -0)

📄 Description

Summary

inspectGhidraPspExtension fingerprints every installed Allegrex component by opening each file with the canonical openRegularFile primitive and closing it in a bare finally { await handle.close(); }. If that close rejects, the rejection replaces any earlier read/change/format failure, and the function retains no owner able to retry the close. The surrounding resolveGhidraPspExtension catch separately substitutes cancellation whenever the caller signal happens to be aborted at catch time, discarding the producer's physical outcome.

This change makes the Allegrex PSP inspection own its descriptor lifecycle through the canonical admitted-file operation and normalize cancellation at its producer, so a physical failure and an unresolved close owner are reported together instead of being lost.

Fixes #4521

Problem and expected behavior

Smallest trigger: a caller-installed ghidra-allegrex extension whose component descriptor close rejects after a successful or failing read. Before this change the thrown close error replaced the read/change/format failure, and because no owner was retained, a later retry could not release the descriptor. A second trigger: any physical inspection failure followed by a caller abort during the same failed operation — the resolver reported AnalysisCancelledError even though the producer had produced a real filesystem/format failure first.

Violated invariants: proof of ownership must stay separate from validity, and a later lifecycle failure must not replace an earlier producer outcome (AGENTS.md lifecycle rules). Expected behavior: the physical inspection outcome stays primary, the unresolved descriptor owner is retained for a bounded retry, and cancellation is reported only where a read boundary actually observed the caller signal.

Change and scope

Admit each component through withArtifactRegularFile(path, resources, signal, operation) and own the descriptors in one ArtifactResourceScope. The scope retries every retained owner once at the end of inspection. A still-failing close keeps the physical inspection outcome primary and surfaces the exact owner as cleanup; a successful close with a completed inspection returns the digest as before.

resolveGhidraPspExtension now reports AnalysisCancelledError only when the failure carries a producer-observed cancellation, and otherwise preserves the physical failure. Descriptor cleanup, when present, is attached with analysisErrorWithCleanupFailure and bound to its exact leases so a retry can resolve it.

Content commitment (sha256 over raw bytes), required-file semantics, the entry/byte/depth bounds, and the canonical-path check are unchanged. Windows behavior is untouched: the inspection path is the same on all platforms, and no new provider capability or execution authority is introduced.

Not included: GhidraLauncher's own re-validation call site is unchanged; it already propagates the typed failure from inspectGhidraPspExtension. GhidraLanguageCatalog uses a separate owner and is out of scope.

Contract and boundary impact

  • Semantic owner and earliest changed stage: descriptor admission and close ownership for Allegrex PSP fingerprinting, in src/ghidra/GhidraPspExtension.ts; earliest changed stage is the descriptor lifecycle during profile resolution.
  • Contribution scope fit and linked maintainer decision: ordinary fix within an existing provider boundary; not applicable (no new capability, execution, injection, mutation, or protection recovery).
  • CLI and MCP/tool-catalog contract: none. The resolve_analysis_profile result shape is unchanged; only the reported failure reason/tag for the affected paths changes.
  • Provider, bridge, target-format, or platform compatibility: Ghidra PSP profile only. No provider version, bridge protocol, or target-format change. Same behavior on all platforms.
  • Evidence, artifact, provenance, or reconstruction contract: the PSP sha256 content commitment is unchanged.
  • Target selection, authority, upstream behavior, and cleanup details: read-only fingerprint of a caller-installed extension; no target execution. Cleanup ownership is now explicit and retryable.
  • Generated metadata, package, or installation impact: none.

Expected effects:

  • Read existing files or captured data.
  • 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.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/filesystem/ghidraPspDescriptorOwnership.test.ts adds two tests. The first forces a component read failure plus a rejecting close and asserts the physical failure stays primary with the retained owner reported as cleanup. The second aborts the caller signal only after a physical read failure and asserts the resolver reports AnalysisCapabilityUnavailableError rather than substituting cancellation.
  • Base reproduction or other evidence: both tests fail on main before this change — the first because the old code throws RegularFileAdmissionFailure with no ArtifactReaderFailure cleanup projection, the second because the resolver returns AnalysisCancelledError instead of the physical failure.
  • User-visible CLI/MCP output: not applicable; the tool result schema is unchanged.
  • Remaining proof gaps: no real Ghidra/Allegrex installation and no native-engine fault injection were run. The tests use a synthetic filesystem seam; a real close-failure and a real PSP extension remain unverified at runtime.

For evidence-bearing changes:

  • 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

  • npx vitest run tests/boundary/filesystem/ghidraPspDescriptorOwnership.test.ts — 2 passed.
  • npx vitest run tests/boundary/filesystem/ghidraPspExtension.test.ts — 1 passed, 43 skipped (POSIX-gated suite skipped on Windows).
  • npx vitest run tests/boundary/providers/ghidra/ghidraLauncher.test.ts tests/boundary/filesystem/ghidraLanguageCatalogCleanup.test.ts tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts — 32 passed.
  • npx tsc --noEmit — clean.
  • npx oxlint and npx oxfmt --check on the changed files — clean.
  • Base revision check: reverting src/ghidra/GhidraPspExtension.ts to main makes both new tests fail, confirming the regression coverage.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: none run; only synthetic filesystem-seam tests.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no change to admission checks, path canonicalization, or bounds; the change only adds explicit descriptor ownership and failure provenance.

Author readiness

Human owner: @DeryFerd

  • 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

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff 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/4544 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/4521-allegrex-descriptor-ownership` --- ### 📝 Commits (1) - [`b9e57b0`](https://github.com/morluto/rea/commit/b9e57b086b0e6c0d7f8e79d4fc34b83f8a578e45) fix(ghidra): own Allegrex PSP descriptor close failures ### 📊 Changes **2 files changed** (+366 additions, -89 deletions) <details> <summary>View changed files</summary> 📝 `src/ghidra/GhidraPspExtension.ts` (+150 -89) ➕ `tests/boundary/filesystem/ghidraPspDescriptorOwnership.test.ts` (+216 -0) </details> ### 📄 Description ## Summary `inspectGhidraPspExtension` fingerprints every installed Allegrex component by opening each file with the canonical `openRegularFile` primitive and closing it in a bare `finally { await handle.close(); }`. If that close rejects, the rejection replaces any earlier read/change/format failure, and the function retains no owner able to retry the close. The surrounding `resolveGhidraPspExtension` catch separately substitutes cancellation whenever the caller signal happens to be aborted at catch time, discarding the producer's physical outcome. This change makes the Allegrex PSP inspection own its descriptor lifecycle through the canonical admitted-file operation and normalize cancellation at its producer, so a physical failure and an unresolved close owner are reported together instead of being lost. Fixes #4521 ## Problem and expected behavior Smallest trigger: a caller-installed `ghidra-allegrex` extension whose component descriptor close rejects after a successful or failing read. Before this change the thrown close error replaced the read/change/format failure, and because no owner was retained, a later retry could not release the descriptor. A second trigger: any physical inspection failure followed by a caller abort during the same failed operation — the resolver reported `AnalysisCancelledError` even though the producer had produced a real filesystem/format failure first. Violated invariants: proof of ownership must stay separate from validity, and a later lifecycle failure must not replace an earlier producer outcome (AGENTS.md lifecycle rules). Expected behavior: the physical inspection outcome stays primary, the unresolved descriptor owner is retained for a bounded retry, and cancellation is reported only where a read boundary actually observed the caller signal. ## Change and scope Admit each component through `withArtifactRegularFile(path, resources, signal, operation)` and own the descriptors in one `ArtifactResourceScope`. The scope retries every retained owner once at the end of inspection. A still-failing close keeps the physical inspection outcome primary and surfaces the exact owner as cleanup; a successful close with a completed inspection returns the digest as before. `resolveGhidraPspExtension` now reports `AnalysisCancelledError` only when the failure carries a producer-observed cancellation, and otherwise preserves the physical failure. Descriptor cleanup, when present, is attached with `analysisErrorWithCleanupFailure` and bound to its exact leases so a retry can resolve it. Content commitment (`sha256` over raw bytes), required-file semantics, the entry/byte/depth bounds, and the canonical-path check are unchanged. Windows behavior is untouched: the inspection path is the same on all platforms, and no new provider capability or execution authority is introduced. Not included: `GhidraLauncher`'s own re-validation call site is unchanged; it already propagates the typed failure from `inspectGhidraPspExtension`. `GhidraLanguageCatalog` uses a separate owner and is out of scope. ## Contract and boundary impact - Semantic owner and earliest changed stage: descriptor admission and close ownership for Allegrex PSP fingerprinting, in `src/ghidra/GhidraPspExtension.ts`; earliest changed stage is the descriptor lifecycle during profile resolution. - Contribution scope fit and linked maintainer decision: ordinary fix within an existing provider boundary; not applicable (no new capability, execution, injection, mutation, or protection recovery). - CLI and MCP/tool-catalog contract: none. The `resolve_analysis_profile` result shape is unchanged; only the reported failure reason/tag for the affected paths changes. - Provider, bridge, target-format, or platform compatibility: Ghidra PSP profile only. No provider version, bridge protocol, or target-format change. Same behavior on all platforms. - Evidence, artifact, provenance, or reconstruction contract: the PSP `sha256` content commitment is unchanged. - Target selection, authority, upstream behavior, and cleanup details: read-only fingerprint of a caller-installed extension; no target execution. Cleanup ownership is now explicit and retryable. - Generated metadata, package, or installation impact: none. Expected effects: - [x] Read existing files or captured data. - [ ] 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. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/filesystem/ghidraPspDescriptorOwnership.test.ts` adds two tests. The first forces a component read failure plus a rejecting close and asserts the physical failure stays primary with the retained owner reported as cleanup. The second aborts the caller signal only after a physical read failure and asserts the resolver reports `AnalysisCapabilityUnavailableError` rather than substituting cancellation. - Base reproduction or other evidence: both tests fail on `main` before this change — the first because the old code throws `RegularFileAdmissionFailure` with no `ArtifactReaderFailure` cleanup projection, the second because the resolver returns `AnalysisCancelledError` instead of the physical failure. - User-visible CLI/MCP output: not applicable; the tool result schema is unchanged. - Remaining proof gaps: no real Ghidra/Allegrex installation and no native-engine fault injection were run. The tests use a synthetic filesystem seam; a real close-failure and a real PSP extension remain unverified at runtime. For evidence-bearing changes: - [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 - `npx vitest run tests/boundary/filesystem/ghidraPspDescriptorOwnership.test.ts` — 2 passed. - `npx vitest run tests/boundary/filesystem/ghidraPspExtension.test.ts` — 1 passed, 43 skipped (POSIX-gated suite skipped on Windows). - `npx vitest run tests/boundary/providers/ghidra/ghidraLauncher.test.ts tests/boundary/filesystem/ghidraLanguageCatalogCleanup.test.ts tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts` — 32 passed. - `npx tsc --noEmit` — clean. - `npx oxlint` and `npx oxfmt --check` on the changed files — clean. - Base revision check: reverting `src/ghidra/GhidraPspExtension.ts` to `main` makes both new tests fail, confirming the regression coverage. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: none run; only synthetic filesystem-seam tests. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no change to admission checks, path canonicalization, or bounds; the change only adds explicit descriptor ownership and failure provenance. ## Author readiness Human owner: @DeryFerd - [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 - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff 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:13:23 +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#6296
No description provided.