[PR #4406] [MERGED] fix(browser): preserve cleanup failure diagnostics #6217

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4406
Author: @N0zoM1z0
Created: 10/16/2026
Status: ✅ Merged
Merged: 10/16/2026
Merged by: @N0zoM1z0

Base: main ← Head: fix/browser-cleanup-diagnostics-4403


📝 Commits (2)

  • 267aa22 fix(browser): preserve cleanup failure diagnostics
  • 690c2e5 test(browser): align cleanup diagnostics with owner observations

📊 Changes

4 files changed (+202 additions, -58 deletions)

View changed files

📝 src/browser/PlaywrightElectronActiveProvider.test.ts (+92 -21)
📝 src/browser/PlaywrightElectronActiveProvider.ts (+10 -17)
📝 src/domain/analysisErrorProjection.ts (+24 -20)
📝 tests/conformance/contracts/analysisErrorProjection.test.ts (+76 -0)

📄 Description

Summary

Fixes #4403. Refs #4227 and #4282.

A completed Linux Electron capture could return cleanup_incomplete with execution_failure: unreadable_output and a generic cleanup reason, hiding the observer’s actual failure. Preserve the unavailable-lineage reason, distinguish an unobserved lineage, and classify cleanup failure as execution_failure. Direct typed observation failures now retain primary_reason, matching aggregated failures.

Change and scope

The Electron cleanup owner handles unavailable lineage once across platforms, preserving existing termination decisions and root-cleanup failures. The shared error projection uses an exhaustive browser-reason map, removing the misleading default for future reasons. CLI and MCP continue using that single projection; schemas and Evidence retention are unchanged.

Ordinary correction within existing authority; maintainer scope decision not applicable. Marker-erasing Chromium helper ownership remains unverified (#4282); #4346 separately addresses the Linux exit race. No new cleanup authority or recovery operation.

Contract and boundary impact

  • Semantic owners: Electron process cleanup and shared analysis-error projection.
  • CLI/MCP: existing cleanup_incomplete, cleanup_reason, execution_failure, and primary_reason fields; readable retained observations are not labeled unreadable.
  • Evidence/provenance: completed actions and runtime observations remain in partial_observation; resource identities and uncertainty remain visible.
  • Provider/platform: real Linux Electron 44.7.0; Windows cleanup decisions exercised through its narrow injected host, not a real Windows runtime.
  • Target selection, permissions, instrumentation, package metadata and installation: unchanged.

Evidence and regression coverage

  • On base 2c7583599b9ac1c847bd77b1940930fd8d4594c7, seven new regression cases failed; verified cleanup and existing contract controls passed.
  • Review follow-up: reproduced the two stale colocated assertions locally and in the initial Source tests CI log. Updated the existing owner tests, injected the unavailable observation to remove dependence on a host PID, consolidated duplicate cleanup tests, and added unobserved-lineage and root/descendant termination-failure controls. Production code and real-provider measurements are unchanged by this test-only follow-up. Latest-head CI (690c2e5d29) passed: 25 successful / 12 intentionally skipped checks. Codex re-review found no remaining findings; Greptile 5/5; both review threads resolved.
  • Real stdio MCP + official Electron in a disposable Ubuntu 24.04 container with authenticated Xvfb: all three actions completed and the IPC file effect was checked independently. The unrelated marked process survived. Capture still returns cleanup_incomplete, now with the observer’s specific reason and execution_failure: execution_failure.
  • Direct and AggregateError-wrapped cancellation, timeout and disconnection retain their typed primary reasons. Unavailable/unobserved lineage never authorizes Windows termination; verified cleanup and termination errors retain their prior outcomes.
  • 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 -- src/browser/PlaywrightElectronActiveProvider.test.ts src/domain/analysisErrorProjection.test.ts tests/conformance/contracts/analysisErrorProjection.test.ts tests/boundary/process/electronLaunchRejections.test.ts tests/process-global/browserScenarioCleanup.test.ts tests/boundary/mcp/toolResult.test.ts — 108 passed across six files.
  • npm run check:fast — typecheck and lint passed after review changes; final source rerun (the two owner files + conformance contract) passed 71 tests.
  • npm run docs:check — generated documentation/schema/license checks and five README tests passed.
  • git diff --cached --check — passed.
  • Real stdio MCP/Electron probe plus two paired runs per revision — expected incomplete cleanup, precise diagnostics, completed actions/file effect, and unaffected control verified.

Performance: base/fixed medians across two equivalent fresh-process/profile captures were 8.28/7.99 s elapsed, 7.75/7.34 s CPU, and 1.24/1.26 GiB sampled family RSS. Input + full MCP-result proxy (o200k_base) was 8,257/8,273 tokens; the additional 16 tokens carry the restored diagnostic. No speedup claim from two runs. Node compile cache disabled, OS cache uncontrolled/warm; actual agent billing, initialization traffic and Xvfb/container baseline are unmeasured. Container capped at 4 GiB/two CPUs; heavy local checks ran serially, maximum sampled passing check RSS 4.45 GiB.

No real Windows/macOS claim. Successful cleanup of marker-erasing Linux helpers remains unproven and is not a claim of this PR. Raw local logs and full responses are retained under /tmp/rea-4403-browser-cleanup-20261016/.

Author readiness

Human owner: @N0zoM1z0

The human owner explicitly authorized delegated review, proxy completion of these declarations, ready publication and normal merge. The agent reviewed the diff and evidence; these declarations record that delegation and do not assert a separate manual review.

  • 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/4406 **Author:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Created:** 10/16/2026 **Status:** ✅ Merged **Merged:** 10/16/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `fix/browser-cleanup-diagnostics-4403` --- ### 📝 Commits (2) - [`267aa22`](https://github.com/morluto/rea/commit/267aa22cce7f7b7174dcdb7756d773eec50fe3af) fix(browser): preserve cleanup failure diagnostics - [`690c2e5`](https://github.com/morluto/rea/commit/690c2e5d29dd71332b8e4ee32869bbd70cf438c1) test(browser): align cleanup diagnostics with owner observations ### 📊 Changes **4 files changed** (+202 additions, -58 deletions) <details> <summary>View changed files</summary> 📝 `src/browser/PlaywrightElectronActiveProvider.test.ts` (+92 -21) 📝 `src/browser/PlaywrightElectronActiveProvider.ts` (+10 -17) 📝 `src/domain/analysisErrorProjection.ts` (+24 -20) 📝 `tests/conformance/contracts/analysisErrorProjection.test.ts` (+76 -0) </details> ### 📄 Description ## Summary Fixes #4403. Refs #4227 and #4282. A completed Linux Electron capture could return `cleanup_incomplete` with `execution_failure: unreadable_output` and a generic cleanup reason, hiding the observer’s actual failure. Preserve the unavailable-lineage reason, distinguish an unobserved lineage, and classify cleanup failure as `execution_failure`. Direct typed observation failures now retain `primary_reason`, matching aggregated failures. ## Change and scope The Electron cleanup owner handles unavailable lineage once across platforms, preserving existing termination decisions and root-cleanup failures. The shared error projection uses an exhaustive browser-reason map, removing the misleading default for future reasons. CLI and MCP continue using that single projection; schemas and Evidence retention are unchanged. Ordinary correction within existing authority; maintainer scope decision not applicable. Marker-erasing Chromium helper ownership remains unverified (#4282); #4346 separately addresses the Linux exit race. No new cleanup authority or recovery operation. ## Contract and boundary impact - Semantic owners: Electron process cleanup and shared analysis-error projection. - CLI/MCP: existing `cleanup_incomplete`, `cleanup_reason`, `execution_failure`, and `primary_reason` fields; readable retained observations are not labeled unreadable. - Evidence/provenance: completed actions and runtime observations remain in `partial_observation`; resource identities and uncertainty remain visible. - Provider/platform: real Linux Electron 44.7.0; Windows cleanup decisions exercised through its narrow injected host, not a real Windows runtime. - Target selection, permissions, instrumentation, package metadata and installation: unchanged. ## Evidence and regression coverage - On base `2c7583599b9ac1c847bd77b1940930fd8d4594c7`, seven new regression cases failed; verified cleanup and existing contract controls passed. - Review follow-up: reproduced the two stale colocated assertions locally and in the initial Source tests CI log. Updated the existing owner tests, injected the unavailable observation to remove dependence on a host PID, consolidated duplicate cleanup tests, and added unobserved-lineage and root/descendant termination-failure controls. Production code and real-provider measurements are unchanged by this test-only follow-up. Latest-head CI (690c2e5d29dd) passed: 25 successful / 12 intentionally skipped checks. Codex re-review found no remaining findings; Greptile 5/5; both review threads resolved. - Real stdio MCP + official Electron in a disposable Ubuntu 24.04 container with authenticated Xvfb: all three actions completed and the IPC file effect was checked independently. The unrelated marked process survived. Capture still returns `cleanup_incomplete`, now with the observer’s specific reason and `execution_failure: execution_failure`. - Direct and AggregateError-wrapped cancellation, timeout and disconnection retain their typed primary reasons. Unavailable/unobserved lineage never authorizes Windows termination; verified cleanup and termination errors retain their prior outcomes. - [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 - `npm run test:focused -- src/browser/PlaywrightElectronActiveProvider.test.ts src/domain/analysisErrorProjection.test.ts tests/conformance/contracts/analysisErrorProjection.test.ts tests/boundary/process/electronLaunchRejections.test.ts tests/process-global/browserScenarioCleanup.test.ts tests/boundary/mcp/toolResult.test.ts` — 108 passed across six files. - `npm run check:fast` — typecheck and lint passed after review changes; final source rerun (the two owner files + conformance contract) passed 71 tests. - `npm run docs:check` — generated documentation/schema/license checks and five README tests passed. - `git diff --cached --check` — passed. - Real stdio MCP/Electron probe plus two paired runs per revision — expected incomplete cleanup, precise diagnostics, completed actions/file effect, and unaffected control verified. Performance: base/fixed medians across two equivalent fresh-process/profile captures were 8.28/7.99 s elapsed, 7.75/7.34 s CPU, and 1.24/1.26 GiB sampled family RSS. Input + full MCP-result proxy (`o200k_base`) was 8,257/8,273 tokens; the additional 16 tokens carry the restored diagnostic. No speedup claim from two runs. Node compile cache disabled, OS cache uncontrolled/warm; actual agent billing, initialization traffic and Xvfb/container baseline are unmeasured. Container capped at 4 GiB/two CPUs; heavy local checks ran serially, maximum sampled passing check RSS 4.45 GiB. No real Windows/macOS claim. Successful cleanup of marker-erasing Linux helpers remains unproven and is not a claim of this PR. Raw local logs and full responses are retained under `/tmp/rea-4403-browser-cleanup-20261016/`. ## Author readiness Human owner: @N0zoM1z0 The human owner explicitly authorized delegated review, proxy completion of these declarations, ready publication and normal merge. The agent reviewed the diff and evidence; these declarations record that delegation and do not assert a separate manual review. - [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-10-31 01:12:46 +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#6217
No description provided.