morluto/rea

Fork 0

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

[PR #4755] [MERGED] fix(javascript): report malformed runtime context events as excluded #6389

New issue

Closed

opened 2026-10-31 01:14:06 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:14:06 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/runtime-observation-malformed-context

📝 Commits (1)

9236b56 fix(javascript): report malformed runtime context events as excluded

📊 Changes

5 files changed (+75 additions, -1 deletions)

View changed files

📝 docs/javascript-runtime-observation.md (+3 -1)
📝 src/inspector/V8InspectorCaptureProjection.test.ts (+1 -0)
📝 src/inspector/V8InspectorCaptureProjection.ts (+9 -0)
📝 src/inspector/V8InspectorProvider.ts (+3 -0)
📝 tests/boundary/inspector/javascriptRuntimeObservation.test.ts (+59 -0)

📄 Description

Summary

Fixes #4685

observe_javascript_runtime / observe-javascript-runtime counted a Runtime.executionContextCreated or Runtime.executionContextDestroyed event with a missing or non-integer context ID (for example the string "2") as retained, then left it out of execution_contexts without saying so. The result said every observed event was kept, but one context was silently missing. With this change the malformed event is reported as an excluded event in unknowns, with a count and a reason. Valid sibling contexts are kept, and all-valid captures produce the same output as before.

Problem and expected behavior

Smallest trigger: two Runtime.executionContextCreated events with context.id 1 and "2". contextKey rejects "2", and ingestContext then calls retainEvent(state, 0) and returns without leaving any record. That increments events_retained, but no context and no invalid-event record are kept. The projection reports the unchanged counters and the surviving context only.

The broken invariant: the docs (docs/javascript-runtime-observation.md, docs/tool-design.md) say capture retains valid context metadata and that partial coverage is reported, not hidden. Expected behavior: preserve context 1, never invent context 2 from a string ID, and account for the rejected event explicitly.

Change and scope

Owner: src/inspector/V8InspectorProvider.ts parses the CDP event and owns the capture counters. CaptureState gains invalidContexts, which is incremented on the same branch that rejects the context ID. This mirrors the existing invalidScripts accounting: a rejected Debugger.scriptParsed or scriptFailedToParse is also counted as retained (0 bytes) and reported as an exclusion.
Projection: src/inspector/V8InspectorCaptureProjection.ts adds one unknowns entry when the count is non-zero, through a small invalidContextUnknowns helper. The helper keeps the finalize function's lint complexity at its baseline.
Docs: docs/javascript-runtime-observation.md now says a created or destroyed event with a missing or non-safe-integer context ID adds no context record and is retained only as a count reported in unknowns.

Why the event is not counted in events_dropped: on main, events_dropped means "dropped because the 8 MiB retained-metadata budget was reached". It always comes with truncated: true and retained_metadata_budget_exceeded. reconcile_javascript_runtime also maps capture.events_dropped to droppedEvents.scripts (src/domain/javascript/javascriptRuntimeReconciliationCaptureParsing.ts). Putting malformed context events there would misreport them as script truncation. Reporting them through unknowns keeps events_observed == events_retained + events_dropped consistent with the invalid-script precedent and does not change the schema.

Not included:

A structured execution_contexts exclusion counter. That would be a schema change: execution_contexts is an array, and adding a required field changes every Evidence ID.
Malformed executionContextId on Debugger.scriptParsed. That value is already kept as execution_context_key: null, which is a separate representation question.

Contribution scope: ordinary fix within the existing passive provider boundary. No maintainer scope decision is needed.

Contract and boundary impact

Semantic owner and earliest changed stage: V8 Inspector capture ingestion (ingestContext in V8InspectorProvider.ts), then the capture projection.
Contribution scope fit and linked maintainer decision: not applicable (bug fix inside the existing passive Inspector boundary).
CLI and MCP/tool-catalog contract: no schema change. unknowns gains one string only when malformed context IDs were observed.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: Evidence for captures with malformed context events now carries the extra unknown, so its Evidence ID differs. Evidence for all-valid captures is unchanged. Reconciliation does not read execution_contexts or these unknowns.
Target selection, authority, upstream behavior, and cleanup details: unchanged. Only Runtime.enable and Debugger.enable are sent.
Generated metadata, package, or installation impact: none.

Before and after for the mixed id: 1 / id: "2" input from the issue (source-derived from the regression test; the CLI repro script itself was not run):

// before
"capture": { "events_observed": 2, "events_retained": 2, "events_dropped": 0, "truncated": false },
"execution_contexts": [{ "context_key": "1", "state": "created", "name": "main", "origin": "" }],
"unknowns": [ /* generic attach/window caveats only */ ]

// after
"capture": { "events_observed": 2, "events_retained": 2, "events_dropped": 0, "truncated": false },
"execution_contexts": [{ "context_key": "1", "state": "created", "name": "main", "origin": "" }],
"unknowns": [
  "1 execution-context lifecycle event with a missing or malformed execution-context ID was excluded; the affected contexts' lifecycle state is unknown.",
  /* generic caveats unchanged */
]

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.

(These describe the existing observer, which attaches to an existing Inspector target. This change adds no new effect.)

Evidence and regression coverage

Tests added or updated:
- tests/boundary/inspector/javascriptRuntimeObservation.test.ts: new accounts for %s context IDs in lifecycle events. It drives the real V8InspectorProvider.observe over the existing loopback fakeV8Inspector WebSocket fixture.
  - Malformed case: created id: 1, created id: "2", destroyed executionContextId: "1". It asserts that context 1 stays created, the counters are 3/3/0 and untruncated, and there is exactly one unknown reporting 2 excluded events.
  - All-valid control: same events with numeric IDs. It asserts both contexts and no invalid-context unknown.
- src/inspector/V8InspectorCaptureProjection.test.ts: the state literal gains the new required field.
Base reproduction (executed on origin/main 73965f943, with the fix's source files temporarily restored to main): the malformed case fails with expected [] to deeply equal [ Array(1) ], because no unknown is reported, while the context list and counters match the issue. The valid control passes on main.
User-visible CLI/MCP output: see the before/after above. The public CLI repro from the issue was not run.
Remaining proof gaps:
- No real Node or Electron producer emitting malformed context IDs was attached.
- The issue's public-CLI script was not re-run.
- includes targets outside the old caller root filter in the same boundary file fails locally both on main and with this change (macOS temp path reported as unverifiable-file-location). It is unrelated to this change, and its cause was not investigated.

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

Local: macOS Darwin arm64, Node v26.5.0. The repository pin is 24.18.0, which was not installed locally; v26 is within engines.

npm ci — passed.
npm run test:focused -- tests/boundary/inspector/javascriptRuntimeObservation.test.ts on main source — 2 failed / 26 passed: the new malformed case, plus the pre-existing includes targets outside the old caller root filter.
npm run test:focused -- tests/boundary/inspector/javascriptRuntimeObservation.test.ts src/inspector/V8InspectorCaptureProjection.test.ts src/inspector/V8InspectorEndpoint.test.ts src/inspector/V8InspectorProvider.cleanup.test.ts src/inspector/V8InspectorProvider.partial.test.ts src/application/javascript/JavaScriptRuntimeObservationService.test.ts with the fix — 45 passed / 1 failed: only the pre-existing includes targets outside the old caller root filter.
npx tsc --noEmit -p . — passed.
npm run docs:check — passed.
npx oxfmt on changed files — clean.
npx oxlint on changed files — no errors. The complexity, max-lines and no-new-array warnings in the touched files already exist on main; the finalize function's complexity stays at its main baseline of 24.

Compatibility, safety, and release

Breaking changes or migration steps: none. This is an additive unknowns entry, shown only for malformed input.
Real Hopper/Ghidra, browser, or OS coverage: no real Node or Electron Inspector target was used; only the loopback stand-in.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new commands, target interaction or persisted values. The unknown contains only a count.

Author readiness

Human owner:

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.

🤖 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/4755 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/17/2026 **Status:** ✅ Merged **Merged:** 10/17/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/runtime-observation-malformed-context` --- ### 📝 Commits (1) - [`9236b56`](https://github.com/morluto/rea/commit/9236b563b051536a033a019552154c90f466a323) fix(javascript): report malformed runtime context events as excluded ### 📊 Changes **5 files changed** (+75 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `docs/javascript-runtime-observation.md` (+3 -1) 📝 `src/inspector/V8InspectorCaptureProjection.test.ts` (+1 -0) 📝 `src/inspector/V8InspectorCaptureProjection.ts` (+9 -0) 📝 `src/inspector/V8InspectorProvider.ts` (+3 -0) 📝 `tests/boundary/inspector/javascriptRuntimeObservation.test.ts` (+59 -0) </details> ### 📄 Description ## Summary Fixes #4685 `observe_javascript_runtime` / `observe-javascript-runtime` counted a `Runtime.executionContextCreated` or `Runtime.executionContextDestroyed` event with a missing or non-integer context ID (for example the string `"2"`) as retained, then left it out of `execution_contexts` without saying so. The result said every observed event was kept, but one context was silently missing. With this change the malformed event is reported as an excluded event in `unknowns`, with a count and a reason. Valid sibling contexts are kept, and all-valid captures produce the same output as before. ## Problem and expected behavior Smallest trigger: two `Runtime.executionContextCreated` events with `context.id` `1` and `"2"`. `contextKey` rejects `"2"`, and `ingestContext` then calls `retainEvent(state, 0)` and returns without leaving any record. That increments `events_retained`, but no context and no invalid-event record are kept. The projection reports the unchanged counters and the surviving context only. The broken invariant: the docs (`docs/javascript-runtime-observation.md`, `docs/tool-design.md`) say capture retains *valid* context metadata and that partial coverage is reported, not hidden. Expected behavior: preserve context `1`, never invent context `2` from a string ID, and account for the rejected event explicitly. ## Change and scope - **Owner:** `src/inspector/V8InspectorProvider.ts` parses the CDP event and owns the capture counters. `CaptureState` gains `invalidContexts`, which is incremented on the same branch that rejects the context ID. This mirrors the existing `invalidScripts` accounting: a rejected `Debugger.scriptParsed` or `scriptFailedToParse` is also counted as retained (0 bytes) and reported as an exclusion. - **Projection:** `src/inspector/V8InspectorCaptureProjection.ts` adds one `unknowns` entry when the count is non-zero, through a small `invalidContextUnknowns` helper. The helper keeps the finalize function's lint complexity at its baseline. - **Docs:** `docs/javascript-runtime-observation.md` now says a created or destroyed event with a missing or non-safe-integer context ID adds no context record and is retained only as a count reported in `unknowns`. Why the event is not counted in `events_dropped`: on main, `events_dropped` means "dropped because the 8 MiB retained-metadata budget was reached". It always comes with `truncated: true` and `retained_metadata_budget_exceeded`. `reconcile_javascript_runtime` also maps `capture.events_dropped` to `droppedEvents.scripts` (`src/domain/javascript/javascriptRuntimeReconciliationCaptureParsing.ts`). Putting malformed context events there would misreport them as script truncation. Reporting them through `unknowns` keeps `events_observed == events_retained + events_dropped` consistent with the invalid-script precedent and does not change the schema. Not included: - A structured `execution_contexts` exclusion counter. That would be a schema change: `execution_contexts` is an array, and adding a required field changes every Evidence ID. - Malformed `executionContextId` on `Debugger.scriptParsed`. That value is already kept as `execution_context_key: null`, which is a separate representation question. Contribution scope: ordinary fix within the existing passive provider boundary. No maintainer scope decision is needed. ## Contract and boundary impact - Semantic owner and earliest changed stage: V8 Inspector capture ingestion (`ingestContext` in `V8InspectorProvider.ts`), then the capture projection. - Contribution scope fit and linked maintainer decision: not applicable (bug fix inside the existing passive Inspector boundary). - CLI and MCP/tool-catalog contract: no schema change. `unknowns` gains one string only when malformed context IDs were observed. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: Evidence for captures with malformed context events now carries the extra unknown, so its Evidence ID differs. Evidence for all-valid captures is unchanged. Reconciliation does not read `execution_contexts` or these `unknowns`. - Target selection, authority, upstream behavior, and cleanup details: unchanged. Only `Runtime.enable` and `Debugger.enable` are sent. - Generated metadata, package, or installation impact: none. Before and after for the mixed `id: 1` / `id: "2"` input from the issue (source-derived from the regression test; the CLI repro script itself was not run): ```jsonc // before "capture": { "events_observed": 2, "events_retained": 2, "events_dropped": 0, "truncated": false }, "execution_contexts": [{ "context_key": "1", "state": "created", "name": "main", "origin": "" }], "unknowns": [ /* generic attach/window caveats only */ ] // after "capture": { "events_observed": 2, "events_retained": 2, "events_dropped": 0, "truncated": false }, "execution_contexts": [{ "context_key": "1", "state": "created", "name": "main", "origin": "" }], "unknowns": [ "1 execution-context lifecycle event with a missing or malformed execution-context ID was excluded; the affected contexts' lifecycle state is unknown.", /* generic caveats unchanged */ ] ``` Expected effects: - [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. - [ ] 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. (These describe the existing observer, which attaches to an existing Inspector target. This change adds no new effect.) ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/inspector/javascriptRuntimeObservation.test.ts`: new `accounts for %s context IDs in lifecycle events`. It drives the real `V8InspectorProvider.observe` over the existing loopback `fakeV8Inspector` WebSocket fixture. - Malformed case: `created id: 1`, `created id: "2"`, `destroyed executionContextId: "1"`. It asserts that context `1` stays `created`, the counters are 3/3/0 and untruncated, and there is exactly one unknown reporting 2 excluded events. - All-valid control: same events with numeric IDs. It asserts both contexts and no invalid-context unknown. - `src/inspector/V8InspectorCaptureProjection.test.ts`: the state literal gains the new required field. - Base reproduction (executed on `origin/main` 73965f943, with the fix's source files temporarily restored to main): the malformed case fails with `expected [] to deeply equal [ Array(1) ]`, because no unknown is reported, while the context list and counters match the issue. The valid control passes on main. - User-visible CLI/MCP output: see the before/after above. The public CLI repro from the issue was not run. - Remaining proof gaps: - No real Node or Electron producer emitting malformed context IDs was attached. - The issue's public-CLI script was not re-run. - `includes targets outside the old caller root filter` in the same boundary file fails locally both on main and with this change (macOS temp path reported as `unverifiable-file-location`). It is unrelated to this change, and its cause was not investigated. 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 Local: macOS Darwin arm64, Node v26.5.0. The repository pin is 24.18.0, which was not installed locally; v26 is within `engines`. - `npm ci` — passed. - `npm run test:focused -- tests/boundary/inspector/javascriptRuntimeObservation.test.ts` on main source — 2 failed / 26 passed: the new malformed case, plus the pre-existing `includes targets outside the old caller root filter`. - `npm run test:focused -- tests/boundary/inspector/javascriptRuntimeObservation.test.ts src/inspector/V8InspectorCaptureProjection.test.ts src/inspector/V8InspectorEndpoint.test.ts src/inspector/V8InspectorProvider.cleanup.test.ts src/inspector/V8InspectorProvider.partial.test.ts src/application/javascript/JavaScriptRuntimeObservationService.test.ts` with the fix — 45 passed / 1 failed: only the pre-existing `includes targets outside the old caller root filter`. - `npx tsc --noEmit -p .` — passed. - `npm run docs:check` — passed. - `npx oxfmt` on changed files — clean. - `npx oxlint` on changed files — no errors. The complexity, max-lines and no-new-array warnings in the touched files already exist on main; the finalize function's complexity stays at its main baseline of 24. ## Compatibility, safety, and release - Breaking changes or migration steps: none. This is an additive `unknowns` entry, shown only for malformed input. - Real Hopper/Ghidra, browser, or OS coverage: no real Node or Electron Inspector target was used; only the loopback stand-in. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new commands, target interaction or persisted values. The unknown contains only a count. ## Author readiness Human owner: - [ ] 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. 🤖 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-10-31 01:14:06 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:11:07 +00:00

[GH-ISSUE #5654] [Tracker] Open PRs that can't merge yet: conflicts and red CI (October 2026) #3425

bsakaguchi referenced this issue

2026-11-01 21:47:07 +00:00

[PR #6389] [CLOSED] fix(domain): reject path traversal and drive-relative paths in hostAbsoluteSchema #7331

bsakaguchi changed title from ~~[PR #4755] [MERGED] fix(javascript): report malformed runtime context events as excluded~~ to [PR #4755] fix(javascript): report malformed runtime context events as excluded

2026-11-03 15:37:35 +00:00

bsakaguchi changed title from ~~[PR #4755] fix(javascript): report malformed runtime context events as excluded~~ to [PR #4755] [MERGED] fix(javascript): report malformed runtime context events as excluded

2026-11-03 16:39:17 +00:00

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

No description provided.

Rows
Columns