[PR #5644] [MERGED] fix(observation): keep incomplete cancellation observations #7009

Closed
opened 2026-11-01 21:44:40 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

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

Base: main ← Head: cursor/cancellation-partial-observation-a5ac


📝 Commits (10+)

  • 381fcbd fix(observation): keep incomplete cancellation observations
  • 87e5199 fix(observation): drop non-JSON incomplete observations
  • c09a9cd fix(observation): validate incomplete partials without copying
  • d847710 merge(main): take non-retryable invalid-request projection
  • b7ec2f6 test(ida): expect non-retryable unresolved procedure lookups
  • 00785c0 test(observation): share incomplete-observation assertions
  • 56583f1 test(observation): keep the assertion helper out of the package
  • 134f627 fix(observation): clear non-JSON partials on cleanup primaries
  • 97abf17 fix(observation): freeze retained incomplete observations
  • fd1ed0c merge(main): take landed audit fixes

📊 Changes

15 files changed (+915 additions, -304 deletions)

View changed files

📝 src/application/BrowserObservationService.test.ts (+173 -12)
📝 src/application/BrowserObservationService.ts (+41 -53)
📝 src/application/BrowserScenarioCaptureService.test.ts (+107 -37)
📝 src/application/BrowserScenarioCaptureService.ts (+13 -11)
📝 src/application/ProviderOperationExecution.test.ts (+93 -2)
📝 src/application/ProviderOperationExecution.ts (+53 -0)
📝 src/application/WebRuntimeService.test.ts (+128 -7)
📝 src/application/WebRuntimeService.ts (+53 -59)
📝 src/application/javascript/ElectronObservationService.test.ts (+56 -42)
📝 src/application/javascript/ElectronObservationService.ts (+27 -26)
📝 src/application/javascript/JavaScriptRuntimeObservationService.test.ts (+49 -16)
📝 src/application/javascript/JavaScriptRuntimeObservationService.ts (+14 -11)
➕ src/application/ownerObservationExpectation.fixture.ts (+72 -0)
➕ src/domain/javascript/electronPageInspection.fixture.ts (+34 -0)
📝 tests/boundary/mcp/electronMcp.test.ts (+2 -28)

📄 Description

Summary

Cancelling an observation that had already retained incomplete facts was reported as unreadable_output, and the partial was dropped. The owner error now stays cancellation or timeout. A JSON partial is kept as one frozen snapshot, so a later mutation cannot make projection throw. A non-JSON partial is cleared from the returned error and from each cleanup primary that projection can select after a successful cleanup retry. An AnalysisOutputError is recorded on each cleared error, so projectAnalysisError returns the typed owner error instead of throwing ZodError.

Fixes #5364 and #5641.

Problem and expected behavior

Several observation services parsed a failure's partialObservation with the completed-result schema. When that parse failed and the error was AnalysisCancelledError, they replaced it with AnalysisOutputError and discarded the partial.

Keeping the raw partial fixed that classification, and then projectAnalysisError called jsonValueSchema.parse on values such as NaN and undefined array entries. Those calls threw. On main, the same page timeout already returned the raw partial and threw the same way. A cleanup wrapper copies the partial onto both itself and its primary, so clearing only the wrapper still threw after cleanup retry. A JSON partial that stayed mutable could become non-JSON before projection.

A completed capture still becomes Evidence. A JSON incomplete observation stays on the owner error as a frozen snapshot. A non-JSON incomplete observation is dropped and recorded as a secondary output failure. Cancellation and timeout stay primary.

Change and scope

keepIncompleteObservation in ProviderOperationExecution is the gate. It uses jsonValueValidationIssue, which checks JSON safety without copying a partial it will drop. A kept partial is replaced by freezeJsonSnapshot(jsonValueSchema.parse(partial)), which projection recognizes and does not copy again. The gate walks the returned error and every cleanupPrimary. It runs at every service return that still holds an unpromoted partial: browser scenario, web page, web bundle, Electron page, web execution, event listeners, and JavaScript runtime observation. Page timeout paths that previously returned a raw non-JSON partial use the same gate.

Shared test assertions live in ownerObservationExpectation.fixture.ts, which the production build excludes.

The branch also includes main's non-retryable invalid_request projection. Unresolved IDA procedure lookups stay invalid_request and are not marked retryable.

Follow the contribution scope. This is an ordinary failure-classification fix inside existing observation services. Maintainer scope decision: not applicable.

Not included: process-table refresh (#5206), Windows CloseHandle ownership (#4505), and the compiler-scratch race scoped to unmerged #5587 (#5616). Those remain separate.

Contract and boundary impact

  • Semantic owner and earliest changed stage: observation service projection, after the provider returns a failure and before CLI/MCP error projection.
  • Contribution scope fit and linked maintainer decision: ordinary fix; not applicable.
  • CLI and MCP/tool-catalog contract: incomplete JSON observations still project as cancelled or provider_timeout with partial_observation. Non-JSON partials are omitted. The public code is not unreadable_output and projection does not throw. No catalog or argument change.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: JSON-safe incomplete observations remain partial facts on the owner error. They are not promoted to completed Evidence. The secondary output failure records that a non-JSON partial was dropped.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): no new target launch, injection, or cleanup. Existing cleanup errors are not rewritten.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): 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

  • Tests added or updated: the five service suites keep the existing incomplete-cancellation cases, and add non-JSON partials (NaN, undefined in arrays) through projectAnalysisError. Page timeout covers both a schema-shaped inspection (network.requests: NaN) and a shapeless { requests: NaN } partial. Web execution timeout with undefined in an array stays provider_timeout. ProviderOperationExecution.test.ts covers a cleanup wrapper whose shared partial becomes non-JSON, then projects cleanup_incomplete before retry and cancelled with no partial after resolveCleanup(). It also mutates a retained array after the gate and still projects the frozen { steps: [] } snapshot. Shared assertions live in ownerObservationExpectation.fixture.ts so those checks do not exceed the duplication limit and are not published. IdaOperationFailures.test.ts expects unresolved procedure lookups to be non-retryable, matching main.
  • Base reproduction or other evidence: the reviewer's three cancelled shapes (bundle bytes: NaN, bundle observations: [undefined], page requests: NaN) project as cancelled with the partial cleared and one secondary AnalysisOutputError.
  • User-visible CLI/MCP output (if applicable): not re-executed through a live browser. Projection is the CLI/MCP boundary these tests call.
  • Remaining proof gaps: no live Chrome/Electron/Inspector session. No claim about how often startup cancellation occurs.

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:local -- the five observation service tests and src/application/ProviderOperationExecution.test.ts — 6 files, 55 tests passed.
  • Earlier the same services plus src/ida/IdaOperationFailures.test.ts — 83 tests passed.
  • npm run check:fast — typecheck and lint passed.
  • npm run jscpd — 3.495% duplicated lines, 45 lines under the 3.5% threshold.
  • tsc -p tsconfig.build.json --listFilesOnly does not list the fixture.

Compatibility, safety, and release

  • Breaking changes or migration steps: callers that treated this cancellation as unreadable_output now see cancelled and a JSON snapshot when one exists. A non-JSON partial is absent rather than thrown. Invalid requests are not retryable, matching main. No configuration migration.
  • Real Hopper/Ghidra, browser, or OS coverage: not run. These are in-process service tests.
  • Package or release metadata impact: none. The Vitest helper is a fixture and is excluded from the published build.
  • Security, privacy, process, or containment review: no new process or network behavior. Non-JSON partials are not copied into the projected error. A kept partial is one frozen JSON snapshot.

Author readiness

Human owner: @morluto

  • 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.
Open in Web Open in Cursor 

🔄 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/5644 **Author:** [@morluto](https://github.com/morluto) **Created:** 10/20/2026 **Status:** ✅ Merged **Merged:** 10/20/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `cursor/cancellation-partial-observation-a5ac` --- ### 📝 Commits (10+) - [`381fcbd`](https://github.com/morluto/rea/commit/381fcbdb8cd5cef7b4e7c4a68de7b59842f43ba4) fix(observation): keep incomplete cancellation observations - [`87e5199`](https://github.com/morluto/rea/commit/87e51999020850f7c2899326ad75b7083957f1ad) fix(observation): drop non-JSON incomplete observations - [`c09a9cd`](https://github.com/morluto/rea/commit/c09a9cd3c350312ff498837f3b95c4f7025d10fb) fix(observation): validate incomplete partials without copying - [`d847710`](https://github.com/morluto/rea/commit/d8477106c7dde097d02b5a2623881e310a8b43eb) merge(main): take non-retryable invalid-request projection - [`b7ec2f6`](https://github.com/morluto/rea/commit/b7ec2f6d6e3d73660dc8b8911a85c072e06ac74d) test(ida): expect non-retryable unresolved procedure lookups - [`00785c0`](https://github.com/morluto/rea/commit/00785c0401f8269e26e6a44b58948a75dace5fcb) test(observation): share incomplete-observation assertions - [`56583f1`](https://github.com/morluto/rea/commit/56583f1705f07bbcb4b8f685e672c3025b8906c0) test(observation): keep the assertion helper out of the package - [`134f627`](https://github.com/morluto/rea/commit/134f6271eeffd3c52fb6725229c9ee74cb7aaff1) fix(observation): clear non-JSON partials on cleanup primaries - [`97abf17`](https://github.com/morluto/rea/commit/97abf177e2bf2ac94081824e11a13877b2269934) fix(observation): freeze retained incomplete observations - [`fd1ed0c`](https://github.com/morluto/rea/commit/fd1ed0cf7bb49fe30dfb90a5023b58b173a0e3cc) merge(main): take landed audit fixes ### 📊 Changes **15 files changed** (+915 additions, -304 deletions) <details> <summary>View changed files</summary> 📝 `src/application/BrowserObservationService.test.ts` (+173 -12) 📝 `src/application/BrowserObservationService.ts` (+41 -53) 📝 `src/application/BrowserScenarioCaptureService.test.ts` (+107 -37) 📝 `src/application/BrowserScenarioCaptureService.ts` (+13 -11) 📝 `src/application/ProviderOperationExecution.test.ts` (+93 -2) 📝 `src/application/ProviderOperationExecution.ts` (+53 -0) 📝 `src/application/WebRuntimeService.test.ts` (+128 -7) 📝 `src/application/WebRuntimeService.ts` (+53 -59) 📝 `src/application/javascript/ElectronObservationService.test.ts` (+56 -42) 📝 `src/application/javascript/ElectronObservationService.ts` (+27 -26) 📝 `src/application/javascript/JavaScriptRuntimeObservationService.test.ts` (+49 -16) 📝 `src/application/javascript/JavaScriptRuntimeObservationService.ts` (+14 -11) ➕ `src/application/ownerObservationExpectation.fixture.ts` (+72 -0) ➕ `src/domain/javascript/electronPageInspection.fixture.ts` (+34 -0) 📝 `tests/boundary/mcp/electronMcp.test.ts` (+2 -28) </details> ### 📄 Description <!-- CURSOR_AGENT_PR_BODY_BEGIN --> ## Summary Cancelling an observation that had already retained incomplete facts was reported as `unreadable_output`, and the partial was dropped. The owner error now stays cancellation or timeout. A JSON partial is kept as one frozen snapshot, so a later mutation cannot make projection throw. A non-JSON partial is cleared from the returned error and from each cleanup primary that projection can select after a successful cleanup retry. An `AnalysisOutputError` is recorded on each cleared error, so `projectAnalysisError` returns the typed owner error instead of throwing `ZodError`. Fixes #5364 and #5641. ## Problem and expected behavior Several observation services parsed a failure's `partialObservation` with the completed-result schema. When that parse failed and the error was `AnalysisCancelledError`, they replaced it with `AnalysisOutputError` and discarded the partial. Keeping the raw partial fixed that classification, and then `projectAnalysisError` called `jsonValueSchema.parse` on values such as `NaN` and `undefined` array entries. Those calls threw. On main, the same page timeout already returned the raw partial and threw the same way. A cleanup wrapper copies the partial onto both itself and its primary, so clearing only the wrapper still threw after cleanup retry. A JSON partial that stayed mutable could become non-JSON before projection. A completed capture still becomes Evidence. A JSON incomplete observation stays on the owner error as a frozen snapshot. A non-JSON incomplete observation is dropped and recorded as a secondary output failure. Cancellation and timeout stay primary. ## Change and scope `keepIncompleteObservation` in `ProviderOperationExecution` is the gate. It uses `jsonValueValidationIssue`, which checks JSON safety without copying a partial it will drop. A kept partial is replaced by `freezeJsonSnapshot(jsonValueSchema.parse(partial))`, which projection recognizes and does not copy again. The gate walks the returned error and every `cleanupPrimary`. It runs at every service return that still holds an unpromoted partial: browser scenario, web page, web bundle, Electron page, web execution, event listeners, and JavaScript runtime observation. Page timeout paths that previously returned a raw non-JSON partial use the same gate. Shared test assertions live in `ownerObservationExpectation.fixture.ts`, which the production build excludes. The branch also includes main's non-retryable `invalid_request` projection. Unresolved IDA procedure lookups stay `invalid_request` and are not marked retryable. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). This is an ordinary failure-classification fix inside existing observation services. Maintainer scope decision: not applicable. Not included: process-table refresh (#5206), Windows `CloseHandle` ownership (#4505), and the compiler-scratch race scoped to unmerged #5587 (#5616). Those remain separate. ## Contract and boundary impact - Semantic owner and earliest changed stage: observation service projection, after the provider returns a failure and before CLI/MCP error projection. - Contribution scope fit and linked maintainer decision: ordinary fix; not applicable. - CLI and MCP/tool-catalog contract: incomplete JSON observations still project as `cancelled` or `provider_timeout` with `partial_observation`. Non-JSON partials are omitted. The public code is not `unreadable_output` and projection does not throw. No catalog or argument change. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: JSON-safe incomplete observations remain partial facts on the owner error. They are not promoted to completed Evidence. The secondary output failure records that a non-JSON partial was dropped. - Target selection, authority, upstream behavior, and cleanup details (if applicable): no new target launch, injection, or cleanup. Existing cleanup errors are not rewritten. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): 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 - Tests added or updated: the five service suites keep the existing incomplete-cancellation cases, and add non-JSON partials (`NaN`, `undefined` in arrays) through `projectAnalysisError`. Page timeout covers both a schema-shaped inspection (`network.requests: NaN`) and a shapeless `{ requests: NaN }` partial. Web execution timeout with `undefined` in an array stays `provider_timeout`. `ProviderOperationExecution.test.ts` covers a cleanup wrapper whose shared partial becomes non-JSON, then projects `cleanup_incomplete` before retry and `cancelled` with no partial after `resolveCleanup()`. It also mutates a retained array after the gate and still projects the frozen `{ steps: [] }` snapshot. Shared assertions live in `ownerObservationExpectation.fixture.ts` so those checks do not exceed the duplication limit and are not published. `IdaOperationFailures.test.ts` expects unresolved procedure lookups to be non-retryable, matching main. - Base reproduction or other evidence: the reviewer's three cancelled shapes (bundle `bytes: NaN`, bundle `observations: [undefined]`, page `requests: NaN`) project as `cancelled` with the partial cleared and one secondary `AnalysisOutputError`. - User-visible CLI/MCP output (if applicable): not re-executed through a live browser. Projection is the CLI/MCP boundary these tests call. - Remaining proof gaps: no live Chrome/Electron/Inspector session. No claim about how often startup cancellation occurs. 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 - `npm run test:local --` the five observation service tests and `src/application/ProviderOperationExecution.test.ts` — 6 files, 55 tests passed. - Earlier the same services plus `src/ida/IdaOperationFailures.test.ts` — 83 tests passed. - `npm run check:fast` — typecheck and lint passed. - `npm run jscpd` — 3.495% duplicated lines, 45 lines under the 3.5% threshold. - `tsc -p tsconfig.build.json --listFilesOnly` does not list the fixture. ## Compatibility, safety, and release - Breaking changes or migration steps: callers that treated this cancellation as `unreadable_output` now see `cancelled` and a JSON snapshot when one exists. A non-JSON partial is absent rather than thrown. Invalid requests are not retryable, matching main. No configuration migration. - Real Hopper/Ghidra, browser, or OS coverage: not run. These are in-process service tests. - Package or release metadata impact: none. The Vitest helper is a fixture and is excluded from the published build. - Security, privacy, process, or containment review: no new process or network behavior. Non-JSON partials are not copied into the projected error. A kept partial is one frozen JSON snapshot. ## Author readiness Human owner: @morluto - [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. - [ ] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-40e947cc-28c3-58f0-b7d5-899ae4dda5ac?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a href="https://cursor.com/background-agent?bcId=bc-40e947cc-28c3-58f0-b7d5-899ae4dda5ac&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:44:40 +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#7009
No description provided.