morluto/rea

Fork 0

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

[PR #6223] [MERGED] fix(browser): keep target detail when attributing CDP failures #7210

New issue

Closed

opened 2026-11-01 21:46:10 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:10 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/6122-cdp-operation-target-detail

📝 Commits (1)

3d610c3 fix(browser): keep target detail when attributing CDP failures

📊 Changes

5 files changed (+202 additions, -25 deletions)

View changed files

📝 src/browser/CdpBrowserProvider.ts (+12 -22)
📝 src/browser/CdpElectronProvider.ts (+2 -3)
📝 src/domain/analysisErrorProjection.test.ts (+77 -0)
📝 src/domain/browserObservationError.ts (+25 -0)
📝 tests/boundary/browser/cdpBrowserProvider.discovery_authorization.test.ts (+86 -0)

📄 Description

What broke: For a refused CDP target, only inspect_web_page kept the target-specific explanation. analyze_web_bundle, observe_web_session, discover_webmcp_tools and capture_web_screenshot replaced it with generic rea doctor advice (disallowed target) or the generic refresh default (missing target).
Fix: withTargetSession now authorizes with the caller's real operation. Relabelling an error for a different operation now goes through one domain method, BrowserObservationError.forOperation, which keeps the producer's detail, remediation and other facts. The CDP browser and CDP Electron providers both use it.
Risk: Low. Error code, category, reason and retryability don't change. A relabelled error now carries the original cause chain rather than wrapping the original error.

Summary

Fixes #6122.

When a target was disallowed or missing, the four non-inspect target operations lost the Target <id> … detail that authorizeCdpTarget had already created. They still refused the target correctly. Now every target operation projects the same message and remediation as inspect_web_page. Only details.operation differs.

Problem and expected behavior

There were two causes, and both are fixed:

withTargetSession received the real operation but called authorizeTarget, which hardcoded inspect_web_page. Its origin-scope helper (scopeToTargetOrigin) also re-derived the target origin that authorizeCdpTarget had already computed.
providerError (CDP browser) and its copy in CdpElectronProvider relabelled any cross-operation BrowserObservationError as new BrowserObservationError(operation, reason, { cause }). That dropped detail, remediationAction, cleanup, diagnostics, capturedOutput and partialObservation. target_not_allowed has no per-operation default message, so the projection fell back to rea doctor.

Change and scope

src/domain/browserObservationError.ts: the class now records the producer detail separately from the derived userMessage. The new forOperation(operation) returns this for the same operation. For a different operation it returns an error that keeps detail, remediation, cleanup, cleanupPrimary, diagnostics, captured output, partial observation and the original cause. A detail-less error still gets the default message for the new operation.
src/browser/CdpBrowserProvider.ts: authorizes with operation and reuses authorizeCdpTarget's allowedOrigins when the caller gave no origin list. Removed authorizeTarget and scopeToTargetOrigin. The latter's origin === null branch was unreachable, because authorization already rejects that case.
src/browser/CdpElectronProvider.ts: uses the same forOperation relabel.
Not included: other producers that still hardcode inspect_web_page without detail (for example CdpScreenshot, CdpWebMcpDiscovery, CdpPageCapture). They keep relying on relabelling. Their default messages are now derived for the caller's operation as before. The detail-less CdpScreenshot frame-scope refusal (#5994) is out of scope.

Contribution scope: not applicable (ordinary fix within existing boundaries).

Contract and boundary impact

Semantic owner and earliest changed stage: BrowserObservationError (domain) owns cross-operation attribution. authorizeCdpTarget owns target selection and origin scope.
Contribution scope fit and linked maintainer decision: not applicable.
CLI and MCP/tool-catalog contract: no schema change. Both adapters share projectAnalysisError, so the message and remediation.action now shown for these refusals are the same through CLI and MCP.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: a relabelled error keeps its partial observation, cleanup resources and primary cause.
Target selection, authority, upstream behavior, and cleanup details: origin authorization is unchanged. Refusal still happens before any WebSocket connection or CDP command.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.

Evidence and regression coverage

Tests added or updated:
- tests/boundary/browser/cdpBrowserProvider.discovery_authorization.test.ts: the production CdpBrowserProvider runs against the loopback fake CDP discovery fixture. All 5 target operations are run with a missing target and a disallowed-page target. The test asserts the operation and reason, the exact detail message, and that each projection equals the inspect_web_page projection apart from details.operation. It also asserts zero CDP commands and zero opened connections.
- src/domain/analysisErrorProjection.test.ts: (a) relabelling a cleanup_failed error that carries detail, remediation, cleanup resources, diagnostics, a partial observation and an aggregate cause keeps every projected field, including primary_reason: target_changed. (b) A detail-less target_not_found error gets the new operation's default message. The same operation returns the same instance. A timeout keeps its code.
Base reproduction: the issue's repro test was run on base 34fd7bdf7 and reproduced the bug. Inspect printed Target disallowed-page must be an HTTP(S) page in the selected origin scope., and the other four printed Analysis could not complete. Run \rea doctor`, then try again.With onlysrc/reverted, the new boundary test fails:analyze_web_bundlewith amissingtarget reports the generic refresh default instead ofTarget missing was not found at the selected endpoint.`

User-visible output after the fix (projection from the issue repro, analyze_web_bundle):

{"code":"target_unavailable","category":"unavailable","message":"Target disallowed-page must be an HTTP(S) page in the selected origin scope.","retryable":false,"remediation":{"action":"Target disallowed-page must be an HTTP(S) page in the selected origin scope."},"details":{"operation":"analyze_web_bundle","reason":"target_not_allowed"}}

Remaining proof gaps: no real Chrome or Electron runtime was used. The CLI/MCP transports were not exercised for this refusal specifically; consistency between them comes from the shared projection, which was checked by reading the source. The Electron relabel is covered only by the existing cdpElectronProvider tests (which pass) and the domain unit tests.
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

vitest run tests/boundary/browser/cdpBrowserProvider.discovery_authorization.test.ts tests/boundary/browser/cdpElectronProvider.test.ts src/domain/analysisErrorProjection.test.ts: 100 passed.
vitest run tests/boundary/browser src/domain src/browser src/application/BrowserObservationService.test.ts --maxWorkers 4: before building, 31 failures, all in CLI/build-dependent files (SAZ, HAR, CLI inventory). After npm run build:cached, those 8 files plus tests/boundary/mcp/browserMcp.test.ts and tests/boundary/cli/browserCli.test.ts all passed.
tests/boundary/inspector/javascriptRuntime{Observation,TargetLocations}.test.ts: 4 local failures (file-location verification). The same 4 fail on unmodified base 34fd7bdf7, so they are unrelated to this change.
npm run check:fast: passed.
npm run format:check: passed.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: none. Only the loopback fake CDP fixture was used.
Package or release metadata impact: none.
Security, privacy, process, or containment review: origin authorization is unchanged. Only the refusal messages change: they now repeat the caller-supplied target ID, which inspect_web_page already did.

Author readiness

Human owner: @kaoru0822-kitauji

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.

🤖 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/6223 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/6122-cdp-operation-target-detail` --- ### 📝 Commits (1) - [`3d610c3`](https://github.com/morluto/rea/commit/3d610c36833832a75587403ac479d0ef54da5238) fix(browser): keep target detail when attributing CDP failures ### 📊 Changes **5 files changed** (+202 additions, -25 deletions) <details> <summary>View changed files</summary> 📝 `src/browser/CdpBrowserProvider.ts` (+12 -22) 📝 `src/browser/CdpElectronProvider.ts` (+2 -3) 📝 `src/domain/analysisErrorProjection.test.ts` (+77 -0) 📝 `src/domain/browserObservationError.ts` (+25 -0) 📝 `tests/boundary/browser/cdpBrowserProvider.discovery_authorization.test.ts` (+86 -0) </details> ### 📄 Description **What broke:** For a refused CDP target, only `inspect_web_page` kept the target-specific explanation. `analyze_web_bundle`, `observe_web_session`, `discover_webmcp_tools` and `capture_web_screenshot` replaced it with generic `rea doctor` advice (disallowed target) or the generic refresh default (missing target). **Fix:** `withTargetSession` now authorizes with the caller's real operation. Relabelling an error for a different operation now goes through one domain method, `BrowserObservationError.forOperation`, which keeps the producer's detail, remediation and other facts. The CDP browser and CDP Electron providers both use it. **Risk:** Low. Error code, category, reason and retryability don't change. A relabelled error now carries the original cause chain rather than wrapping the original error. ## Summary Fixes #6122. When a target was disallowed or missing, the four non-inspect target operations lost the `Target <id> …` detail that `authorizeCdpTarget` had already created. They still refused the target correctly. Now every target operation projects the same message and remediation as `inspect_web_page`. Only `details.operation` differs. ## Problem and expected behavior There were two causes, and both are fixed: 1. `withTargetSession` received the real operation but called `authorizeTarget`, which hardcoded `inspect_web_page`. Its origin-scope helper (`scopeToTargetOrigin`) also re-derived the target origin that `authorizeCdpTarget` had already computed. 2. `providerError` (CDP browser) and its copy in `CdpElectronProvider` relabelled any cross-operation `BrowserObservationError` as `new BrowserObservationError(operation, reason, { cause })`. That dropped `detail`, `remediationAction`, `cleanup`, `diagnostics`, `capturedOutput` and `partialObservation`. `target_not_allowed` has no per-operation default message, so the projection fell back to `rea doctor`. ## Change and scope - `src/domain/browserObservationError.ts`: the class now records the producer `detail` separately from the derived `userMessage`. The new `forOperation(operation)` returns `this` for the same operation. For a different operation it returns an error that keeps detail, remediation, cleanup, cleanupPrimary, diagnostics, captured output, partial observation and the original `cause`. A detail-less error still gets the default message for the new operation. - `src/browser/CdpBrowserProvider.ts`: authorizes with `operation` and reuses `authorizeCdpTarget`'s `allowedOrigins` when the caller gave no origin list. Removed `authorizeTarget` and `scopeToTargetOrigin`. The latter's `origin === null` branch was unreachable, because authorization already rejects that case. - `src/browser/CdpElectronProvider.ts`: uses the same `forOperation` relabel. - Not included: other producers that still hardcode `inspect_web_page` without detail (for example `CdpScreenshot`, `CdpWebMcpDiscovery`, `CdpPageCapture`). They keep relying on relabelling. Their default messages are now derived for the caller's operation as before. The detail-less `CdpScreenshot` frame-scope refusal (#5994) is out of scope. Contribution scope: not applicable (ordinary fix within existing boundaries). ## Contract and boundary impact - Semantic owner and earliest changed stage: `BrowserObservationError` (domain) owns cross-operation attribution. `authorizeCdpTarget` owns target selection and origin scope. - Contribution scope fit and linked maintainer decision: not applicable. - CLI and MCP/tool-catalog contract: no schema change. Both adapters share `projectAnalysisError`, so the `message` and `remediation.action` now shown for these refusals are the same through CLI and MCP. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: a relabelled error keeps its partial observation, cleanup resources and primary cause. - Target selection, authority, upstream behavior, and cleanup details: origin authorization is unchanged. Refusal still happens before any WebSocket connection or CDP command. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/browser/cdpBrowserProvider.discovery_authorization.test.ts`: the production `CdpBrowserProvider` runs against the loopback fake CDP discovery fixture. All 5 target operations are run with a `missing` target and a `disallowed-page` target. The test asserts the operation and reason, the exact detail message, and that each projection equals the `inspect_web_page` projection apart from `details.operation`. It also asserts zero CDP commands and zero opened connections. - `src/domain/analysisErrorProjection.test.ts`: (a) relabelling a `cleanup_failed` error that carries detail, remediation, cleanup resources, diagnostics, a partial observation and an aggregate cause keeps every projected field, including `primary_reason: target_changed`. (b) A detail-less `target_not_found` error gets the new operation's default message. The same operation returns the same instance. A `timeout` keeps its code. - Base reproduction: the issue's repro test was run on base `34fd7bdf7` and reproduced the bug. Inspect printed `Target disallowed-page must be an HTTP(S) page in the selected origin scope.`, and the other four printed `Analysis could not complete. Run \`rea doctor\`, then try again.` With only `src/` reverted, the new boundary test fails: `analyze_web_bundle` with a `missing` target reports the generic refresh default instead of `Target missing was not found at the selected endpoint.` - User-visible output after the fix (projection from the issue repro, `analyze_web_bundle`): ```json {"code":"target_unavailable","category":"unavailable","message":"Target disallowed-page must be an HTTP(S) page in the selected origin scope.","retryable":false,"remediation":{"action":"Target disallowed-page must be an HTTP(S) page in the selected origin scope."},"details":{"operation":"analyze_web_bundle","reason":"target_not_allowed"}} ``` - Remaining proof gaps: no real Chrome or Electron runtime was used. The CLI/MCP transports were not exercised for this refusal specifically; consistency between them comes from the shared projection, which was checked by reading the source. The Electron relabel is covered only by the existing `cdpElectronProvider` tests (which pass) and the domain unit tests. - [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 - `vitest run tests/boundary/browser/cdpBrowserProvider.discovery_authorization.test.ts tests/boundary/browser/cdpElectronProvider.test.ts src/domain/analysisErrorProjection.test.ts`: 100 passed. - `vitest run tests/boundary/browser src/domain src/browser src/application/BrowserObservationService.test.ts --maxWorkers 4`: before building, 31 failures, all in CLI/build-dependent files (SAZ, HAR, CLI inventory). After `npm run build:cached`, those 8 files plus `tests/boundary/mcp/browserMcp.test.ts` and `tests/boundary/cli/browserCli.test.ts` all passed. - `tests/boundary/inspector/javascriptRuntime{Observation,TargetLocations}.test.ts`: 4 local failures (file-location verification). The same 4 fail on unmodified base `34fd7bdf7`, so they are unrelated to this change. - `npm run check:fast`: passed. - `npm run format:check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: none. Only the loopback fake CDP fixture was used. - Package or release metadata impact: none. - Security, privacy, process, or containment review: origin authorization is unchanged. Only the refusal messages change: they now repeat the caller-supplied target ID, which `inspect_web_page` already did. ## Author readiness Human owner: @kaoru0822-kitauji - [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. 🤖 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-11-01 21:46:10 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-03 13:39:53 +00:00

[PR #7210] feat(javascript): retain source-proven main-to-renderer IPC sends #7720

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

No description provided.

Rows
Columns