morluto/rea

Fork 0

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

[PR #4762] [MERGED] fix(flutter): report cancellation during snapshot cleanup with completed observations #6396

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4762
Author: @kaoru0822-kitauji
Created: 10/17/2026
Status: ✅ Merged
Merged: 10/17/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/flutter-cancel-during-cleanup

📝 Commits (1)

ac2486c fix(flutter): report cancellation during snapshot cleanup with completed observations

📊 Changes

2 files changed (+92 additions, -1 deletions)

View changed files

📝 src/cli/flutterCommands.ts (+2 -0)
📝 tests/boundary/cli/flutterCli.test.ts (+90 -1)

📄 Description

Summary

Fixes #4640

identify-flutter-build and inspect-dart-aot printed ordinary successful Evidence when SIGINT arrived during the provider's close-time retry of a snapshot root whose first removal had failed. The process still exited 130, so the JSON and the exit status disagreed. With this change those runs return typed cancelled and keep the completed Evidence under details.partial_observation. Runs that are never interrupted still succeed.

Problem and expected behavior

Trigger: the first snapshot-root removal inside FlutterBuildProvider.execute fails. The provider keeps the root and returns successful inspection Evidence. provider.close() then retries the removal from the one-shot CLI finalizer, and SIGINT arrives during that awaited retry.

Violated invariant (own the lifecycle): before returning success, a cancellable observation must check the signal after its last awaited finalization step and keep collected observations under typed cancellation.

Root cause: registerFlutterCommands gave the command signal to the inspection but not to finishAnalysisCommand. The finalizer's post-close guard if (result.ok && signal?.aborted === true) only runs when a signal is supplied, so the abort observed after close() was dropped. withCommandCancellation still set exit code 130.

Change and scope

src/cli/flutterCommands.ts now passes the command signal to finishAnalysisCommand, which owns finalization for one-shot CLI commands. This matches the firmware, Rust and WASM registrations. The finalizer's existing ordering is unchanged:

A failed close (incomplete cleanup) is still reported first as cleanup_incomplete with the completed Evidence retained, so a later signal does not override an earlier physical failure.
A primary failure from the inspection keeps its own classification. Only successful results become cancellations.
The caller's abort reason stays the cancellation cause.

Not included:

#4717: SIGINT during a successful first removal inside FlutterBuildProvider.execute. That path is caught by the provider's own post-cleanup guard, which builds a cancellation error without partialObservation. It is a separate producer boundary.
Other CLI registrations that also leave out the finalizer signal (for example goCommands.ts). They have the same pattern but are outside the scope of #4640.

Contribution scope: an ordinary fix within existing boundaries, so no maintainer scope decision is needed.

Contract and boundary impact

Semantic owner and earliest changed stage: CLI one-shot finalization (finishAnalysisCommand, wired from registerFlutterCommands).
Contribution scope fit and linked maintainer decision: not applicable (fix within the existing lifecycle contract).
CLI and MCP/tool-catalog contract: CLI result classification changes only when SIGINT/SIGTERM is observed after the awaited cleanup retry. MCP and the tool catalog are unchanged.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: completed Evidence is kept unchanged under details.partial_observation.
Target selection, authority, upstream behavior, and cleanup details (if applicable): cleanup ownership and retry behavior are unchanged. A failed retry still reports cleanup_incomplete.
Generated metadata, package, or installation impact: none.

Before (SIGINT during the successful cleanup retry, exit 130):

{ "operation": "inspect_dart_aot", "confidence": "observed", "normalized_result": { "abi": "arm64-v8a", "coverage": "complete", "...": "..." }, "...": "..." }

After (same trigger, exit 130):

{
  "code": "cancelled",
  "details": {
    "operation": "inspect_dart_aot",
    "partial_observation": {
      "operation": "inspect_dart_aot",
      "subject": { "local_path": "<apk>" },
      "confidence": "observed",
      "normalized_result": { "abi": "arm64-v8a", "coverage": "complete", "...": "..." }
    }
  }
}

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/cli/flutterCli.test.ts, one case for each command. It registers the real Flutter commands on an incur CLI with a real FlutterBuildProvider and an APK built from the authored aotFixtureImage(). The first snapshot-root removal is made to fail once with EACCES. During the close-time retry, the test calls the SIGINT listener installed by withCommandCancellation (it does not send a process signal), then performs the real rm. For each command the test checks three runs:
1. Control, no interrupt: successful Evidence, exit code unchanged.
2. Interrupt during a successful retry: exit code 130, code: "cancelled", and partial_observation whose normalized_result equals the control's.
3. Interrupt during a retry that also fails: cleanup_incomplete with the retained root and the completed Evidence.
Each run asserts that the same root was removed twice.
Base reproduction or other evidence (executed): on unmodified origin/main (ae029fe2f), with src/cli/flutterCommands.ts restored from main, the new test fails at the cancellation assertion for both commands. The interrupted run returned the full successful Evidence object (operation, normalized_result, subject, ...) instead of code: "cancelled". Exit code 130 was already asserted and passed, matching the issue. Result: 2 failed | 3 passed (5).
User-visible CLI/MCP output (if applicable): see the before/after JSON above.
Remaining proof gaps: the interrupt is delivered by calling the registered SIGINT listener in-process. A real OS signal through the built launcher was not run. MCP cancellation delivery was not exercised. Only macOS arm64 was run. #4717 is not addressed here.

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/flutterCli.test.ts on main's source: 2 failed | 3 passed (5). These are the two new cases.
npm run test:focused -- tests/boundary/cli/flutterCli.test.ts tests/boundary/filesystem/flutterBuildProvider.test.ts tests/boundary/filesystem/flutterElfNotes.test.ts tests/boundary/mcp/flutterMcp.test.ts tests/boundary/mcp/dartAotSummaryMcp.test.ts tests/acceptance/flutterAnalysis.test.ts src/application/flutter/FlutterBuildAnalysisService.test.ts src/flutter/DartElf.test.ts src/flutter/FlutterPayloadScan.test.ts with the fix: 9 files, 165 passed.
npm run test:focused -- tests/boundary/cli/flutterCli.test.ts after adding the failed-retry case: 5 passed (5).
npx tsc --noEmit -p .: exit 0.
npx oxfmt --check and npx oxlint on both changed files: exit 0.
pre-push check:fast (turbo typecheck and lint): 2 of 2 tasks successful.

Compatibility, safety, and release

Breaking changes or migration steps: none. Only interrupted runs that previously emitted success-shaped JSON with exit 130 change classification.
Real Hopper/Ghidra, browser, or OS coverage: not applicable. Pure APK parsing on macOS arm64 only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new I/O, process, or authority. Only the result classification after cleanup changes.

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/4762 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/17/2026 **Status:** ✅ Merged **Merged:** 10/17/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/flutter-cancel-during-cleanup` --- ### 📝 Commits (1) - [`ac2486c`](https://github.com/morluto/rea/commit/ac2486c2af0a51ff310524bc58746648a30ecaad) fix(flutter): report cancellation during snapshot cleanup with completed observations ### 📊 Changes **2 files changed** (+92 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/cli/flutterCommands.ts` (+2 -0) 📝 `tests/boundary/cli/flutterCli.test.ts` (+90 -1) </details> ### 📄 Description ## Summary Fixes #4640 `identify-flutter-build` and `inspect-dart-aot` printed ordinary successful Evidence when SIGINT arrived during the provider's close-time retry of a snapshot root whose first removal had failed. The process still exited 130, so the JSON and the exit status disagreed. With this change those runs return typed `cancelled` and keep the completed Evidence under `details.partial_observation`. Runs that are never interrupted still succeed. ## Problem and expected behavior Trigger: the first snapshot-root removal inside `FlutterBuildProvider.execute` fails. The provider keeps the root and returns successful inspection Evidence. `provider.close()` then retries the removal from the one-shot CLI finalizer, and SIGINT arrives during that awaited retry. Violated invariant ([own the lifecycle](https://github.com/morluto/rea/blob/main/docs/tool-design.md#own-the-lifecycle)): before returning success, a cancellable observation must check the signal after its last awaited finalization step and keep collected observations under typed cancellation. Root cause: `registerFlutterCommands` gave the command signal to the inspection but not to `finishAnalysisCommand`. The finalizer's post-close guard `if (result.ok && signal?.aborted === true)` only runs when a signal is supplied, so the abort observed after `close()` was dropped. `withCommandCancellation` still set exit code 130. ## Change and scope `src/cli/flutterCommands.ts` now passes the command signal to `finishAnalysisCommand`, which owns finalization for one-shot CLI commands. This matches the firmware, Rust and WASM registrations. The finalizer's existing ordering is unchanged: - A failed close (incomplete cleanup) is still reported first as `cleanup_incomplete` with the completed Evidence retained, so a later signal does not override an earlier physical failure. - A primary failure from the inspection keeps its own classification. Only successful results become cancellations. - The caller's abort reason stays the cancellation cause. Not included: - #4717: SIGINT during a *successful first* removal inside `FlutterBuildProvider.execute`. That path is caught by the provider's own post-cleanup guard, which builds a cancellation error without `partialObservation`. It is a separate producer boundary. - Other CLI registrations that also leave out the finalizer signal (for example `goCommands.ts`). They have the same pattern but are outside the scope of #4640. Contribution scope: an ordinary fix within existing boundaries, so no maintainer scope decision is needed. ## Contract and boundary impact - Semantic owner and earliest changed stage: CLI one-shot finalization (`finishAnalysisCommand`, wired from `registerFlutterCommands`). - Contribution scope fit and linked maintainer decision: not applicable (fix within the existing lifecycle contract). - CLI and MCP/tool-catalog contract: CLI result classification changes only when SIGINT/SIGTERM is observed after the awaited cleanup retry. MCP and the tool catalog are unchanged. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: completed Evidence is kept unchanged under `details.partial_observation`. - Target selection, authority, upstream behavior, and cleanup details (if applicable): cleanup ownership and retry behavior are unchanged. A failed retry still reports `cleanup_incomplete`. - Generated metadata, package, or installation impact: none. Before (SIGINT during the successful cleanup retry, exit 130): ```json { "operation": "inspect_dart_aot", "confidence": "observed", "normalized_result": { "abi": "arm64-v8a", "coverage": "complete", "...": "..." }, "...": "..." } ``` After (same trigger, exit 130): ```json { "code": "cancelled", "details": { "operation": "inspect_dart_aot", "partial_observation": { "operation": "inspect_dart_aot", "subject": { "local_path": "<apk>" }, "confidence": "observed", "normalized_result": { "abi": "arm64-v8a", "coverage": "complete", "...": "..." } } } } ``` 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/cli/flutterCli.test.ts`, one case for each command. It registers the real Flutter commands on an `incur` CLI with a real `FlutterBuildProvider` and an APK built from the authored `aotFixtureImage()`. The first snapshot-root removal is made to fail once with `EACCES`. During the close-time retry, the test calls the SIGINT listener installed by `withCommandCancellation` (it does not send a process signal), then performs the real `rm`. For each command the test checks three runs: 1. Control, no interrupt: successful Evidence, exit code unchanged. 2. Interrupt during a successful retry: exit code 130, `code: "cancelled"`, and `partial_observation` whose `normalized_result` equals the control's. 3. Interrupt during a retry that also fails: `cleanup_incomplete` with the retained root and the completed Evidence. Each run asserts that the same root was removed twice. - Base reproduction or other evidence (executed): on unmodified `origin/main` (`ae029fe2f`), with `src/cli/flutterCommands.ts` restored from main, the new test fails at the cancellation assertion for both commands. The interrupted run returned the full successful Evidence object (`operation`, `normalized_result`, `subject`, ...) instead of `code: "cancelled"`. Exit code 130 was already asserted and passed, matching the issue. Result: 2 failed | 3 passed (5). - User-visible CLI/MCP output (if applicable): see the before/after JSON above. - Remaining proof gaps: the interrupt is delivered by calling the registered SIGINT listener in-process. A real OS signal through the built launcher was not run. MCP cancellation delivery was not exercised. Only macOS arm64 was run. #4717 is not addressed here. 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/flutterCli.test.ts` on main's source: 2 failed | 3 passed (5). These are the two new cases. - `npm run test:focused -- tests/boundary/cli/flutterCli.test.ts tests/boundary/filesystem/flutterBuildProvider.test.ts tests/boundary/filesystem/flutterElfNotes.test.ts tests/boundary/mcp/flutterMcp.test.ts tests/boundary/mcp/dartAotSummaryMcp.test.ts tests/acceptance/flutterAnalysis.test.ts src/application/flutter/FlutterBuildAnalysisService.test.ts src/flutter/DartElf.test.ts src/flutter/FlutterPayloadScan.test.ts` with the fix: 9 files, 165 passed. - `npm run test:focused -- tests/boundary/cli/flutterCli.test.ts` after adding the failed-retry case: 5 passed (5). - `npx tsc --noEmit -p .`: exit 0. - `npx oxfmt --check` and `npx oxlint` on both changed files: exit 0. - pre-push `check:fast` (turbo typecheck and lint): 2 of 2 tasks successful. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only interrupted runs that previously emitted success-shaped JSON with exit 130 change classification. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. Pure APK parsing on macOS arm64 only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new I/O, process, or authority. Only the result classification after cleanup changes. ## 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:09 +00:00