[PR #6249] [CLOSED] fix(protobuf): preserve schema admission diagnostics #7226

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6249
Author: @wunianze666-netizen
Created: 10/22/2026
Status: ❌ Closed

Base: main ← Head: codex/protobuf-schema-diagnostics


📝 Commits (1)

  • f415732 fix(protobuf): preserve schema admission diagnostics

📊 Changes

5 files changed (+292 additions, -43 deletions)

View changed files

📝 docs/web-network-captures.md (+9 -0)
📝 src/browser/history/protobuf/ProtobufCapture.ts (+8 -19)
📝 src/browser/history/protobuf/ProtobufSchema.ts (+62 -15)
📝 src/browser/history/protobuf/ProtobufWire.test.ts (+106 -9)
📝 tests/acceptance/tools/protobufCapture.test.ts (+107 -0)

📄 Description

Status — 2026-10-24: superseded, not merged. Maintainer commit b3c3b21e fixed and closed #6118 on main. The core diagnostic distinction in this draft is therefore already covered by upstream's own admission result model.

I independently built unmodified main b5c42dfc2a8eddeffb8d37c669178fa04169f050 with pinned Node 24.22.1/npm 11.16.0. All 9 generated controls passed: valid descriptor, missing selected message and malformed descriptor through the production CLI plus full/compact production stdio MCP. Four retained views remained readable after deleting the generated capture/descriptor inputs; source SHA, original ordinal 0, parent Evidence, field-specific path/reason and safe wire reason were preserved. SDK-converted request/response schemas were checked. This is selected Windows public-adapter evidence, not a complete suite, live provider or whole-platform qualification.

Closing this draft as superseded instead of resolving conflicts by reintroducing the old implementation. Original head f415732d and branch/verification assets are retained. This PR has not been merged; no upstream adoption or co-authorship credit is claimed. No target traffic was replayed and no paid model was called.

Original PR description and historical verification

Summary

Fixes #6118. Preserve actionable Protobuf schema-admission diagnostics: an undefined selected message points to protobuf.schema.message_type (invalid_value); malformed or unresolved descriptor definitions point to protobuf.schema.descriptor_set_path (invalid_format). Both still return invalid_request and retain sanitized partial capture Evidence.

Problem and expected behavior

A valid descriptor with capture.Missing and corrupt descriptor bytes previously produced the same generic schema error. Callers could not tell whether to correct the selected type or replace the descriptor. Wire preflight also discarded its known byte offset/reason.

Change and scope

ProtobufSchema.read now returns the existing typed Result<ProtobufSchema, AnalysisInputError> for schema admission. The owning stage identifies the rejected field and retains wire facts or SDK causes internally. ProtobufCapture forwards this result, removing its blanket catch/reclassification. SDK exception text is not copied into public issues; shared byte-budget and cancellation failures remain outside SDK catches. An entry checkpoint also honors cancellation for empty descriptors.

Within the existing offline capture scope; no new provider, target execution or authority requiring a separate maintainer decision.

Contract and boundary impact

  • Semantic owner/earliest changed stage: descriptor-wire/SDK registry admission in ProtobufSchema, consumed by ProtobufCapture.
  • Scope/maintainer decision: correction within existing schema interpretation; separate decision not applicable.
  • CLI/MCP/tool-catalog: no new argument, tool or success schema. Both interfaces expose the corrected details.issues paths/reasons. Example: {path:["protobuf","schema","message_type"],reason:"invalid_value"}.
  • Provider/bridge/format/platform: existing FileDescriptorSet and HAR workflow; underlying protobuf-es 2.16.0 and framing remain unchanged.
  • Evidence/artifact/provenance: valid CLI/MCP Evidence IDs remain equal; failures preserve the original capture SHA and retained ordinal 0 in the generated controls.
  • Target selection/authority/cleanup: selected local files only; no acquisition, network retrieval, replay or cleanup-policy expansion.
  • Generated metadata/package/install: no dependency, version or installer change. No generated catalogs, binaries or build outputs committed.

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.

The verification harness starts REA's own CLI/MCP subprocesses and writes self-owned fixtures; the feature adds no target-process operation.

Evidence and regression coverage

  • Fixed baseline: main 26356dc34a74e38b68eb2acb4befb126176ed978, package 6.7.0.

  • Before the production change, the three new CLI/MCP cases selected by test name had one valid case pass and two error-path cases fail on the incorrect issue paths. The other 21 cases were skipped in that selected baseline run, not counted as passed.

  • After the change, all 24 capture acceptance cases pass through the compiled CLI and production stdio MCP, including valid/missing/malformed descriptor cases, retained views after failure, source identity, byte-budget refusal, credential exclusions and schema round trips.

  • Producer tests preserve malformed unknown-field rejection with the original byte-16 overflowing-varint cause, SDK decoding/registry failures, empty descriptor versus missing type, leading-dot lookup, registry expansion budget and exact caller cancellation reasons.

  • Current-head code CI 39723699187 passed for f415732d. Source and all eight portable-shard logs executed merge b22decce070262030bb0368eaf524b878c785ed9 against main bd5d189a8f8aeea669bcb93e68af6c046b567939. Raw logs confirm 21 wire/schema, 2 capture, 5 trailer, 13 failure-projection, 24 CLI/production-stdio acceptance and one MCP-contract checks.

  • Source/static/docs/build, eight portable shards, Windows native artifact build, selected Linux package baseline and controlled browser/web-network lanes passed. These are remote selected-lane results, not a new local rerun or full Windows/provider qualification.

  • Remaining gaps: no complete local repository suite, external customer capture or downstream-recovery frequency measurement; no local macOS/Linux run or comparative performance benchmark. Generated local controls and controlled CI fixture lanes do not establish customer adoption. Human initial-readiness declarations remain pending.

  • 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

Windows x64, Node 24.18.0/npm 11.16.0. npm was invoked via the pinned Node/npm-cli entry.

  • node scripts/run-development-tests.mjs focused src/browser/history/protobuf/ProtobufWire.test.ts src/browser/history/protobuf/ProtobufCapture.test.ts src/browser/history/protobuf/ProtobufBodyTrailer.test.ts src/browser/history/CaptureFailures.test.ts tests/acceptance/tools/protobufCapture.test.ts tests/boundary/mcp/webNetworkCaptureMcp.test.ts tests/conformance/readmeConsistency.test.ts — 71 passed, zero skipped, including 24 production CLI/stdio acceptance cases and SDK-converted schema checks.
  • npm run check — passed typecheck, lint/module, format, metadata and architecture checks.
  • npm run docs:check — generated checks and five fresh consistency controls passed; these repeat the same documentation file rather than add five distinct tests to the 71.
  • node scripts/verify-test-discovery.mjs — all 1,967 test files discovered exactly once.
  • node scripts/check-dependency-install.mjs and node scripts/verify-windows-native-artifact.mjs — passed. Official 6.7.0 native artifact from main ancestor 87c8086f has matching native/build/verification source relative to the frozen baseline; this is component qualification, not full Windows-provider support.

Compatibility, safety, and release

  • Breaking changes/migration: valid success behavior and inputs unchanged. Invalid schema requests intentionally receive more specific correction paths/reasons; error code/category and partial-observation behavior remain stable.
  • Real tool/OS: Windows public CLI and production stdio over generated offline inputs. No Ghidra/Hopper, live browser, capture or request replay.
  • Package/release metadata: unchanged; no release or deployment.
  • Privacy/containment: owned wire diagnostics and correction guidance only; raw SDK exception text stays in the internal cause. No unbounded list of available message types, descriptor content or credentials added to public correction messages.
  • No token, speed, memory or customer-adoption benefit measured or claimed.

Author readiness

Human owner: @wunianze666-netizen

AI-assisted implementation and verification. The human owner's initial review declaration remains pending; this PR is a draft.

  • 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 within the existing offline parsing boundary.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs and contracts 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/6249 **Author:** [@wunianze666-netizen](https://github.com/wunianze666-netizen) **Created:** 10/22/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `codex/protobuf-schema-diagnostics` --- ### 📝 Commits (1) - [`f415732`](https://github.com/morluto/rea/commit/f415732d61aa9aefd177949f3b45344c62e0274a) fix(protobuf): preserve schema admission diagnostics ### 📊 Changes **5 files changed** (+292 additions, -43 deletions) <details> <summary>View changed files</summary> 📝 `docs/web-network-captures.md` (+9 -0) 📝 `src/browser/history/protobuf/ProtobufCapture.ts` (+8 -19) 📝 `src/browser/history/protobuf/ProtobufSchema.ts` (+62 -15) 📝 `src/browser/history/protobuf/ProtobufWire.test.ts` (+106 -9) 📝 `tests/acceptance/tools/protobufCapture.test.ts` (+107 -0) </details> ### 📄 Description > **Status — 2026-10-24: superseded, not merged.** Maintainer commit [b3c3b21e](https://github.com/morluto/rea/commit/b3c3b21e43cab4e71b460d08829d8c6e4e9b9c21) fixed and [closed #6118](https://github.com/morluto/rea/issues/6118#issuecomment-6376018562) on main. The core diagnostic distinction in this draft is therefore already covered by upstream's own admission result model. > > I independently built unmodified main `b5c42dfc2a8eddeffb8d37c669178fa04169f050` with pinned Node 24.22.1/npm 11.16.0. All 9 generated controls passed: valid descriptor, missing selected message and malformed descriptor through the production CLI plus full/compact production stdio MCP. Four retained views remained readable after deleting the generated capture/descriptor inputs; source SHA, original ordinal 0, parent Evidence, field-specific path/reason and safe wire reason were preserved. SDK-converted request/response schemas were checked. This is selected Windows public-adapter evidence, not a complete suite, live provider or whole-platform qualification. > > Closing this draft as superseded instead of resolving conflicts by reintroducing the old implementation. Original head `f415732d` and branch/verification assets are retained. This PR has not been merged; no upstream adoption or co-authorship credit is claimed. No target traffic was replayed and no paid model was called. <details> <summary>Original PR description and historical verification</summary> ## Summary Fixes #6118. Preserve actionable Protobuf schema-admission diagnostics: an undefined selected message points to `protobuf.schema.message_type` (`invalid_value`); malformed or unresolved descriptor definitions point to `protobuf.schema.descriptor_set_path` (`invalid_format`). Both still return `invalid_request` and retain sanitized partial capture Evidence. ## Problem and expected behavior A valid descriptor with `capture.Missing` and corrupt descriptor bytes previously produced the same generic schema error. Callers could not tell whether to correct the selected type or replace the descriptor. Wire preflight also discarded its known byte offset/reason. ## Change and scope `ProtobufSchema.read` now returns the existing typed `Result<ProtobufSchema, AnalysisInputError>` for schema admission. The owning stage identifies the rejected field and retains wire facts or SDK causes internally. `ProtobufCapture` forwards this result, removing its blanket catch/reclassification. SDK exception text is not copied into public issues; shared byte-budget and cancellation failures remain outside SDK catches. An entry checkpoint also honors cancellation for empty descriptors. Within the [existing offline capture scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope); no new provider, target execution or authority requiring a separate maintainer decision. ## Contract and boundary impact - Semantic owner/earliest changed stage: descriptor-wire/SDK registry admission in `ProtobufSchema`, consumed by `ProtobufCapture`. - Scope/maintainer decision: correction within existing schema interpretation; separate decision not applicable. - CLI/MCP/tool-catalog: no new argument, tool or success schema. Both interfaces expose the corrected `details.issues` paths/reasons. Example: `{path:["protobuf","schema","message_type"],reason:"invalid_value"}`. - Provider/bridge/format/platform: existing FileDescriptorSet and HAR workflow; underlying protobuf-es 2.16.0 and framing remain unchanged. - Evidence/artifact/provenance: valid CLI/MCP Evidence IDs remain equal; failures preserve the original capture SHA and retained ordinal 0 in the generated controls. - Target selection/authority/cleanup: selected local files only; no acquisition, network retrieval, replay or cleanup-policy expansion. - Generated metadata/package/install: no dependency, version or installer change. No generated catalogs, binaries or build outputs committed. 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. The verification harness starts REA's own CLI/MCP subprocesses and writes self-owned fixtures; the feature adds no target-process operation. ## Evidence and regression coverage - Fixed baseline: main `26356dc34a74e38b68eb2acb4befb126176ed978`, package 6.7.0. - Before the production change, the three new CLI/MCP cases selected by test name had one valid case pass and two error-path cases fail on the incorrect issue paths. The other 21 cases were skipped in that selected baseline run, not counted as passed. - After the change, all 24 capture acceptance cases pass through the compiled CLI and production stdio MCP, including valid/missing/malformed descriptor cases, retained views after failure, source identity, byte-budget refusal, credential exclusions and schema round trips. - Producer tests preserve malformed unknown-field rejection with the original byte-16 overflowing-varint cause, SDK decoding/registry failures, empty descriptor versus missing type, leading-dot lookup, registry expansion budget and exact caller cancellation reasons. - [Current-head code CI 39723699187](https://github.com/morluto/rea/actions/runs/39723699187) **passed** for `f415732d`. Source and all eight portable-shard logs executed merge `b22decce070262030bb0368eaf524b878c785ed9` against main `bd5d189a8f8aeea669bcb93e68af6c046b567939`. Raw logs confirm 21 wire/schema, 2 capture, 5 trailer, 13 failure-projection, 24 CLI/production-stdio acceptance and one MCP-contract checks. - Source/static/docs/build, eight portable shards, Windows native artifact build, selected Linux package baseline and controlled browser/web-network lanes passed. These are remote selected-lane results, not a new local rerun or full Windows/provider qualification. - Remaining gaps: no complete local repository suite, external customer capture or downstream-recovery frequency measurement; no local macOS/Linux run or comparative performance benchmark. Generated local controls and controlled CI fixture lanes do not establish customer adoption. Human initial-readiness declarations remain pending. - [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 Windows x64, Node 24.18.0/npm 11.16.0. npm was invoked via the pinned Node/npm-cli entry. - `node scripts/run-development-tests.mjs focused src/browser/history/protobuf/ProtobufWire.test.ts src/browser/history/protobuf/ProtobufCapture.test.ts src/browser/history/protobuf/ProtobufBodyTrailer.test.ts src/browser/history/CaptureFailures.test.ts tests/acceptance/tools/protobufCapture.test.ts tests/boundary/mcp/webNetworkCaptureMcp.test.ts tests/conformance/readmeConsistency.test.ts` — **71 passed, zero skipped**, including 24 production CLI/stdio acceptance cases and SDK-converted schema checks. - `npm run check` — passed typecheck, lint/module, format, metadata and architecture checks. - `npm run docs:check` — generated checks and five fresh consistency controls passed; these repeat the same documentation file rather than add five distinct tests to the 71. - `node scripts/verify-test-discovery.mjs` — all 1,967 test files discovered exactly once. - `node scripts/check-dependency-install.mjs` and `node scripts/verify-windows-native-artifact.mjs` — passed. Official 6.7.0 native artifact from main ancestor `87c8086f` has matching native/build/verification source relative to the frozen baseline; this is component qualification, not full Windows-provider support. ## Compatibility, safety, and release - Breaking changes/migration: valid success behavior and inputs unchanged. Invalid schema requests intentionally receive more specific correction paths/reasons; error code/category and partial-observation behavior remain stable. - Real tool/OS: Windows public CLI and production stdio over generated offline inputs. No Ghidra/Hopper, live browser, capture or request replay. - Package/release metadata: unchanged; no release or deployment. - Privacy/containment: owned wire diagnostics and correction guidance only; raw SDK exception text stays in the internal cause. No unbounded list of available message types, descriptor content or credentials added to public correction messages. - No token, speed, memory or customer-adoption benefit measured or claimed. ## Author readiness Human owner: @wunianze666-netizen AI-assisted implementation and verification. The human owner's initial review declaration remains pending; this PR is a draft. - [ ] 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 - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved within the existing offline parsing boundary. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs and contracts are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. </details> --- <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:19 +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#7226
No description provided.