morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[PR #3479] [MERGED] fix(browser): retain the null console argument in selected console text #5766

New issue

Closed

opened 2026-10-31 01:09:17 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:17 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3479
Author: @NIU-123370
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/21/2026
Merged by: @morluto

Base: main ← Head: fix/browser-console-null-argument

📝 Commits (1)

c60cc96 fix(browser): retain the null console argument in selected console text

📊 Changes

4 files changed (+21 additions, -1 deletions)

View changed files

📝 src/browser/CdpCaptureEventHelpers.ts (+6 -0)
📝 tests/boundary/browser/cdpBrowserProvider.sensitive_data.test.ts (+10 -1)
📝 tests/fixtures/fakeCdpBrowserEvents.ts (+3 -0)
📝 tests/fixtures/fakeCdpBrowserTypes.ts (+2 -0)

📄 Description

Summary

Fixes #3377.

inspect-web-page --include-console-text silently dropped every null console
argument. CDP spells null as { type: "object", subtype: "null", value: null },
and consolePrimitive only recognized the protocol types that carry a scalar
value, so a null argument fell through to default: return undefined and never
reached text_capture.values, while adjacent primitives in the same call stayed.

docs/browser-observation.md:284 already promises that selected console text
retains "already-delivered primitive values … verbatim" while "objects, getters,
and remote properties are never expanded". null is already delivered; this
aligns the capture with that documented contract.

Problem and expected behavior

Trigger: an observed page runs console.log(null, undefined, false) and console
text capture is selected.
Violated invariant: a selected console-text capture retains every
already-delivered primitive verbatim without expanding a remote object.
Before (real Google Chrome 155.0.8059.79, main cbc0e97c):
values = index 1 undefined, index 2 false; retained_bytes = 14.
Now: index 0 { "argument_index": 0, "type": "object", "text": "null" } is
retained alongside them; retained_bytes = 18.

argument_types still reports object at index 0 because that is what the
protocol declares. The retained text is what separates the null sentinel from a
genuinely unexpanded remote object, which remains omitted.

Change and scope

src/browser/CdpCaptureEventHelpers.ts — consolePrimitive gains one
case "object" that produces the sentinel text only when the delivered
subtype is null. Everything else, including an object carrying an
objectId, still returns undefined, so no Runtime.getProperties /
Runtime.callFunctionOn is implied.
tests/fixtures/fakeCdpBrowserTypes.ts, tests/fixtures/fakeCdpBrowserEvents.ts
— one opt-in knob, nullConsoleArgument, that delivers the exact wire shape
recorded from a live Chrome payload probe
({"type":"object","subtype":"null","value":null}) on the console event the
sensitive-data fixture already emits.
tests/boundary/browser/cdpBrowserProvider.sensitive_data.test.ts — the existing
"captures requested console text verbatim and value-free payload shapes" case now
carries the null argument, pinning that index 0 is retained, that the unexpanded
object at index 2 is still absent, and that objectId / object-secret still
never enter Evidence.

Owner check: consolePrimitive is the only producer of
console.events[].text_capture.values (single consumer,
src/browser/CdpCaptureEventHandlers.ts:482). The scenario path
(src/browser/PlaywrightScenarioEvents.ts:348) formats from Playwright's
ConsoleMessage.text(), which already renders null, so it is not a second copy
of this defect and is intentionally untouched.

Intentionally not included: no schema change (values[].type keeps the protocol
type), no retention of other object subtypes (array, error, node, date
still yield nothing), and no documentation edit, because the existing doc line
already states the contract this restores.

Contract and boundary impact

Semantic owner and earliest changed stage: consolePrimitive in the CDP passive
capture path; earliest stage is console event projection, before Evidence
construction.
Contribution scope fit and linked maintainer decision: ordinary fix inside an
existing boundary — not applicable.
CLI and MCP/tool-catalog contract: no flag, schema, or tool-description change;
selected console text may carry one additional value entry per console event.
Provider, bridge, target-format, or platform compatibility: no provider, bridge,
or platform gate touched; CDP observation only.
Evidence, artifact, provenance, or reconstruction contract: console text gains
the retained null primitive and retained_bytes grows accordingly. Captures that
contain a null console argument get a different console projection, which is the
point of the fix; nothing previously retained is renamed or removed.
Target selection, authority, upstream behavior, and cleanup details: unchanged —
REA still sends no property-expansion command for console arguments (asserted in
the updated test).
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 or updated: one existing boundary case extended (no new test file,
so no repeated capture preamble enters the duplication report).

Base reproduction (executed, not source-derived): the reporter's own script was
run against the compiled CLI on pristine cbc0e97c sources and again with the
fix, same machine, same Chrome:

// base cbc0e97c
{"argument_types":["object","undefined","boolean"],
 "text_capture":{"status":"included",
   "values":[{"argument_index":1,"type":"undefined","text":"undefined"},
             {"argument_index":2,"type":"boolean","text":"false"}],
   "retained_bytes":14}}
// with this patch
{"argument_types":["object","undefined","boolean"],
 "text_capture":{"status":"included",
   "values":[{"argument_index":0,"type":"object","text":"null"},
             {"argument_index":1,"type":"undefined","text":"undefined"},
             {"argument_index":2,"type":"boolean","text":"false"}],
   "retained_bytes":18}}

Real producer wire shape (executed probe, no REA code involved):
[{type:"object",subtype:"null",value:null},{type:"undefined"},{type:"boolean",value:false}]
— the fixture now delivers exactly this.

Mutation arms, each actually run against
tests/boundary/browser/cdpBrowserProvider.sensitive_data.test.ts, tree restored
and re-verified between arms:

arm	mutation	observed
C1	none (control)	`Tests 6 passed (6)`
M1	remove the `case "object"` block (the base defect)	`1 failed \| 5 passed (6)`
M2	invert the subtype comparison (`!== "null"`)	`1 failed \| 5 passed (6)`
M3	match the wrong sentinel (`"undefined"`)	`1 failed \| 5 passed (6)`
M4	drop the subtype guard so every object yields text	`1 failed \| 5 passed (6)`
M5	change the sentinel spelling (`"NULL"`)	`1 failed \| 5 passed (6)`
M6	report `type: "null"` instead of the protocol type	`1 failed \| 5 passed (6)`
M7	retain null with empty text	`1 failed \| 5 passed (6)`

M4 is the arm that keeps "objects are never expanded" honest; M6 is the arm that
keeps the protocol type preserved.

Remaining proof gaps: no Windows/Linux Chrome run, no Playwright-driven scenario
console capture (different owner, argued above), and the
verify:browser:runtime lane was not executed locally.

Validation performed

npx vitest run tests/boundary/browser/cdpBrowserProvider.sensitive_data.test.ts
with the updated test on base sources — 1 failed | 5 passed (6); the
assertion diff shows the absent {argument_index: 3, text: "null"} entry and
retained_bytes 37 vs 41.
Same command after the source change — Test Files 1 passed (1),
Tests 6 passed (6).
npx vitest run tests/boundary/browser/ tests/boundary/mcp/browserMcp.test.ts —
Test Files 50 passed | 1 skipped (51), Tests 550 passed | 6 skipped (556).
npx vitest run tests/boundary/cli/browserCli.test.ts tests/boundary/mcp/browserComparisonInput.test.ts (other consumers of the
touched fixture) — Test Files 2 passed (2), Tests 12 passed (12).
npm run build — exit 0 (Node 24.18.0; required because the CLI repro runs the
compiled runtime).
npm run typecheck — exit 0.
npm run lint — exit 0, Checked resolved module boundaries in 1878 source files.
./node_modules/.bin/oxfmt --check <the four changed files> — exit 0,
All matched files use the correct format. (prettier was not run).
npm run jscpd with the change — Duplication: 2715 files, 2006 clones, 3.22%;
the same tree with the change reverted — byte-identical
2715 files, 2006 clones, 3.22%, so this patch contributes zero clones
(.jscpd.json threshold is 3.5).
Real-browser CLI pair quoted above: executed.
Not run: CI shard lanes, verify:browser:runtime, other platforms.

Compatibility, safety, and release

Breaking changes or migration steps: none. Retained output is additive; no
selected-behavior flag changes.
Real Hopper/Ghidra, browser, or OS coverage: real Google Chrome
155.0.8059.79 on macOS 26 arm64 through inspect-web-page --include-console-text; no Windows or Linux browser coverage.
Package or release metadata impact: none.
Security, privacy, process, or containment review: adds no process, no CDP
command, and no remote-object expansion. null carries no secret surface, and the
updated test still asserts that must-not-be-expanded and object-secret never
appear in the returned Evidence.

Author readiness

Human owner: @NIU-123370

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/3479 **Author:** [@NIU-123370](https://github.com/NIU-123370) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/browser-console-null-argument` --- ### 📝 Commits (1) - [`c60cc96`](https://github.com/morluto/rea/commit/c60cc96fae4b1b0ced3e205444c8fa738e781669) fix(browser): retain the null console argument in selected console text ### 📊 Changes **4 files changed** (+21 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/browser/CdpCaptureEventHelpers.ts` (+6 -0) 📝 `tests/boundary/browser/cdpBrowserProvider.sensitive_data.test.ts` (+10 -1) 📝 `tests/fixtures/fakeCdpBrowserEvents.ts` (+3 -0) 📝 `tests/fixtures/fakeCdpBrowserTypes.ts` (+2 -0) </details> ### 📄 Description ## Summary Fixes #3377. `inspect-web-page --include-console-text` silently dropped every `null` console argument. CDP spells `null` as `{ type: "object", subtype: "null", value: null }`, and `consolePrimitive` only recognized the protocol types that carry a scalar `value`, so a null argument fell through to `default: return undefined` and never reached `text_capture.values`, while adjacent primitives in the same call stayed. `docs/browser-observation.md:284` already promises that selected console text retains "already-delivered primitive values … verbatim" while "objects, getters, and remote properties are never expanded". `null` is already delivered; this aligns the capture with that documented contract. ## Problem and expected behavior - Trigger: an observed page runs `console.log(null, undefined, false)` and console text capture is selected. - Violated invariant: a selected console-text capture retains every already-delivered primitive verbatim without expanding a remote object. - Before (real Google Chrome 155.0.8059.79, main `cbc0e97c`): `values` = index 1 `undefined`, index 2 `false`; `retained_bytes` = 14. - Now: index 0 `{ "argument_index": 0, "type": "object", "text": "null" }` is retained alongside them; `retained_bytes` = 18. `argument_types` still reports `object` at index 0 because that is what the protocol declares. The retained text is what separates the null sentinel from a genuinely unexpanded remote object, which remains omitted. ## Change and scope - `src/browser/CdpCaptureEventHelpers.ts` — `consolePrimitive` gains one `case "object"` that produces the sentinel text **only** when the delivered `subtype` is `null`. Everything else, including an object carrying an `objectId`, still returns `undefined`, so no `Runtime.getProperties` / `Runtime.callFunctionOn` is implied. - `tests/fixtures/fakeCdpBrowserTypes.ts`, `tests/fixtures/fakeCdpBrowserEvents.ts` — one opt-in knob, `nullConsoleArgument`, that delivers the exact wire shape recorded from a live Chrome payload probe (`{"type":"object","subtype":"null","value":null}`) on the console event the sensitive-data fixture already emits. - `tests/boundary/browser/cdpBrowserProvider.sensitive_data.test.ts` — the existing "captures requested console text verbatim and value-free payload shapes" case now carries the null argument, pinning that index 0 is retained, that the unexpanded object at index 2 is still absent, and that `objectId` / `object-secret` still never enter Evidence. Owner check: `consolePrimitive` is the only producer of `console.events[].text_capture.values` (single consumer, `src/browser/CdpCaptureEventHandlers.ts:482`). The scenario path (`src/browser/PlaywrightScenarioEvents.ts:348`) formats from Playwright's `ConsoleMessage.text()`, which already renders `null`, so it is not a second copy of this defect and is intentionally untouched. Intentionally not included: no schema change (`values[].type` keeps the protocol type), no retention of other object subtypes (`array`, `error`, `node`, `date` still yield nothing), and no documentation edit, because the existing doc line already states the contract this restores. ## Contract and boundary impact - Semantic owner and earliest changed stage: `consolePrimitive` in the CDP passive capture path; earliest stage is console event projection, before Evidence construction. - Contribution scope fit and linked maintainer decision: ordinary fix inside an existing boundary — not applicable. - CLI and MCP/tool-catalog contract: no flag, schema, or tool-description change; selected console text may carry one additional value entry per console event. - Provider, bridge, target-format, or platform compatibility: no provider, bridge, or platform gate touched; CDP observation only. - Evidence, artifact, provenance, or reconstruction contract: console text gains the retained null primitive and `retained_bytes` grows accordingly. Captures that contain a null console argument get a different console projection, which is the point of the fix; nothing previously retained is renamed or removed. - Target selection, authority, upstream behavior, and cleanup details: unchanged — REA still sends no property-expansion command for console arguments (asserted in the updated test). - 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 or updated: one existing boundary case extended (no new test file, so no repeated capture preamble enters the duplication report). - Base reproduction (executed, not source-derived): the reporter's own script was run against the compiled CLI on pristine `cbc0e97c` sources and again with the fix, same machine, same Chrome: ```json // base cbc0e97c {"argument_types":["object","undefined","boolean"], "text_capture":{"status":"included", "values":[{"argument_index":1,"type":"undefined","text":"undefined"}, {"argument_index":2,"type":"boolean","text":"false"}], "retained_bytes":14}} // with this patch {"argument_types":["object","undefined","boolean"], "text_capture":{"status":"included", "values":[{"argument_index":0,"type":"object","text":"null"}, {"argument_index":1,"type":"undefined","text":"undefined"}, {"argument_index":2,"type":"boolean","text":"false"}], "retained_bytes":18}} ``` - Real producer wire shape (executed probe, no REA code involved): `[{type:"object",subtype:"null",value:null},{type:"undefined"},{type:"boolean",value:false}]` — the fixture now delivers exactly this. - Mutation arms, each actually run against `tests/boundary/browser/cdpBrowserProvider.sensitive_data.test.ts`, tree restored and re-verified between arms: | arm | mutation | observed | | --- | --- | --- | | C1 | none (control) | `Tests 6 passed (6)` | | M1 | remove the `case "object"` block (the base defect) | `1 failed \| 5 passed (6)` | | M2 | invert the subtype comparison (`!== "null"`) | `1 failed \| 5 passed (6)` | | M3 | match the wrong sentinel (`"undefined"`) | `1 failed \| 5 passed (6)` | | M4 | drop the subtype guard so every object yields text | `1 failed \| 5 passed (6)` | | M5 | change the sentinel spelling (`"NULL"`) | `1 failed \| 5 passed (6)` | | M6 | report `type: "null"` instead of the protocol type | `1 failed \| 5 passed (6)` | | M7 | retain null with empty text | `1 failed \| 5 passed (6)` | M4 is the arm that keeps "objects are never expanded" honest; M6 is the arm that keeps the protocol type preserved. - Remaining proof gaps: no Windows/Linux Chrome run, no Playwright-driven scenario console capture (different owner, argued above), and the `verify:browser:runtime` lane was not executed locally. ## Validation performed - `npx vitest run tests/boundary/browser/cdpBrowserProvider.sensitive_data.test.ts` with the updated test on **base** sources — `1 failed | 5 passed (6)`; the assertion diff shows the absent `{argument_index: 3, text: "null"}` entry and `retained_bytes` 37 vs 41. - Same command after the source change — `Test Files 1 passed (1)`, `Tests 6 passed (6)`. - `npx vitest run tests/boundary/browser/ tests/boundary/mcp/browserMcp.test.ts` — `Test Files 50 passed | 1 skipped (51)`, `Tests 550 passed | 6 skipped (556)`. - `npx vitest run tests/boundary/cli/browserCli.test.ts tests/boundary/mcp/browserComparisonInput.test.ts` (other consumers of the touched fixture) — `Test Files 2 passed (2)`, `Tests 12 passed (12)`. - `npm run build` — exit 0 (Node 24.18.0; required because the CLI repro runs the compiled runtime). - `npm run typecheck` — exit 0. - `npm run lint` — exit 0, `Checked resolved module boundaries in 1878 source files.` - `./node_modules/.bin/oxfmt --check <the four changed files>` — exit 0, `All matched files use the correct format.` (prettier was not run). - `npm run jscpd` with the change — `Duplication: 2715 files, 2006 clones, 3.22%`; the same tree with the change reverted — byte-identical `2715 files, 2006 clones, 3.22%`, so this patch contributes zero clones (`.jscpd.json` threshold is 3.5). - Real-browser CLI pair quoted above: executed. - Not run: CI shard lanes, `verify:browser:runtime`, other platforms. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Retained output is additive; no selected-behavior flag changes. - Real Hopper/Ghidra, browser, or OS coverage: real Google Chrome 155.0.8059.79 on macOS 26 arm64 through `inspect-web-page --include-console-text`; no Windows or Linux browser coverage. - Package or release metadata impact: none. - Security, privacy, process, or containment review: adds no process, no CDP command, and no remote-object expansion. `null` carries no secret surface, and the updated test still asserts that `must-not-be-expanded` and `object-secret` never appear in the returned Evidence. ## Author readiness Human owner: @NIU-123370 - [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 - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:09:17 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:44:57 +00:00

[PR #5766] [MERGED] perf(wasm): skip payload text during WAT framing #7045

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

No description provided.

Rows
Columns