[PR #2874] [CLOSED] fix(pcap): page retained diagnostics without splitting a surrogate pair #5457

Closed
opened 2026-10-31 01:06:48 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2874
Author: @Dragolone
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: fix/pcap-diagnostics-text-page


📝 Commits (7)

  • 0c8d5b9 fix(pcap): page retained diagnostics without splitting a surrogate pair
  • 267c099 fix(comparison): recheck historical calls-only changes (#2223)
  • 0360e16 fix(ci): isolate Linux Hopper process fixture inventory
  • 14e3bb0 fix(android): make DEX descriptor pattern portable
  • 3a0cc33 Merge upstream main into fix/pcap-diagnostics-text-page
  • 69b7461 Merge branch 'main' into fix/pcap-diagnostics-text-page
  • e42d3dc Merge branch 'main' into fix/pcap-diagnostics-text-page

📊 Changes

2 files changed (+109 additions, -3 deletions)

View changed files

➕ src/domain/analysisView/pcapCaptureView.test.ts (+99 -0)
📝 src/domain/analysisView/pcapCaptureView.ts (+10 -3)

📄 Description

Summary

The retained pcap view's diagnostics facet pages tshark/capinfos stderr with a raw value.slice(offset, offset + limit). A page can therefore end on the high half of a non-BMP character, and the next page starts with the orphaned low half. An offset inside a surrogate pair is also accepted. Every other retained text view (Android manifest, native pseudocode, WASM, EVM) uses the shared analysisTextPage owner, which backs off to a whole character and rejects a splitting offset.

No issue is linked: I searched issues and PRs for "pcap diagnostics", "pcap surrogate", "surrogate pair", "diagnostics page" and "pcap view" and found no report. pcapCaptureView.ts has only its introducing commit (9581e067a), and no open PR touches it.

Problem and expected behavior

Smallest trigger: retained tshark_stderr = "warn 😀 done", view {kind:"pcap", facet:"diagnostics", stream:"tshark", offset:0, limit:6}, then the returned next_offset.

  • Before: page 1 "warn \ud83d" (examined: 6, next_offset: 6), page 2 "\ude00 done".
  • After: page 1 "warn " (examined: 5, next_offset: 5), page 2 starts with the whole emoji. offset: 6 is rejected as invalid view input ("Diagnostics offset splits a UTF-16 surrogate pair."), as the other text views do.

The view already declares that diagnostic page totals count "UTF-16 diagnostic code units"; this keeps that unit while never emitting half a character.

Change and scope

The diagnostics branch calls analysisTextPage(value ?? "", view.offset, view.limit, "Diagnostics") and returns its error or its text and coverage. status (retained / unknown) and the missing-diagnostics unknown are unchanged. For offsets past the end and for absent diagnostics, the coverage is identical to before (pageViewCoverage does not depend on the offset once exhausted).

Not included: other pcap facets, which page rows, not text.

Contribution scope: an ordinary retained-view correctness fix reusing the existing owner; a maintainer scope decision is not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: diagnostics projection in src/domain/analysisView/pcapCaptureView.ts.
  • Contribution scope fit and linked maintainer decision: in scope; not applicable.
  • CLI and MCP/tool-catalog contract: no schema change. Diagnostics pages no longer split a character, and a splitting offset now returns AnalysisInputError instead of a half character.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: concatenated pages now reproduce the retained text exactly.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

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.

Evidence and regression coverage

  • Tests added or updated: new src/domain/analysisView/pcapCaptureView.test.ts — "pages diagnostics without splitting a surrogate pair". It covers three cases: a splitting offset is rejected, a page whose limit would split the pair backs off ("ab", next_offset: 2), and following next_offset with limit 3 reassembles "ab😀cd" exactly. The parent is built with createEvidence, like the web-capture view tests.
  • Base reproduction: on 204a6f3f2 the test fails at the first assertion (the splitting offset returns ok: true).
  • User-visible CLI/MCP output: not captured separately; the existing pcap retained-view boundary test is Linux-only and was skipped on this macOS host.
  • Remaining proof gaps: Linux-only pcap boundary tests not run locally.

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

macOS (Apple silicon), Node 24.19.0.

  • npx vitest run src/domain/analysisView/pcapCaptureView.test.ts against the base view — fails.
  • npx vitest run src/domain/analysisView/ with the fix — 6 files, 53 tests passed.
  • npm run build:cached && npx vitest run src/pcap tests/boundary/pcap tests/boundary/cli/pcapCli.test.ts tests/boundary/mcp/pcapMcp.test.ts — 34 passed, 21 skipped (Linux-only).
  • npm run verify:test-discovery — all test files discovered exactly once.
  • npm run typecheck — passed.
  • npx oxfmt --check and npx oxlint on both changed files — passed, no errors.
  • npm run verify:module-boundaries — passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: a caller that requested an offset inside a surrogate pair now gets an input error; resuming from the returned next_offset never does that.
  • Real Hopper/Ghidra, browser, or OS coverage: macOS only; Linux pcap boundary tests skipped here.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none.

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/2874 **Author:** [@Dragolone](https://github.com/Dragolone) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/pcap-diagnostics-text-page` --- ### 📝 Commits (7) - [`0c8d5b9`](https://github.com/morluto/rea/commit/0c8d5b936e2c982ccf019d2cc1a85613cf7f424d) fix(pcap): page retained diagnostics without splitting a surrogate pair - [`267c099`](https://github.com/morluto/rea/commit/267c099b3e8d66e2f265e664f8e9353f9e015a19) fix(comparison): recheck historical calls-only changes (#2223) - [`0360e16`](https://github.com/morluto/rea/commit/0360e16c1e4ec08af28537bd81862142050bb9c1) fix(ci): isolate Linux Hopper process fixture inventory - [`14e3bb0`](https://github.com/morluto/rea/commit/14e3bb0c1998efad55a55e1c549a653daaffdc1d) fix(android): make DEX descriptor pattern portable - [`3a0cc33`](https://github.com/morluto/rea/commit/3a0cc33d6e5f4c517e43cb90226d0abb3bc8b829) Merge upstream main into fix/pcap-diagnostics-text-page - [`69b7461`](https://github.com/morluto/rea/commit/69b7461d700342abe18a72c1c223d5fedb4852a1) Merge branch 'main' into fix/pcap-diagnostics-text-page - [`e42d3dc`](https://github.com/morluto/rea/commit/e42d3dc992817365b7fad979568934fbb966938d) Merge branch 'main' into fix/pcap-diagnostics-text-page ### 📊 Changes **2 files changed** (+109 additions, -3 deletions) <details> <summary>View changed files</summary> ➕ `src/domain/analysisView/pcapCaptureView.test.ts` (+99 -0) 📝 `src/domain/analysisView/pcapCaptureView.ts` (+10 -3) </details> ### 📄 Description ## Summary The retained pcap view's `diagnostics` facet pages tshark/capinfos stderr with a raw `value.slice(offset, offset + limit)`. A page can therefore end on the high half of a non-BMP character, and the next page starts with the orphaned low half. An offset inside a surrogate pair is also accepted. Every other retained text view (Android manifest, native pseudocode, WASM, EVM) uses the shared `analysisTextPage` owner, which backs off to a whole character and rejects a splitting offset. No issue is linked: I searched issues and PRs for "pcap diagnostics", "pcap surrogate", "surrogate pair", "diagnostics page" and "pcap view" and found no report. `pcapCaptureView.ts` has only its introducing commit (9581e067a), and no open PR touches it. ## Problem and expected behavior Smallest trigger: retained `tshark_stderr = "warn 😀 done"`, view `{kind:"pcap", facet:"diagnostics", stream:"tshark", offset:0, limit:6}`, then the returned `next_offset`. - Before: page 1 `"warn \ud83d"` (`examined: 6, next_offset: 6`), page 2 `"\ude00 done"`. - After: page 1 `"warn "` (`examined: 5, next_offset: 5`), page 2 starts with the whole emoji. `offset: 6` is rejected as invalid view input ("Diagnostics offset splits a UTF-16 surrogate pair."), as the other text views do. The view already declares that diagnostic page totals count "UTF-16 diagnostic code units"; this keeps that unit while never emitting half a character. ## Change and scope The diagnostics branch calls `analysisTextPage(value ?? "", view.offset, view.limit, "Diagnostics")` and returns its error or its text and coverage. `status` (`retained` / `unknown`) and the missing-diagnostics unknown are unchanged. For offsets past the end and for absent diagnostics, the coverage is identical to before (`pageViewCoverage` does not depend on the offset once exhausted). Not included: other pcap facets, which page rows, not text. Contribution scope: an ordinary retained-view correctness fix reusing the existing owner; a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: diagnostics projection in `src/domain/analysisView/pcapCaptureView.ts`. - Contribution scope fit and linked maintainer decision: in scope; not applicable. - CLI and MCP/tool-catalog contract: no schema change. Diagnostics pages no longer split a character, and a splitting offset now returns `AnalysisInputError` instead of a half character. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: concatenated pages now reproduce the retained text exactly. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. 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. - [ ] 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: new `src/domain/analysisView/pcapCaptureView.test.ts` — "pages diagnostics without splitting a surrogate pair". It covers three cases: a splitting offset is rejected, a page whose limit would split the pair backs off (`"ab"`, `next_offset: 2`), and following `next_offset` with limit 3 reassembles `"ab😀cd"` exactly. The parent is built with `createEvidence`, like the web-capture view tests. - Base reproduction: on `204a6f3f2` the test fails at the first assertion (the splitting offset returns `ok: true`). - User-visible CLI/MCP output: not captured separately; the existing pcap retained-view boundary test is Linux-only and was skipped on this macOS host. - Remaining proof gaps: Linux-only pcap boundary tests not run locally. 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 macOS (Apple silicon), Node 24.19.0. - `npx vitest run src/domain/analysisView/pcapCaptureView.test.ts` against the base view — fails. - `npx vitest run src/domain/analysisView/` with the fix — 6 files, 53 tests passed. - `npm run build:cached && npx vitest run src/pcap tests/boundary/pcap tests/boundary/cli/pcapCli.test.ts tests/boundary/mcp/pcapMcp.test.ts` — 34 passed, 21 skipped (Linux-only). - `npm run verify:test-discovery` — all test files discovered exactly once. - `npm run typecheck` — passed. - `npx oxfmt --check` and `npx oxlint` on both changed files — passed, no errors. - `npm run verify:module-boundaries` — passed. ## Compatibility, safety, and release - Breaking changes or migration steps: a caller that requested an offset inside a surrogate pair now gets an input error; resuming from the returned `next_offset` never does that. - Real Hopper/Ghidra, browser, or OS coverage: macOS only; Linux pcap boundary tests skipped here. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none. ## 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-10-31 01:06:48 +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#5457
No description provided.