morluto/rea

Fork 0

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

[PR #6393] [CLOSED] fix(artifacts): keep the primary catalog failure when cleanup fails #7330

New issue

Closed

opened 2026-11-01 21:47:07 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:47:07 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6393
Author: @saimmehmood
Created: 10/22/2026
Status: ❌ Closed

Base: main ← Head: fix/asset-catalog-cleanup-primary-failure

📝 Commits (2)

d998479 fix(artifacts): keep the primary catalog failure when cleanup fails
c19782a fix(artifacts): retain the catalog reader as its cleanup owner

📊 Changes

3 files changed (+189 additions, -8 deletions)

View changed files

📝 src/artifacts/DirectoryArtifactReader.ts (+9 -7)
📝 src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts (+151 -0)
📝 src/artifacts/apple/AppleAssetCatalogAnalysis.ts (+29 -1)

📄 Description

Summary

inspect_asset_catalog discarded its primary catalog diagnosis whenever the
bundle directory reader also failed to close. The caller received the cleanup
failure instead of the reason the catalog could not be inspected.

Fixes #5992.

Problem and expected behavior

collectAssetCatalogs closed its DirectoryArtifactReader from a bare
finally, so a rejected close() propagated out of the block and replaced the
in-flight walk failure:

} finally {
  await reader.close();
}

Smallest trigger: a catalog whose identity changes between the pre- and
post-assetutil lstat, while the reader's directory close rejects. The walk
throws ArtifactReaderFailure("integrity", "Asset catalog changed during inspection: ..."); the caller instead observes reason: "io" and
"Directory artifact cleanup failed".

Violated invariant: cleanup must report unverified release without replacing the
primary diagnostic. ArtifactReaderFailure.withCleanup already models that
composition, and DirectoryArtifactReader.#finishDirectory already applies it
one level down for the same failure pair.

Expected: the final code may remain cleanup-bearing while reason: "integrity",
the changed-catalog message and cause, and the cleanup resources all survive.

Change and scope

Record the walk outcome instead of letting finally swallow it, then close
explicitly:

walk failed and close failed → ArtifactReaderFailure.withCleanup(walkCause, cleanupObservation(closeCause, bundlePath)), preserving the primary reason,
message, cause and boundary while merging the cleanup observation.
walk succeeded and close failed → the close failure propagates unchanged, so
unverified release is still reported.
close succeeded and walk failed → the walk failure propagates unchanged.

cleanupObservation returns the close failure's own cleanup when it carries
one, so DirectoryArtifactReader.close()'s real resource list is preserved;
bundlePath is only the fallback for an untyped cause.

Not included: no change to assetutil invocation, catalog parsing, paging, the
DirectoryArtifactReader itself, or any other reader with the same shape. This
is the single reported path.

Follow the contribution scope.

Contract and boundary impact

Semantic owner and earliest changed stage: collectAssetCatalogs in
src/artifacts/apple/AppleAssetCatalogAnalysis.ts; error composition at the
artifact-reader boundary.
Contribution scope fit and linked maintainer decision: ordinary fix within an
existing boundary; not applicable.
CLI and MCP/tool-catalog contract: unchanged. No schema, tool name, or
advertised field changes. The failure classification callers already receive
for a primary-only failure is now also what they receive when cleanup
coincides.
Provider, bridge, target-format, or platform compatibility: unchanged. The path
is macOS-only via /usr/bin/assetutil; tests drive it through the injected
runAssetUtil seam.
Evidence, artifact, provenance, or reconstruction contract: strictly more
preserved — the primary reason, message, cause and cleanup resources now all
survive instead of only the cleanup failure.
Target selection, authority, upstream behavior, and cleanup details: cleanup
behavior itself is unchanged; reader.close() is still awaited exactly once on
every path. Only the reported error composition changes.
Generated metadata, package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

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

Executed, not source-derived.

Tests added or updated: src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts
gains a describe("Apple asset catalog cleanup composition") with three cases:
the integrity and format primary failures composed with a rejected close,
and a cleanup-only control asserting the close failure still surfaces by
identity. The close failure is injected by spying on
DirectoryArtifactReader.prototype.close, so the real production composition
runs; no filesystem module is mocked.
Base reproduction: the two composition cases fail on the affected base
revision, with the cleanup-only control already passing:

FAIL  retains the primary integrity catalog failure when directory cleanup fails
-   "message": StringContaining "Asset catalog changed during inspection",
-   "reason": "integrity",
+   "reason": "io",

FAIL  retains the primary format catalog failure when directory cleanup fails
-   "message": StringContaining "assetutil returned invalid JSON",
-   "reason": "format",
+   "reason": "io",

Tests  2 failed | 1 passed | 16 skipped (19)

The received error is ArtifactReaderFailure: Directory artifact cleanup failed, confirming the close failure replaced the primary. Both cases pass
after the change.

User-visible CLI/MCP output: not applicable; no output shape changed. The
projected error for a coincident failure now carries the catalog reason and
detail that a primary-only failure already produced.
Remaining proof gaps: the close rejection is injected at the reader seam, so
this does not establish natural close-failure incidence, real assetutil
behavior under a failing close, or non-macOS behavior. No real
/usr/bin/assetutil run is included — the macOS verification lane was not
executed. Other readers sharing this shape were not audited.

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 --project adapters src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts — 19 passed (19).
npx vitest run --project adapters src/artifacts/ — 34 files, 471 passed (471).
npm run check:fast — typecheck + lint, 2 successful, 2 total.
npm run lint — 4695 files, 0 errors.
npx oxfmt --check src/artifacts/apple/ — all matched files correctly formatted.

Not run: npm run check:pr, built/boundary/acceptance projects, and the macOS
real-assetutil verification lane. A skipped lane is not a pass.

Compatibility, safety, and release

Breaking changes or migration steps: none. The change only adds preserved
information to an existing failure path.
Real Hopper/Ghidra, browser, or OS coverage: none exercised; this path is
macOS assetutil and was driven through its injected seam.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new process launch,
network access, or filesystem write outside the test temporary directory,
which comes from createTestTempDirectory for exact-path awaited cleanup.

Author readiness

Human owner: @saimmehmood

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/6393 **Author:** [@saimmehmood](https://github.com/saimmehmood) **Created:** 10/22/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/asset-catalog-cleanup-primary-failure` --- ### 📝 Commits (2) - [`d998479`](https://github.com/morluto/rea/commit/d99847989ac3ede3ff767fa0b65fffb6c323172e) fix(artifacts): keep the primary catalog failure when cleanup fails - [`c19782a`](https://github.com/morluto/rea/commit/c19782a3e25ef5cfaf82d2c6b159f82dca53d086) fix(artifacts): retain the catalog reader as its cleanup owner ### 📊 Changes **3 files changed** (+189 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/DirectoryArtifactReader.ts` (+9 -7) 📝 `src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts` (+151 -0) 📝 `src/artifacts/apple/AppleAssetCatalogAnalysis.ts` (+29 -1) </details> ### 📄 Description ## Summary `inspect_asset_catalog` discarded its primary catalog diagnosis whenever the bundle directory reader also failed to close. The caller received the cleanup failure instead of the reason the catalog could not be inspected. Fixes #5992. ## Problem and expected behavior `collectAssetCatalogs` closed its `DirectoryArtifactReader` from a bare `finally`, so a rejected `close()` propagated out of the block and replaced the in-flight walk failure: ```ts } finally { await reader.close(); } ``` Smallest trigger: a catalog whose identity changes between the pre- and post-`assetutil` `lstat`, while the reader's directory close rejects. The walk throws `ArtifactReaderFailure("integrity", "Asset catalog changed during inspection: ...")`; the caller instead observes `reason: "io"` and `"Directory artifact cleanup failed"`. Violated invariant: cleanup must report unverified release without replacing the primary diagnostic. `ArtifactReaderFailure.withCleanup` already models that composition, and `DirectoryArtifactReader.#finishDirectory` already applies it one level down for the same failure pair. Expected: the final code may remain cleanup-bearing while `reason: "integrity"`, the changed-catalog message and cause, and the cleanup resources all survive. ## Change and scope Record the walk outcome instead of letting `finally` swallow it, then close explicitly: - walk failed **and** close failed → `ArtifactReaderFailure.withCleanup(walkCause, cleanupObservation(closeCause, bundlePath))`, preserving the primary reason, message, cause and boundary while merging the cleanup observation. - walk succeeded and close failed → the close failure propagates unchanged, so unverified release is still reported. - close succeeded and walk failed → the walk failure propagates unchanged. `cleanupObservation` returns the close failure's own `cleanup` when it carries one, so `DirectoryArtifactReader.close()`'s real resource list is preserved; `bundlePath` is only the fallback for an untyped cause. Not included: no change to `assetutil` invocation, catalog parsing, paging, the `DirectoryArtifactReader` itself, or any other reader with the same shape. This is the single reported path. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). ## Contract and boundary impact - Semantic owner and earliest changed stage: `collectAssetCatalogs` in `src/artifacts/apple/AppleAssetCatalogAnalysis.ts`; error composition at the artifact-reader boundary. - Contribution scope fit and linked maintainer decision: ordinary fix within an existing boundary; not applicable. - CLI and MCP/tool-catalog contract: unchanged. No schema, tool name, or advertised field changes. The failure classification callers already receive for a primary-only failure is now also what they receive when cleanup coincides. - Provider, bridge, target-format, or platform compatibility: unchanged. The path is macOS-only via `/usr/bin/assetutil`; tests drive it through the injected `runAssetUtil` seam. - Evidence, artifact, provenance, or reconstruction contract: strictly more preserved — the primary reason, message, cause and cleanup resources now all survive instead of only the cleanup failure. - Target selection, authority, upstream behavior, and cleanup details: cleanup behavior itself is unchanged; `reader.close()` is still awaited exactly once on every path. Only the reported error composition changes. - Generated metadata, package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [x] Launch or attach to a process. - [x] Inject instrumentation or run scripts/code. - [x] Change process memory, registers, files, or configuration. - [x] Contact network services. - [x] Recover protected code or work around protections. - [x] Documentation or usability only; no target interaction. - [x] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage Executed, not source-derived. - Tests added or updated: `src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts` gains a `describe("Apple asset catalog cleanup composition")` with three cases: the `integrity` and `format` primary failures composed with a rejected close, and a cleanup-only control asserting the close failure still surfaces by identity. The close failure is injected by spying on `DirectoryArtifactReader.prototype.close`, so the real production composition runs; no filesystem module is mocked. - Base reproduction: the two composition cases fail on the affected base revision, with the cleanup-only control already passing: ``` FAIL retains the primary integrity catalog failure when directory cleanup fails - "message": StringContaining "Asset catalog changed during inspection", - "reason": "integrity", + "reason": "io", FAIL retains the primary format catalog failure when directory cleanup fails - "message": StringContaining "assetutil returned invalid JSON", - "reason": "format", + "reason": "io", Tests 2 failed | 1 passed | 16 skipped (19) ``` The received error is `ArtifactReaderFailure: Directory artifact cleanup failed`, confirming the close failure replaced the primary. Both cases pass after the change. - User-visible CLI/MCP output: not applicable; no output shape changed. The projected error for a coincident failure now carries the catalog reason and detail that a primary-only failure already produced. - Remaining proof gaps: the close rejection is injected at the reader seam, so this does not establish natural close-failure incidence, real `assetutil` behavior under a failing close, or non-macOS behavior. No real `/usr/bin/assetutil` run is included — the macOS verification lane was not executed. Other readers sharing this shape were not audited. 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 --project adapters src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts` — 19 passed (19). - `npx vitest run --project adapters src/artifacts/` — 34 files, 471 passed (471). - `npm run check:fast` — typecheck + lint, 2 successful, 2 total. - `npm run lint` — 4695 files, 0 errors. - `npx oxfmt --check src/artifacts/apple/` — all matched files correctly formatted. Not run: `npm run check:pr`, built/boundary/acceptance projects, and the macOS real-`assetutil` verification lane. A skipped lane is not a pass. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The change only adds preserved information to an existing failure path. - Real Hopper/Ghidra, browser, or OS coverage: none exercised; this path is macOS `assetutil` and was driven through its injected seam. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new process launch, network access, or filesystem write outside the test temporary directory, which comes from `createTestTempDirectory` for exact-path awaited cleanup. ## Author readiness Human owner: @saimmehmood - [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 - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] 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-11-01 21:47:07 +00:00