morluto/rea

Fork 0

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

[PR #4766] [MERGED] fix(flutter): keep completed inspection facts when cancelled during snapshot cleanup #6399

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/flutter-provider-cancel-partial

📝 Commits (1)

786fe5d fix(flutter): keep completed inspection facts when cancelled during snapshot cleanup

📊 Changes

4 files changed (+95 additions, -22 deletions)

View changed files

📝 src/application/AnalysisProvider.ts (+27 -3)
📝 src/application/ProviderOperationExecution.ts (+10 -18)
📝 src/flutter/FlutterBuildProvider.ts (+16 -1)
📝 tests/boundary/filesystem/flutterBuildProvider.test.ts (+42 -0)

📄 Description

Summary

Fixes #4717

When SIGINT arrived during the first, successful snapshot-root removal, identify-flutter-build and inspect-dart-aot returned a typed cancelled error with cleanup complete but no details.partial_observation. The inspection had already finished, and its facts were dropped. With this change the cancellation keeps the completed Evidence, identical to an uninterrupted run, along with the caller's abort reason.

Problem and expected behavior

Trigger: the caller signal is aborted while FlutterBuildProvider.execute awaits discardRoot, and that removal succeeds.

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

Root cause: after cleanup, the provider's unconditional if (signal?.aborted) branch replaced the completed AnalysisExecution with new AnalysisCancelledError(operation, { cause: signal.reason }). That error had no partialObservation, so the shared CLI/MCP Evidence assembly never saw the facts. The shared late-abort path in executeProviderOperation (completeProviderEvidence) keeps facts, but it only runs when the provider returns success.

Change and scope

FlutterBuildProvider.execute: the post-cleanup cancellation keeps cause: signal.reason. When the inspection succeeded, it also attaches the completed result as partialObservation.
The Evidence composition that was inlined in executeProviderOperation moves to createAnalysisExecutionEvidence in src/application/AnalysisProvider.ts, next to createAnalysisExecution. The success path and the provider's retained cancellation now build Evidence through one composer, so partial_observation equals the control Evidence, evidence_id included. executeProviderOperation behaves the same as before.
If the inspection had already failed, the error is still classified as before (cancellation). This change does not alter that classification.

This complements #4762, which owns src/cli/flutterCommands.ts and covers SIGINT during the close-time retry after a failed first removal (#4640). This PR does not touch that file. The two fixes are independent: this one handles the producer boundary inside the provider, and #4762 handles the CLI finalizer signal.

Not included: preserving an earlier non-cancellation primary failure ahead of a later abort in this branch, and the duplicated "observed" confidence literal that the provider now shares with FlutterBuildAnalysisService. Both are noted under remaining gaps.

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: FlutterBuildProvider.execute post-cleanup cancellation, plus the shared execution-to-Evidence composer (an extraction with no behavior change).
Contribution scope fit and linked maintainer decision: not applicable (fix within the existing lifecycle contract).
CLI and MCP/tool-catalog contract: cancelled identify_flutter_build / inspect_dart_aot results now carry details.partial_observation when the inspection had completed. The tool catalog and schemas are unchanged.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: the retained Evidence equals the uninterrupted Evidence. Other executeProviderOperation consumers are unchanged.
Target selection, authority, upstream behavior, and cleanup details (if applicable): snapshot cleanup ownership, ordering, and retry are unchanged.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Before (SIGINT during the successful first removal):

{ "code": "cancelled", "details": { "operation": "inspect_dart_aot" } }

After (same trigger):

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

(The JSON shapes come from reading the source and the test assertions. I did not capture this CLI output from a real SIGINT run.)

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

Tests added or updated: tests/boundary/filesystem/flutterBuildProvider.test.ts, %s keeps completed Evidence when cancelled during successful snapshot removal, for both operations. It uses a synthetic aotFixtureImage() APK. An uninterrupted control goes through FlutterBuildAnalysisService. The interrupted run injects removeRoot, which awaits the real recursive rm and then aborts with a caller reason. The test asserts a typed AnalysisCancelledError with that exact cause, partialObservation equal to the control Evidence, exactly one removal, a removed root (ENOENT), and a no-op close().
Base reproduction: on origin/main 9c56d09c0, with only the test applied, both new cases failed with AssertionError: expected undefined to deeply equal { …(16) } at the partialObservation assertion. The typed cancellation and cause assertions before it passed. File result: 2 failed, 96 passed (98).
User-visible CLI/MCP output (if applicable): see the before/after above (from source, not captured from a run).
Remaining proof gaps:
- No real SIGINT run through the compiled CLI, and no MCP cancellation delivery; covered at the provider/service boundary only.
- In the post-cleanup branch, an earlier non-cancellation failure is still replaced by cancellation (unchanged behavior; the lifecycle guidance prefers preserving the primary failure).
- The provider repeats the "observed" confidence that FlutterBuildAnalysisService passes.
- macOS only; Windows/Linux not run; no real Flutter APK.

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 run test:focused -- tests/boundary/filesystem/flutterBuildProvider.test.ts on base source with the new test: 2 failed, 96 passed (98)
npm run test:focused -- tests/boundary/filesystem/flutterBuildProvider.test.ts with the fix: 98 passed (98)
npm run test:focused -- on the 11 Flutter/Dart test files (src/application/flutter/FlutterBuildAnalysisService.test.ts, src/contracts/toolEffects.test.ts, src/flutter/DartElf.test.ts, src/flutter/FlutterPayloadScan.test.ts, tests/acceptance/flutterAnalysis.test.ts, tests/acceptance/applications/mobileFrameworkConventions.test.ts, tests/boundary/cli/flutterCli.test.ts, tests/boundary/filesystem/flutterBuildProvider.test.ts, tests/boundary/filesystem/flutterElfNotes.test.ts, tests/boundary/mcp/dartAotSummaryMcp.test.ts, tests/boundary/mcp/flutterMcp.test.ts): 11 files, 172 passed
npm run test:focused -- on 10 other executeProviderOperation consumer files (binary layout, PE resources, Electron fuse, EVM, Go, Rust and WASM services, lateAbortApplicationEvidenceMcp, providerCompletion conformance, goBinaryMcp): 10 files, 70 passed
npx tsc --noEmit -p .: exit 0
npx oxfmt --check on the changed files: all formatted
npx oxlint on the changed files: 0 errors (existing warnings only)

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; macOS arm64 local tests only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new I/O. The retained Evidence is the same Evidence a successful run returns.

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/4766 **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-provider-cancel-partial` --- ### 📝 Commits (1) - [`786fe5d`](https://github.com/morluto/rea/commit/786fe5dc2c256f1c022914d80eae09e6fcfb3701) fix(flutter): keep completed inspection facts when cancelled during snapshot cleanup ### 📊 Changes **4 files changed** (+95 additions, -22 deletions) <details> <summary>View changed files</summary> 📝 `src/application/AnalysisProvider.ts` (+27 -3) 📝 `src/application/ProviderOperationExecution.ts` (+10 -18) 📝 `src/flutter/FlutterBuildProvider.ts` (+16 -1) 📝 `tests/boundary/filesystem/flutterBuildProvider.test.ts` (+42 -0) </details> ### 📄 Description ## Summary Fixes #4717 When SIGINT arrived during the first, successful snapshot-root removal, `identify-flutter-build` and `inspect-dart-aot` returned a typed `cancelled` error with cleanup complete but no `details.partial_observation`. The inspection had already finished, and its facts were dropped. With this change the cancellation keeps the completed Evidence, identical to an uninterrupted run, along with the caller's abort reason. ## Problem and expected behavior Trigger: the caller signal is aborted while `FlutterBuildProvider.execute` awaits `discardRoot`, and that removal succeeds. Violated invariant ([own the lifecycle](https://github.com/morluto/rea/blob/main/docs/tool-design.md#own-the-lifecycle)): after the last awaited finalization step, a cancellable observation must check the signal and keep its collected observations under typed cancellation. Root cause: after cleanup, the provider's unconditional `if (signal?.aborted)` branch replaced the completed `AnalysisExecution` with `new AnalysisCancelledError(operation, { cause: signal.reason })`. That error had no `partialObservation`, so the shared CLI/MCP Evidence assembly never saw the facts. The shared late-abort path in `executeProviderOperation` (`completeProviderEvidence`) keeps facts, but it only runs when the provider returns success. ## Change and scope - `FlutterBuildProvider.execute`: the post-cleanup cancellation keeps `cause: signal.reason`. When the inspection succeeded, it also attaches the completed result as `partialObservation`. - The Evidence composition that was inlined in `executeProviderOperation` moves to `createAnalysisExecutionEvidence` in `src/application/AnalysisProvider.ts`, next to `createAnalysisExecution`. The success path and the provider's retained cancellation now build Evidence through one composer, so `partial_observation` equals the control Evidence, `evidence_id` included. `executeProviderOperation` behaves the same as before. - If the inspection had already failed, the error is still classified as before (cancellation). This change does not alter that classification. This complements #4762, which owns `src/cli/flutterCommands.ts` and covers SIGINT during the close-time *retry* after a failed first removal (#4640). This PR does not touch that file. The two fixes are independent: this one handles the producer boundary inside the provider, and #4762 handles the CLI finalizer signal. Not included: preserving an earlier non-cancellation primary failure ahead of a later abort in this branch, and the duplicated `"observed"` confidence literal that the provider now shares with `FlutterBuildAnalysisService`. Both are noted under remaining gaps. 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: `FlutterBuildProvider.execute` post-cleanup cancellation, plus the shared execution-to-Evidence composer (an extraction with no behavior change). - Contribution scope fit and linked maintainer decision: not applicable (fix within the existing lifecycle contract). - CLI and MCP/tool-catalog contract: cancelled `identify_flutter_build` / `inspect_dart_aot` results now carry `details.partial_observation` when the inspection had completed. The tool catalog and schemas are unchanged. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: the retained Evidence equals the uninterrupted Evidence. Other `executeProviderOperation` consumers are unchanged. - Target selection, authority, upstream behavior, and cleanup details (if applicable): snapshot cleanup ownership, ordering, and retry are unchanged. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Before (SIGINT during the successful first removal): ```json { "code": "cancelled", "details": { "operation": "inspect_dart_aot" } } ``` After (same trigger): ```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", "...": "..." } } } } ``` (The JSON shapes come from reading the source and the test assertions. I did not capture this CLI output from a real SIGINT run.) 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. - [ ] 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/flutterBuildProvider.test.ts`, `%s keeps completed Evidence when cancelled during successful snapshot removal`, for both operations. It uses a synthetic `aotFixtureImage()` APK. An uninterrupted control goes through `FlutterBuildAnalysisService`. The interrupted run injects `removeRoot`, which awaits the real recursive `rm` and then aborts with a caller reason. The test asserts a typed `AnalysisCancelledError` with that exact `cause`, `partialObservation` equal to the control Evidence, exactly one removal, a removed root (`ENOENT`), and a no-op `close()`. - Base reproduction: on `origin/main` 9c56d09c0, with only the test applied, both new cases failed with `AssertionError: expected undefined to deeply equal { …(16) }` at the `partialObservation` assertion. The typed cancellation and cause assertions before it passed. File result: 2 failed, 96 passed (98). - User-visible CLI/MCP output (if applicable): see the before/after above (from source, not captured from a run). - Remaining proof gaps: - No real SIGINT run through the compiled CLI, and no MCP cancellation delivery; covered at the provider/service boundary only. - In the post-cleanup branch, an earlier non-cancellation failure is still replaced by cancellation (unchanged behavior; the lifecycle guidance prefers preserving the primary failure). - The provider repeats the `"observed"` confidence that `FlutterBuildAnalysisService` passes. - macOS only; Windows/Linux not run; no real Flutter APK. 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 run test:focused -- tests/boundary/filesystem/flutterBuildProvider.test.ts` on base source with the new test: 2 failed, 96 passed (98) - `npm run test:focused -- tests/boundary/filesystem/flutterBuildProvider.test.ts` with the fix: 98 passed (98) - `npm run test:focused --` on the 11 Flutter/Dart test files (`src/application/flutter/FlutterBuildAnalysisService.test.ts`, `src/contracts/toolEffects.test.ts`, `src/flutter/DartElf.test.ts`, `src/flutter/FlutterPayloadScan.test.ts`, `tests/acceptance/flutterAnalysis.test.ts`, `tests/acceptance/applications/mobileFrameworkConventions.test.ts`, `tests/boundary/cli/flutterCli.test.ts`, `tests/boundary/filesystem/flutterBuildProvider.test.ts`, `tests/boundary/filesystem/flutterElfNotes.test.ts`, `tests/boundary/mcp/dartAotSummaryMcp.test.ts`, `tests/boundary/mcp/flutterMcp.test.ts`): 11 files, 172 passed - `npm run test:focused --` on 10 other `executeProviderOperation` consumer files (binary layout, PE resources, Electron fuse, EVM, Go, Rust and WASM services, `lateAbortApplicationEvidenceMcp`, `providerCompletion` conformance, `goBinaryMcp`): 10 files, 70 passed - `npx tsc --noEmit -p .`: exit 0 - `npx oxfmt --check` on the changed files: all formatted - `npx oxlint` on the changed files: 0 errors (existing warnings only) ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; macOS arm64 local tests only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new I/O. The retained Evidence is the same Evidence a successful run returns. ## 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:10 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #6411] [MERGED] fix(lifecycle): reject session work once close has started #7340

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

No description provided.

Rows
Columns