morluto/rea

Fork 0

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

[PR #4527] [MERGED] fix(javascript): keep invalid V8 script events out of policy filtering #6286

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:13:18 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/4376-v8-invalid-script-exclusions

📝 Commits (1)

ad78a7a fix(javascript): keep invalid V8 script events out of policy filtering

📊 Changes

2 files changed (+128 additions, -17 deletions)

View changed files

📝 src/domain/javascript/javascriptRuntimeReconciliationCaptureParsing.ts (+39 -17)
📝 tests/boundary/browser/cdpScriptMetadata.test.ts (+89 -0)

📄 Description

Summary

Fixes #4376.

When a V8 Inspector capture is adapted for runtime reconciliation, invalid script events (Debugger.scriptFailedToParse, or Debugger.scriptParsed without a string url) were folded into one out_of_target_scope exclusion and marked scripts as policy filtered. They now keep their invalid_protocol_value reason and mark scripts unavailable, the same way every CDP browser and Electron producer already reports them.

Problem and expected behavior

normalizeV8Inspection in src/domain/javascript/javascriptRuntimeReconciliationCaptureParsing.ts summed both producer counters (unsupported_location and invalid_protocol_value) into a single { section: "scripts", reason: "out_of_target_scope" } entry, and added scripts to policy_filtered_sections whenever that sum was non-zero. With three invalid events and no denied locations, the parsed completeness was:

{"excluded":[{"section":"scripts","reason":"out_of_target_scope","count":3}],
 "policy_filtered_sections":["scripts"],"status":"policy_filtered"}

BrowserCompleteness already has an invalid_protocol_value reason, and the shared ledger (CdpCaptureCompleteness.exclude()) treats it as unavailable data rather than a policy decision. A script that failed to parse was not filtered by any target scope.

Change and scope

normalizeV8Inspection now routes each counter the way CdpCaptureCompleteness.exclude() routes the same reasons:

unsupported_location keeps the out_of_target_scope exclusion and still marks scripts policy filtered.
invalid_protocol_value emits its own invalid_protocol_value exclusion and adds scripts to unavailable_sections, as CdpCaptureEventHandlers.ts and CdpElectronScriptEvents.ts do for scripts without a scriptId.

Not included: the out_of_target_scope label for unsupported_location is unchanged. The issue only covers invalid protocol values, and inspectorExclusionKey already collapses every location denial into that bucket on purpose.

Contribution scope: ordinary fix within existing boundaries; not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: V8 capture normalization inside runtime reconciliation capture parsing.
Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing boundaries).
CLI and MCP/tool-catalog contract: no schema change. For V8 captures with invalid script events, the internal normalized completeness now reports invalid_protocol_value, scripts in unavailable_sections, and status attach_limited instead of policy_filtered (when there are no denied locations or truncation).
Provider, bridge, target-format, or platform compatibility: none. The V8 producer is unchanged.
Evidence, artifact, provenance, or reconstruction contract: no change to reconciliation output. Checked by running the pipeline on origin/main and on this branch (finalizer, Evidence, parseRuntimeCaptures, projectRuntimeCaptures) for 0 and 3 invalid scripts, including truncated and dropped-event variants. evidence_id and captureSha256 were identical in all cases, and the runtime projection was byte-identical apart from the completeness fields. Downstream readers only use conditions.includes("truncated"), unavailable_sections.length > 0 and status === "complete_within_window", none of which change for V8: frames, script sources and workers are always unavailable, and V8 scriptsCompleteWithinScope is always false.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.

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: Inspector script exclusions in tests/boundary/browser/cdpScriptMetadata.test.ts. Both tests drive the real V8InspectorProvider against the loopback fake Inspector, then parse the resulting Evidence:
- keeps invalid protocol scripts out of policy filtering: one scriptFailedToParse and one scriptParsed without url.
- reports unsupported and invalid script exclusions separately: one scriptFailedToParse and one data: script, so both counters are non-zero.
  Each test first asserts the producer counts, so a failure can only come from the adapter mapping.
Base reproduction: on main at fe2a398dd both new tests fail. The first gets out_of_target_scope, count 2, policy_filtered_sections: ["scripts"], status policy_filtered. The second gets one merged out_of_target_scope entry with count 2. The issue's repro.mts printed the same lumped exclusion on main and prints invalid_protocol_value / attach_limited on this branch.
Mutation check: six single-line mutations of the fix (policy keyed on the sum, policy keyed on the invalid count, no unavailable routing, invalid relabeled out_of_target_scope, each exclusion entry dropped) each failed at least one new test.
User-visible CLI/MCP output: not applicable; the changed fields are internal normalized completeness.
Remaining proof gaps: no live Node or Electron Inspector run; the fake Inspector replays real protocol event shapes. Truncated plus invalid captures were checked by hand (status truncated, no policy_filtered) but have no dedicated test.

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

Node 24.18.0 (.nvmrc), macOS arm64. Branch rebased onto main at 6d2eeeaed; the base reproduction ran on fe2a398dd (no intervening commits touch the changed or producer files).

npm run test:focused -- tests/boundary/browser/cdpScriptMetadata.test.ts: 17 passed (the 2 new tests failed on fe2a398dd before the fix).
npm run check:changed after rebase: typecheck and lint clean, 563 test files, 8701 passed, 13 skipped.
npm run check: exit 0, 6/6 tasks; no lint warnings in the changed files.
npm run test:focused on the neighboring Inspector and reconciliation suites (12 files): 118 passed, 4 failed. The 4 failures (javascriptRuntimeTargetLocations.test.ts "interprets entry#hash.js / entry%percent.js / entry%23hash.js as a literal Node discovery path on POSIX" and javascriptRuntimeObservation.test.ts "includes targets outside the old caller root filter") fail identically on a clean origin/main checkout on this machine and do not touch completeness.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: none needed; no provider behavior changed. Tests use the fake V8 Inspector over loopback.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new target interaction, process, or network access.

Author readiness

Human owner: @amyanger

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/4527 **Author:** [@amyanger](https://github.com/amyanger) **Created:** 10/16/2026 **Status:** ✅ Merged **Merged:** 10/16/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `fix/4376-v8-invalid-script-exclusions` --- ### 📝 Commits (1) - [`ad78a7a`](https://github.com/morluto/rea/commit/ad78a7a4f7f3e14392deba507054642eb0afb1fa) fix(javascript): keep invalid V8 script events out of policy filtering ### 📊 Changes **2 files changed** (+128 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptRuntimeReconciliationCaptureParsing.ts` (+39 -17) 📝 `tests/boundary/browser/cdpScriptMetadata.test.ts` (+89 -0) </details> ### 📄 Description ## Summary Fixes #4376. When a V8 Inspector capture is adapted for runtime reconciliation, invalid script events (`Debugger.scriptFailedToParse`, or `Debugger.scriptParsed` without a string `url`) were folded into one `out_of_target_scope` exclusion and marked `scripts` as policy filtered. They now keep their `invalid_protocol_value` reason and mark `scripts` unavailable, the same way every CDP browser and Electron producer already reports them. ## Problem and expected behavior `normalizeV8Inspection` in `src/domain/javascript/javascriptRuntimeReconciliationCaptureParsing.ts` summed both producer counters (`unsupported_location` and `invalid_protocol_value`) into a single `{ section: "scripts", reason: "out_of_target_scope" }` entry, and added `scripts` to `policy_filtered_sections` whenever that sum was non-zero. With three invalid events and no denied locations, the parsed completeness was: ```json {"excluded":[{"section":"scripts","reason":"out_of_target_scope","count":3}], "policy_filtered_sections":["scripts"],"status":"policy_filtered"} ``` `BrowserCompleteness` already has an `invalid_protocol_value` reason, and the shared ledger (`CdpCaptureCompleteness.exclude()`) treats it as unavailable data rather than a policy decision. A script that failed to parse was not filtered by any target scope. ## Change and scope `normalizeV8Inspection` now routes each counter the way `CdpCaptureCompleteness.exclude()` routes the same reasons: - `unsupported_location` keeps the `out_of_target_scope` exclusion and still marks `scripts` policy filtered. - `invalid_protocol_value` emits its own `invalid_protocol_value` exclusion and adds `scripts` to `unavailable_sections`, as `CdpCaptureEventHandlers.ts` and `CdpElectronScriptEvents.ts` do for scripts without a `scriptId`. Not included: the `out_of_target_scope` label for `unsupported_location` is unchanged. The issue only covers invalid protocol values, and `inspectorExclusionKey` already collapses every location denial into that bucket on purpose. Contribution scope: ordinary fix within existing boundaries; not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: V8 capture normalization inside runtime reconciliation capture parsing. - Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing boundaries). - CLI and MCP/tool-catalog contract: no schema change. For V8 captures with invalid script events, the internal normalized completeness now reports `invalid_protocol_value`, `scripts` in `unavailable_sections`, and status `attach_limited` instead of `policy_filtered` (when there are no denied locations or truncation). - Provider, bridge, target-format, or platform compatibility: none. The V8 producer is unchanged. - Evidence, artifact, provenance, or reconstruction contract: no change to reconciliation output. Checked by running the pipeline on `origin/main` and on this branch (finalizer, Evidence, `parseRuntimeCaptures`, `projectRuntimeCaptures`) for 0 and 3 invalid scripts, including truncated and dropped-event variants. `evidence_id` and `captureSha256` were identical in all cases, and the runtime projection was byte-identical apart from the completeness fields. Downstream readers only use `conditions.includes("truncated")`, `unavailable_sections.length > 0` and `status === "complete_within_window"`, none of which change for V8: frames, script sources and workers are always unavailable, and V8 `scriptsCompleteWithinScope` is always `false`. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. 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: `Inspector script exclusions` in `tests/boundary/browser/cdpScriptMetadata.test.ts`. Both tests drive the real `V8InspectorProvider` against the loopback fake Inspector, then parse the resulting Evidence: - `keeps invalid protocol scripts out of policy filtering`: one `scriptFailedToParse` and one `scriptParsed` without `url`. - `reports unsupported and invalid script exclusions separately`: one `scriptFailedToParse` and one `data:` script, so both counters are non-zero. Each test first asserts the producer counts, so a failure can only come from the adapter mapping. - Base reproduction: on `main` at `fe2a398dd` both new tests fail. The first gets `out_of_target_scope`, count 2, `policy_filtered_sections: ["scripts"]`, status `policy_filtered`. The second gets one merged `out_of_target_scope` entry with count 2. The issue's `repro.mts` printed the same lumped exclusion on main and prints `invalid_protocol_value` / `attach_limited` on this branch. - Mutation check: six single-line mutations of the fix (policy keyed on the sum, policy keyed on the invalid count, no unavailable routing, invalid relabeled `out_of_target_scope`, each exclusion entry dropped) each failed at least one new test. - User-visible CLI/MCP output: not applicable; the changed fields are internal normalized completeness. - Remaining proof gaps: no live Node or Electron Inspector run; the fake Inspector replays real protocol event shapes. Truncated plus invalid captures were checked by hand (status `truncated`, no `policy_filtered`) but have no dedicated test. 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 Node 24.18.0 (`.nvmrc`), macOS arm64. Branch rebased onto `main` at `6d2eeeaed`; the base reproduction ran on `fe2a398dd` (no intervening commits touch the changed or producer files). - `npm run test:focused -- tests/boundary/browser/cdpScriptMetadata.test.ts`: 17 passed (the 2 new tests failed on `fe2a398dd` before the fix). - `npm run check:changed` after rebase: typecheck and lint clean, 563 test files, 8701 passed, 13 skipped. - `npm run check`: exit 0, 6/6 tasks; no lint warnings in the changed files. - `npm run test:focused` on the neighboring Inspector and reconciliation suites (12 files): 118 passed, 4 failed. The 4 failures (`javascriptRuntimeTargetLocations.test.ts` "interprets entry#hash.js / entry%percent.js / entry%23hash.js as a literal Node discovery path on POSIX" and `javascriptRuntimeObservation.test.ts` "includes targets outside the old caller root filter") fail identically on a clean `origin/main` checkout on this machine and do not touch completeness. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: none needed; no provider behavior changed. Tests use the fake V8 Inspector over loopback. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new target interaction, process, or network access. ## Author readiness Human owner: @amyanger - [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. - [ ] 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:13:18 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #6286] [CLOSED] feat(setup): add Trae CN client registration #7257

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

No description provided.

Rows
Columns