[PR #6358] [MERGED] fix(browser): account for late script metadata #7305

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

📋 Pull Request Information

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

Base: main ← Head: fix/browser-late-script-inventory-6231


📝 Commits (2)

  • 46681bb fix(browser): account for late script metadata
  • e329736 chore: sync upstream cancellation assertion fix

📊 Changes

4 files changed (+259 additions, -12 deletions)

View changed files

📝 docs/browser-observation.md (+8 -0)
📝 src/browser/CdpPageCapture.ts (+6 -2)
📝 src/browser/CdpPageCaptureScripts.ts (+25 -10)
➕ tests/boundary/mcp/scriptInventoryRace.test.ts (+220 -0)

📄 Description

Summary

Fixes #6231. Keep passive page script metadata/source reads tied to a fixed admitted-script snapshot, and explicitly account for additional in-scope script IDs still retained at the end of capture. Incomplete inventories must not produce a false unchanged script comparison.

Problem and expected behavior

If Debugger.scriptParsed adds a second script while the initial script's source request is pending, the old producer can return scripts.total=2, one retained item, zero dropped scripts, and no script truncation. An identical pair of those incomplete outputs can then compare as unchanged. Scripts arriving during later storage or final-frame reads can also escape accounting entirely.

The fixed producer preserves the initial metadata and selected source-read pass. At the final capture boundary it counts distinct additional IDs still retained, records them as dropped script metadata, and marks scripts truncated. For one late ID the registered MCP workflow now observes total=2, retained items =1, dropped scripts =1, and compare_web_captures reports script status unknown rather than asserting equality.

Change and scope

  • Snapshot the admitted ID-to-metadata map once, retaining the original facts and identity set even if an existing ID is subsequently replaced or removed.
  • Settle newly retained IDs once after the last CDP read/frame validation, before the completeness snapshot. Repeated events for one new ID count once.
  • Do not extend source reads, add CDP commands, or claim source byte/time-budget exhaustion for this omission.
  • Document that this is accounting for final retained IDs, not a claim to reconstruct complete page history.

This is an ordinary correctness fix within the existing passive CDP observation boundary. Maintainer scope decision: not applicable; no new execution, injection, mutation, provider, or protection-recovery capability. Coordination: https://github.com/morluto/rea/issues/6231#issuecomment-6345563758 . Nearby #4014 addresses comparison identities, and #6276 addresses when coverage starts; neither supplies this producer snapshot/finalization fix. No directly overlapping public implementation was found in the inspected issue timeline and open-PR search; unpublished work cannot be ruled out.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.

Contract and boundary impact

  • Semantic owner and earliest changed stage: CdpPageCaptureScripts snapshots admitted script metadata before source reads; CdpPageCapture settles final omissions before normalizing completeness.
  • Contribution scope fit and linked maintainer decision: existing passive observation bug fix; not applicable for an additional scope decision.
  • CLI and MCP/tool-catalog contract: unchanged parameters, tools, and schemas. Existing totals, dropped-event counters, truncation, and uncertainty now describe the actual retained snapshot.
  • Provider, bridge, target-format, or platform compatibility: existing CDP provider only; no new dependencies or browser-specific protocol features.
  • Evidence, artifact, provenance, or reconstruction contract: initial source SHA/metadata remain intact; incomplete script evidence stays visibly incomplete. No reconstruction change.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): existing target/origin filtering, source opt-in, cancellation, and detach/socket ownership remain in place; no additional target commands.
  • 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): no authored catalog, package, lockfile, installation, or release metadata changes; ignored generated artifacts were validated and are not committed.

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.

The changed production path attaches to an explicitly selected CDP target and reads admitted metadata/source; it does not evaluate target JavaScript. Real-browser verification uses owned local fixtures and intentionally exercises the repository's broader existing browser workflows, separate from the authority of this fix.

Evidence and regression coverage

  • Tests added or updated: Ten deterministic cases traverse the real CdpBrowserProvider and registered MCP inspect_web_page / compare_web_captures tools against a fake CDP transport. They cover source-pass, post-source, final-frame, source-disabled, duplicate-ID, shared-metadata/different-ID, foreign-origin, existing-ID replacement, deletion-plus-late-ID, and unchanged controls.
  • Assertions consume actual producer output: total/retained/dropped accounting, truncation, no fabricated resource-budget reason, original source SHA, conservative comparison, no extra source reads, detach and socket closure, and continued MCP responsiveness.
  • Base reproduction or other evidence: Exact affected base 2127095e3b4a377abbc124ca83af46b98a2a557b, with the same regression file copied in: 7 failed, 3 passed. Failures are missing drop accounting or missing final totals. With this fix: 10/10 passed, plus the four existing source-budget/cancellation tests and six existing browser MCP tests (20 passed).
  • The deterministic interleavings are transport-controlled, not represented as a reproduced real-Chrome race. The separate real Chrome verifier is regression/integration evidence for actual browser, CLI, stdio MCP, passive/active capture, script export, cancellation, and cleanup workflows.
  • User-visible CLI/MCP output (if applicable): For the one-late-ID fixture, the actual registered-tool result has scripts.total=2, one retained item, dropped_events.scripts=1, script truncation, and script comparison status unknown. No additional source read is issued.
  • Remaining proof gaps: no exhaustive timing/browser/platform matrix, no Windows/Linux or non-Chromium run, no full repository test suite, and no assertion about unrecoverable events in older stored captures.

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

Original implementation: 46681bb87062d852667681e0d30cce4fecdb120a. Current head: e329736561b647bfd96565d3540a73faebf7412f, merging upstream 791f6328b02a29f23822fe600754fcd228bf754b. The maintainer-owned cancellation assertion fix is now included; no separate JavaScript implementation or duplicate fix was added. Relative to that upstream revision, this PR still contains only the original four browser inventory files.

On macOS, Node 24.18.0, npm 11.16.0, at the current tree:

  • npm run deps:check and npm run check:fast — passed (typecheck/lint also passed the push hook).
  • Focused regression through the existing runner — 154 passed across 6 files: JavaScript artifact reconstruction, JavaScript application cancellation/service, public error projection, CDP script capture, and the 10-case registered-MCP inventory race suite. The previously failing cancellation test now passes.
  • npm run docs:check — generated/document/license checks and all 9 README consistency tests passed.
  • git diff --check and merge-tree identity check — passed; hooks did not change the validated tree.

The two local macOS complete real-browser verifier attempts below did not complete successfully. Current-head Linux real-browser CI subsequently passed (linked below); that does not erase the local macOS limitation. Two complete verifier attempts were made without changing budgets: the first CLI child was killed (the helper has a 60-second cap); the isolated full rerun reached the later module-trace verification and returned provider_timeout at the existing 20000ms server deadline. That rerun had already completed the basic real provider/CLI/stdio-MCP observation, stable source comparison, scenario/storage/service-worker, screenshot, popup, network and script-export stages. The timeout cause is not established; no unrelated timeout or module-trace change has been included. At the original implementation head, the 20 focused tests and complete Chrome 155.0.8059.85 verifier had passed. That earlier result is not claimed as a pass for the current head.

Follow-up isolation check: a separate worktree at exact upstream 791f6328b02a29f23822fe600754fcd228bf754b was installed and built using the normal runners. verifyBrowserModules then passed sequentially on that baseline and current head e329736561b647bfd96565d3540a73faebf7412f, each covering all 8 public CLI/stdio-MCP trace cases, cancellation cleanup, source/map identity and zero implicit refetches. The timeout was not reproduced in these isolated runs; its cause is still unproven, and these isolated passes alone do not establish a complete macOS verifier pass. No timeout budgets or unrelated source were changed.

All npm commands above ran via mise exec node@24.18.0 -- .... Upstream CI and maintainer approval remain separate from these local results.

Upstream CI history and current status

At the previous head 46681bb87062d852667681e0d30cce4fecdb120a, CI run 39817264495 failed on the stale JavaScript reconstruction cancellation assertion. Baseline run 39814960190 had the same expected AnalysisCancelledError versus actual ArtifactOperationError mismatch. Upstream subsequently fixed the assertion while preserving artifact-owned cancellation/provenance; this head merges that fix. At current head e329736561b647bfd96565d3540a73faebf7412f, CI run 39827440624 completed successfully, including CI required, all eight test shards, source tests, static/docs/build checks, portable tests, Linux native package checks and the Linux real-browser verifier. The 19 unselected/skipped lanes are not passing coverage. Superseded/cancelled jobs are not reused as evidence. Human readiness and successful CI request initial review, not maintainer approval or merge permission.

Compatibility, safety, and release

  • Breaking changes or migration steps: no API/schema migration. New incomplete producer outputs truthfully become truncated/unknown; existing stored captures are not retroactively repaired.
  • Real Hopper/Ghidra, browser, or OS coverage: real Chromium-family Chrome on macOS; no Hopper/Ghidra dependency or change.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: keeps target/origin gates, original snapshot provenance, bounded source selection, and cleanup ownership; no new target execution or protection bypass. The snapshot is bounded by the existing admitted metadata retention limit.

Author readiness

Human owner: @gongfpp

AI-assisted contribution. The human owner confirmed review and requested initial maintainer review; automated/static review is not a substitute for those human declarations.

  • 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/6358 **Author:** [@gongfpp](https://github.com/gongfpp) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/browser-late-script-inventory-6231` --- ### 📝 Commits (2) - [`46681bb`](https://github.com/morluto/rea/commit/46681bb87062d852667681e0d30cce4fecdb120a) fix(browser): account for late script metadata - [`e329736`](https://github.com/morluto/rea/commit/e329736561b647bfd96565d3540a73faebf7412f) chore: sync upstream cancellation assertion fix ### 📊 Changes **4 files changed** (+259 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `docs/browser-observation.md` (+8 -0) 📝 `src/browser/CdpPageCapture.ts` (+6 -2) 📝 `src/browser/CdpPageCaptureScripts.ts` (+25 -10) ➕ `tests/boundary/mcp/scriptInventoryRace.test.ts` (+220 -0) </details> ### 📄 Description <!-- PR title: type(optional-scope): imperative outcome The title becomes the squash-merge commit subject and generated changelog entry. --> ## Summary Fixes #6231. Keep passive page script metadata/source reads tied to a fixed admitted-script snapshot, and explicitly account for additional in-scope script IDs still retained at the end of capture. Incomplete inventories must not produce a false `unchanged` script comparison. ## Problem and expected behavior If `Debugger.scriptParsed` adds a second script while the initial script's source request is pending, the old producer can return `scripts.total=2`, one retained item, zero dropped scripts, and no script truncation. An identical pair of those incomplete outputs can then compare as `unchanged`. Scripts arriving during later storage or final-frame reads can also escape accounting entirely. The fixed producer preserves the initial metadata and selected source-read pass. At the final capture boundary it counts distinct additional IDs still retained, records them as dropped script metadata, and marks `scripts` truncated. For one late ID the registered MCP workflow now observes `total=2`, retained items `=1`, dropped scripts `=1`, and `compare_web_captures` reports script status `unknown` rather than asserting equality. ## Change and scope - Snapshot the admitted ID-to-metadata map once, retaining the original facts and identity set even if an existing ID is subsequently replaced or removed. - Settle newly retained IDs once after the last CDP read/frame validation, before the completeness snapshot. Repeated events for one new ID count once. - Do not extend source reads, add CDP commands, or claim source byte/time-budget exhaustion for this omission. - Document that this is accounting for final retained IDs, not a claim to reconstruct complete page history. This is an ordinary correctness fix within the existing passive CDP observation boundary. Maintainer scope decision: not applicable; no new execution, injection, mutation, provider, or protection-recovery capability. Coordination: https://github.com/morluto/rea/issues/6231#issuecomment-6345563758 . Nearby #4014 addresses comparison identities, and #6276 addresses when coverage starts; neither supplies this producer snapshot/finalization fix. No directly overlapping public implementation was found in the inspected issue timeline and open-PR search; unpublished work cannot be ruled out. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the readiness and merge checklists unchecked. Maintainers can help resolve open questions. ## Contract and boundary impact - Semantic owner and earliest changed stage: `CdpPageCaptureScripts` snapshots admitted script metadata before source reads; `CdpPageCapture` settles final omissions before normalizing completeness. - Contribution scope fit and linked maintainer decision: existing passive observation bug fix; not applicable for an additional scope decision. - CLI and MCP/tool-catalog contract: unchanged parameters, tools, and schemas. Existing totals, dropped-event counters, truncation, and uncertainty now describe the actual retained snapshot. - Provider, bridge, target-format, or platform compatibility: existing CDP provider only; no new dependencies or browser-specific protocol features. - Evidence, artifact, provenance, or reconstruction contract: initial source SHA/metadata remain intact; incomplete script evidence stays visibly incomplete. No reconstruction change. - Target selection, authority, upstream behavior, and cleanup details (if applicable): existing target/origin filtering, source opt-in, cancellation, and detach/socket ownership remain in place; no additional target commands. - 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): no authored catalog, package, lockfile, installation, or release metadata changes; ignored generated artifacts were validated and are not committed. 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. - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [x] 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. The changed production path attaches to an explicitly selected CDP target and reads admitted metadata/source; it does not evaluate target JavaScript. Real-browser verification uses owned local fixtures and intentionally exercises the repository's broader existing browser workflows, separate from the authority of this fix. ## Evidence and regression coverage - Tests added or updated: Ten deterministic cases traverse the real `CdpBrowserProvider` and registered MCP `inspect_web_page` / `compare_web_captures` tools against a fake CDP transport. They cover source-pass, post-source, final-frame, source-disabled, duplicate-ID, shared-metadata/different-ID, foreign-origin, existing-ID replacement, deletion-plus-late-ID, and unchanged controls. - Assertions consume actual producer output: total/retained/dropped accounting, truncation, no fabricated resource-budget reason, original source SHA, conservative comparison, no extra source reads, detach and socket closure, and continued MCP responsiveness. - Base reproduction or other evidence: Exact affected base `2127095e3b4a377abbc124ca83af46b98a2a557b`, with the same regression file copied in: **7 failed, 3 passed**. Failures are missing drop accounting or missing final totals. With this fix: **10/10 passed**, plus the four existing source-budget/cancellation tests and six existing browser MCP tests (**20 passed**). - The deterministic interleavings are transport-controlled, not represented as a reproduced real-Chrome race. The separate real Chrome verifier is regression/integration evidence for actual browser, CLI, stdio MCP, passive/active capture, script export, cancellation, and cleanup workflows. - User-visible CLI/MCP output (if applicable): For the one-late-ID fixture, the actual registered-tool result has `scripts.total=2`, one retained item, `dropped_events.scripts=1`, script truncation, and script comparison status `unknown`. No additional source read is issued. - Remaining proof gaps: no exhaustive timing/browser/platform matrix, no Windows/Linux or non-Chromium run, no full repository test suite, and no assertion about unrecoverable events in older stored captures. 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 Original implementation: `46681bb87062d852667681e0d30cce4fecdb120a`. Current head: `e329736561b647bfd96565d3540a73faebf7412f`, merging upstream `791f6328b02a29f23822fe600754fcd228bf754b`. The maintainer-owned cancellation assertion fix is now included; no separate JavaScript implementation or duplicate fix was added. Relative to that upstream revision, this PR still contains only the original four browser inventory files. On macOS, Node **24.18.0**, npm **11.16.0**, at the current tree: - `npm run deps:check` and `npm run check:fast` — passed (typecheck/lint also passed the push hook). - Focused regression through the existing runner — **154 passed across 6 files**: JavaScript artifact reconstruction, JavaScript application cancellation/service, public error projection, CDP script capture, and the 10-case registered-MCP inventory race suite. The previously failing cancellation test now passes. - `npm run docs:check` — generated/document/license checks and all **9** README consistency tests passed. - `git diff --check` and merge-tree identity check — passed; hooks did not change the validated tree. The two local macOS complete real-browser verifier attempts below did not complete successfully. Current-head Linux real-browser CI subsequently passed (linked below); that does not erase the local macOS limitation. Two complete verifier attempts were made without changing budgets: the first CLI child was killed (the helper has a 60-second cap); the isolated full rerun reached the later module-trace verification and returned `provider_timeout` at the existing **20000ms** server deadline. That rerun had already completed the basic real provider/CLI/stdio-MCP observation, stable source comparison, scenario/storage/service-worker, screenshot, popup, network and script-export stages. The timeout cause is not established; no unrelated timeout or module-trace change has been included. At the original implementation head, the 20 focused tests and complete Chrome **155.0.8059.85** verifier had passed. That earlier result is not claimed as a pass for the current head. Follow-up isolation check: a separate worktree at exact upstream `791f6328b02a29f23822fe600754fcd228bf754b` was installed and built using the normal runners. `verifyBrowserModules` then passed sequentially on that baseline and current head `e329736561b647bfd96565d3540a73faebf7412f`, each covering all **8** public CLI/stdio-MCP trace cases, cancellation cleanup, source/map identity and zero implicit refetches. The timeout was not reproduced in these isolated runs; its cause is still unproven, and these isolated passes alone do not establish a complete macOS verifier pass. No timeout budgets or unrelated source were changed. All npm commands above ran via `mise exec node@24.18.0 -- ...`. Upstream CI and maintainer approval remain separate from these local results. ### Upstream CI history and current status At the previous head `46681bb87062d852667681e0d30cce4fecdb120a`, [CI run 39817264495](https://github.com/morluto/rea/actions/runs/39817264495) failed on the stale JavaScript reconstruction cancellation assertion. [Baseline run 39814960190](https://github.com/morluto/rea/actions/runs/39814960190/job/119956708297) had the same expected `AnalysisCancelledError` versus actual `ArtifactOperationError` mismatch. Upstream subsequently fixed the assertion while preserving artifact-owned cancellation/provenance; this head merges that fix. At current head `e329736561b647bfd96565d3540a73faebf7412f`, [CI run 39827440624](https://github.com/morluto/rea/actions/runs/39827440624) completed successfully, including [CI required](https://github.com/morluto/rea/actions/runs/39827440624/job/120002205986), all eight test shards, source tests, static/docs/build checks, portable tests, Linux native package checks and the [Linux real-browser verifier](https://github.com/morluto/rea/actions/runs/39827440624/job/119996227824). The 19 unselected/skipped lanes are not passing coverage. Superseded/cancelled jobs are not reused as evidence. Human readiness and successful CI request initial review, not maintainer approval or merge permission. ## Compatibility, safety, and release - Breaking changes or migration steps: no API/schema migration. New incomplete producer outputs truthfully become truncated/`unknown`; existing stored captures are not retroactively repaired. - Real Hopper/Ghidra, browser, or OS coverage: real Chromium-family Chrome on macOS; no Hopper/Ghidra dependency or change. - Package or release metadata impact: none. - Security, privacy, process, or containment review: keeps target/origin gates, original snapshot provenance, bounded source selection, and cleanup ownership; no new target execution or protection bypass. The snapshot is bounded by the existing admitted metadata retention limit. ## Author readiness Human owner: @gongfpp AI-assisted contribution. The human owner confirmed review and requested initial maintainer review; automated/static review is not a substitute for those human declarations. - [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-11-01 21:46:55 +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#7305
No description provided.