morluto/rea

Fork 0

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

[PR #4799] [CLOSED] fix(wasm): resolve WABT runtime cleanup after a successful retry #6415

New issue

Closed

opened 2026-10-31 01:14:17 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4799
Author: @kaoru0822-kitauji
Created: 10/17/2026
Status: ❌ Closed

Base: main ← Head: fix/wabt-cleanup-retry-resolution

📝 Commits (1)

3e1f48d fix(wasm): resolve WABT runtime cleanup after a successful retry

📊 Changes

4 files changed (+199 additions, -28 deletions)

View changed files

📝 src/wasm/WabtArtifactProvider.test.ts (+23 -1)
📝 src/wasm/WabtArtifactProvider.ts (+41 -26)
📝 tests/boundary/cli/wasmCleanupCli.test.ts (+91 -1)
➕ tests/fixtures/wasm/denyFirstRuntimeRootRemoval.mjs (+44 -0)

📄 Description

Summary

Fixes #4787

When the first removal of an inspect-wasm-artifact runtime root (rea-wabt-*) fails and the provider's own close retry later removes it, CLI/MCP output still reported cleanup_incomplete and listed the removed root as a live resource. If WABT had also rejected the module, the original invalid_request appeared only in diagnostics. With this change, the error resolves once that exact root confirms release. The typed primary failure becomes the reported code again, and the completed Evidence stays available as a partial observation.

Problem and expected behavior

Trigger: the first rm of the provider-owned runtime root fails, and the retry in WabtArtifactProvider.close() succeeds.

Violated invariant (own the lifecycle): bind a cleanup failure to its exact owner, and resolve the uncertainty only after that owner confirms release. Keep the original failure and the completed observations.

Root cause: WabtArtifactProvider kept the failed root in its ArtifactResourceScope as a bare private-runtime-root owner and returned an unbound ProviderCleanupError. A later successful close() released that owner but had no link back to the returned error. The primary failure was also flattened into the cleanup error's previous_error diagnostic, so even a resolved error could not show the typed primary.

Change and scope

The runtime root is now retained as an AnalysisCleanupLease labelled with the root path. The returned ProviderCleanupError is bound to that lease through AnalysisCleanupLeaseGroup, the same mechanism used by SignatureTargetBinding and the firmware provider.
A primary failure (input rejection, cancellation, output error, ...) is combined with the cleanup failure by analysisErrorWithCleanupFailure(...).bindCleanupDependencies([primary, cleanup]). Until release, the error stays cleanup_incomplete. After release, resolvedAnalysisError shows the primary.
Two other errors that report the same retained lease are now bound to it through the existing bindArtifactFileCleanup: retry failures returned from inspect() admission and the failure thrown by close(). This follows the lifecycle rule to bind every retry error the caller sees.
Not included: a cleanup failure during descriptor-backed reads inside #inspect (readStableArtifact translated by wabtFailure) is a separate owner, and this PR does not change it. HistoricalCaptureDecoder also uses the bare private-runtime-root owner kind, and this PR does not change it either.

Contribution scope: an ordinary fix within existing boundaries; a maintainer scope decision is not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: src/wasm/WabtArtifactProvider.ts (provider-owned runtime-root cleanup).
Contribution scope fit and linked maintainer decision: not applicable (existing provider lifecycle fix).
CLI and MCP/tool-catalog contract: no schema changes. What changes is the live cleanup status and code of returned errors after recovery. When a primary failure is combined with incomplete cleanup, the diagnostics now carry primary_error / cleanup_error (the shared composition shape) in place of previous_error.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: completed Evidence is still retained as partial_observation. The cleanup history is still recorded under cleanup_history.
Target selection, authority, upstream behavior, and cleanup details: affects only REA's own private runtime root. The exact owner retries it, and only that owner's confirmed release resolves the error.
Generated metadata, package, or installation impact: none.

Before (WABT rejects the module, first root removal denied, close retry succeeds), abridged. This shape comes from the source and from the report in #4787. On the base run here, the valid-module case was observed as cleanup_incomplete, and the test stopped at that assertion before reaching the rejected case.

{
  "code": "cleanup_incomplete",
  "details": {
    "cleanup": "incomplete",
    "resources": ["<TMPDIR>/rea-wabt-XXXX"],
    "diagnostics": {
      "reason": "EACCES: ...",
      "previous_error": { "code": "invalid_request" }
    }
  }
}

After (observed in the new CLI test), abridged:

{
  "code": "invalid_request",
  "details": {
    "issues": [{ "path": ["path"], "reason": "invalid_format" }],
    "captured_output": { "stderr": "fixture validation rejected\n" },
    "partial_observation": { "normalized_result": { "validation": "rejected-by-selected-profile" } },
    "cleanup_history": {
      "primary_error": { "code": "invalid_request" },
      "cleanup_error": { "code": "cleanup_incomplete", "details": { "resources": ["<TMPDIR>/rea-wabt-XXXX"] } }
    }
  }
}

For a valid module with the same recovered cleanup, the result changes from cleanup_incomplete (with the root in resources) to execution_failure, with no cleanup: "incomplete" and no live resources. The completed Evidence is still in partial_observation.

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.

(These are the existing effects of inspect-wasm-artifact, which reads the selected bytes and launches the caller-supplied WABT tools. This PR adds no new effects.)

Evidence and regression coverage

Tests added or updated:
- tests/boundary/cli/wasmCleanupCli.test.ts: new public CLI workflow test. It runs the built CLI against fake POSIX WABT 1.0.42 tools. A Node --import preload (tests/fixtures/wasm/denyFirstRuntimeRootRemoval.mjs) denies only the first fs.promises.rm of the CLI's own rea-wabt-* root in an isolated TMPDIR. Later calls use the real filesystem. The test checks that the trace shows the denial and then a real removal, that the isolated temp directory ends up empty, that code is not cleanup_incomplete, that there is no cleanup: "incomplete", and that the root is not among the live resources. It covers both a completed valid module (Evidence kept as partial_observation) and a WABT-rejected module (code: "invalid_request", with the WABT stderr kept).
- src/wasm/WabtArtifactProvider.test.ts: the existing cleanup mode now also checks that the original error and the admission-retry error both resolve after provider.close() succeeds. cancelled-close-failure now checks that the error resolves to cancelled, keeps its partial observation, and reports diagnostics under primary_error.
Base reproduction: with only the test changes and src/wasm/WabtArtifactProvider.ts restored to origin/main (7aa4b98c9), npm run test:focused -- tests/boundary/cli/wasmCleanupCli.test.ts src/wasm/WabtArtifactProvider.test.ts gave 3 failed and 21 passed (24 total). The new CLI test failed with expected 'cleanup_incomplete' not to be 'cleanup_incomplete'. The provider cleanup and cancelled-close-failure modes failed on the resolution assertions. With the fix, all 24 pass.
User-visible CLI/MCP output: see the before/after JSON above.
Remaining proof gaps: no real WABT ran (not installed here; fake shell tools stood in for it). The cleanup denial was injected and controlled; no natural OS permission failure was tested. Only macOS was covered; Windows and Linux did not run. MCP shares this provider and its errors, but no new MCP workflow test exercises recovery. Performance was not measured, since this change does not affect agent tokens or analysis time.

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

npm ci: succeeded.
npm run test:focused -- tests/boundary/cli/wasmCleanupCli.test.ts src/wasm/WabtArtifactProvider.test.ts src/wasm/WabtFailures.test.ts src/wasm/WabtInterface.test.ts src/wasm/WabtOutput.test.ts tests/boundary/cli/wasmLateCancellationCli.test.ts tests/boundary/mcp/wasmArtifactMcp.test.ts tests/boundary/cli/artifactInputLimits.test.ts: 8 files, 61 tests passed.
The same focused command on the base provider source (only the two test files): 3 failed, 21 passed (see above).
npx tsc --noEmit -p .: exit 0.
npx oxfmt and npx oxfmt --check on the 4 changed files: all correctly formatted.
npx oxlint on the 4 changed files: 0 errors. It printed warnings: WabtArtifactProvider.ts max-lines (the file was already over 500 lines on main), #inspect complexity/length (already present), and test complexity in runOwnedArtifactOutcome, a function this PR does not change.

Compatibility, safety, and release

Breaking changes or migration steps: none to schemas. Error diagnostics for a primary failure combined with incomplete cleanup now use the shared primary_error/cleanup_error composition in place of the WABT-specific previous_error key.
Real Hopper/Ghidra, browser, or OS coverage: macOS only, with fake WABT tools. No real WABT ran.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new authority. Cleanup still removes only the provider-owned private runtime root.

Author readiness

Human owner:

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.

🤖 Generated with Claude Code

_{🔄 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/4799 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/17/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/wabt-cleanup-retry-resolution` --- ### 📝 Commits (1) - [`3e1f48d`](https://github.com/morluto/rea/commit/3e1f48dd680171953b5bf2a369e723ccf0e96cd8) fix(wasm): resolve WABT runtime cleanup after a successful retry ### 📊 Changes **4 files changed** (+199 additions, -28 deletions) <details> <summary>View changed files</summary> 📝 `src/wasm/WabtArtifactProvider.test.ts` (+23 -1) 📝 `src/wasm/WabtArtifactProvider.ts` (+41 -26) 📝 `tests/boundary/cli/wasmCleanupCli.test.ts` (+91 -1) ➕ `tests/fixtures/wasm/denyFirstRuntimeRootRemoval.mjs` (+44 -0) </details> ### 📄 Description ## Summary Fixes #4787 When the first removal of an `inspect-wasm-artifact` runtime root (`rea-wabt-*`) fails and the provider's own close retry later removes it, CLI/MCP output still reported `cleanup_incomplete` and listed the removed root as a live resource. If WABT had also rejected the module, the original `invalid_request` appeared only in diagnostics. With this change, the error resolves once that exact root confirms release. The typed primary failure becomes the reported code again, and the completed Evidence stays available as a partial observation. ## Problem and expected behavior Trigger: the first `rm` of the provider-owned runtime root fails, and the retry in `WabtArtifactProvider.close()` succeeds. Violated invariant ([own the lifecycle](https://github.com/morluto/rea/blob/main/docs/tool-design.md#own-the-lifecycle)): bind a cleanup failure to its exact owner, and resolve the uncertainty only after that owner confirms release. Keep the original failure and the completed observations. Root cause: `WabtArtifactProvider` kept the failed root in its `ArtifactResourceScope` as a bare `private-runtime-root` owner and returned an unbound `ProviderCleanupError`. A later successful `close()` released that owner but had no link back to the returned error. The primary failure was also flattened into the cleanup error's `previous_error` diagnostic, so even a resolved error could not show the typed primary. ## Change and scope - The runtime root is now retained as an `AnalysisCleanupLease` labelled with the root path. The returned `ProviderCleanupError` is bound to that lease through `AnalysisCleanupLeaseGroup`, the same mechanism used by `SignatureTargetBinding` and the firmware provider. - A primary failure (input rejection, cancellation, output error, ...) is combined with the cleanup failure by `analysisErrorWithCleanupFailure(...).bindCleanupDependencies([primary, cleanup])`. Until release, the error stays `cleanup_incomplete`. After release, `resolvedAnalysisError` shows the primary. - Two other errors that report the same retained lease are now bound to it through the existing `bindArtifactFileCleanup`: retry failures returned from `inspect()` admission and the failure thrown by `close()`. This follows the lifecycle rule to bind every retry error the caller sees. - Not included: a cleanup failure during descriptor-backed reads inside `#inspect` (`readStableArtifact` translated by `wabtFailure`) is a separate owner, and this PR does not change it. `HistoricalCaptureDecoder` also uses the bare `private-runtime-root` owner kind, and this PR does not change it either. Contribution scope: an ordinary fix within existing boundaries; a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/wasm/WabtArtifactProvider.ts` (provider-owned runtime-root cleanup). - Contribution scope fit and linked maintainer decision: not applicable (existing provider lifecycle fix). - CLI and MCP/tool-catalog contract: no schema changes. What changes is the live cleanup status and code of returned errors after recovery. When a primary failure is combined with incomplete cleanup, the diagnostics now carry `primary_error` / `cleanup_error` (the shared composition shape) in place of `previous_error`. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: completed Evidence is still retained as `partial_observation`. The cleanup history is still recorded under `cleanup_history`. - Target selection, authority, upstream behavior, and cleanup details: affects only REA's own private runtime root. The exact owner retries it, and only that owner's confirmed release resolves the error. - Generated metadata, package, or installation impact: none. Before (WABT rejects the module, first root removal denied, close retry succeeds), abridged. This shape comes from the source and from the report in #4787. On the base run here, the valid-module case was observed as `cleanup_incomplete`, and the test stopped at that assertion before reaching the rejected case. ```json { "code": "cleanup_incomplete", "details": { "cleanup": "incomplete", "resources": ["<TMPDIR>/rea-wabt-XXXX"], "diagnostics": { "reason": "EACCES: ...", "previous_error": { "code": "invalid_request" } } } } ``` After (observed in the new CLI test), abridged: ```json { "code": "invalid_request", "details": { "issues": [{ "path": ["path"], "reason": "invalid_format" }], "captured_output": { "stderr": "fixture validation rejected\n" }, "partial_observation": { "normalized_result": { "validation": "rejected-by-selected-profile" } }, "cleanup_history": { "primary_error": { "code": "invalid_request" }, "cleanup_error": { "code": "cleanup_incomplete", "details": { "resources": ["<TMPDIR>/rea-wabt-XXXX"] } } } } } ``` For a valid module with the same recovered cleanup, the result changes from `cleanup_incomplete` (with the root in `resources`) to `execution_failure`, with no `cleanup: "incomplete"` and no live `resources`. The completed Evidence is still in `partial_observation`. Expected effects: - [x] Read existing files or captured data. - [x] 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. (These are the existing effects of `inspect-wasm-artifact`, which reads the selected bytes and launches the caller-supplied WABT tools. This PR adds no new effects.) ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/cli/wasmCleanupCli.test.ts`: new public CLI workflow test. It runs the built CLI against fake POSIX WABT 1.0.42 tools. A Node `--import` preload (`tests/fixtures/wasm/denyFirstRuntimeRootRemoval.mjs`) denies only the first `fs.promises.rm` of the CLI's own `rea-wabt-*` root in an isolated `TMPDIR`. Later calls use the real filesystem. The test checks that the trace shows the denial and then a real removal, that the isolated temp directory ends up empty, that `code` is not `cleanup_incomplete`, that there is no `cleanup: "incomplete"`, and that the root is not among the live `resources`. It covers both a completed valid module (Evidence kept as `partial_observation`) and a WABT-rejected module (`code: "invalid_request"`, with the WABT stderr kept). - `src/wasm/WabtArtifactProvider.test.ts`: the existing `cleanup` mode now also checks that the original error and the admission-retry error both resolve after `provider.close()` succeeds. `cancelled-close-failure` now checks that the error resolves to `cancelled`, keeps its partial observation, and reports diagnostics under `primary_error`. - Base reproduction: with only the test changes and `src/wasm/WabtArtifactProvider.ts` restored to `origin/main` (7aa4b98c9), `npm run test:focused -- tests/boundary/cli/wasmCleanupCli.test.ts src/wasm/WabtArtifactProvider.test.ts` gave 3 failed and 21 passed (24 total). The new CLI test failed with `expected 'cleanup_incomplete' not to be 'cleanup_incomplete'`. The provider `cleanup` and `cancelled-close-failure` modes failed on the resolution assertions. With the fix, all 24 pass. - User-visible CLI/MCP output: see the before/after JSON above. - Remaining proof gaps: no real WABT ran (not installed here; fake shell tools stood in for it). The cleanup denial was injected and controlled; no natural OS permission failure was tested. Only macOS was covered; Windows and Linux did not run. MCP shares this provider and its errors, but no new MCP workflow test exercises recovery. Performance was not measured, since this change does not affect agent tokens or analysis time. 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 - `npm ci`: succeeded. - `npm run test:focused -- tests/boundary/cli/wasmCleanupCli.test.ts src/wasm/WabtArtifactProvider.test.ts src/wasm/WabtFailures.test.ts src/wasm/WabtInterface.test.ts src/wasm/WabtOutput.test.ts tests/boundary/cli/wasmLateCancellationCli.test.ts tests/boundary/mcp/wasmArtifactMcp.test.ts tests/boundary/cli/artifactInputLimits.test.ts`: 8 files, 61 tests passed. - The same focused command on the base provider source (only the two test files): 3 failed, 21 passed (see above). - `npx tsc --noEmit -p .`: exit 0. - `npx oxfmt` and `npx oxfmt --check` on the 4 changed files: all correctly formatted. - `npx oxlint` on the 4 changed files: 0 errors. It printed warnings: `WabtArtifactProvider.ts` max-lines (the file was already over 500 lines on main), `#inspect` complexity/length (already present), and test complexity in `runOwnedArtifactOutcome`, a function this PR does not change. ## Compatibility, safety, and release - Breaking changes or migration steps: none to schemas. Error diagnostics for a primary failure combined with incomplete cleanup now use the shared `primary_error`/`cleanup_error` composition in place of the WABT-specific `previous_error` key. - Real Hopper/Ghidra, browser, or OS coverage: macOS only, with fake WABT tools. No real WABT ran. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new authority. Cleanup still removes only the provider-owned private runtime root. ## Author readiness Human owner: - [ ] 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #6424] [MERGED] fix(python): reject code keys and set members with unhashable consts #7352

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

No description provided.

Rows
Columns