morluto/rea

Fork 0

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

[PR #3444] fix(process): bound bridge response framing #5744

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3444
Author: @shivsin25
Created: 10/14/2026
Status: 🔄 Open

Base: main ← Head: fix/bridge-response-line-limit

📝 Commits (4)

cabbe2c fix(process): bound bridge response framing
bc19465 fix(process): fail only the request with an oversized reply
0d4708c Merge branch 'main' into fix/bridge-response-line-limit
b465765 Merge branch 'main' into fix/bridge-response-line-limit

📊 Changes

11 files changed (+393 additions, -4 deletions)

View changed files

📝 src/ghidra/GhidraClient.ts (+23 -0)
📝 src/ghidra/GhidraProviderClient.ts (+22 -0)
📝 src/ghidra/GhidraSessionError.ts (+1 -0)
📝 src/hopper/HopperClient.ts (+21 -0)
📝 src/hopper/HopperRequestQueue.ts (+6 -3)
📝 src/hopper/HopperResponseStream.ts (+8 -0)
➕ src/process/ProviderResponseLines.test.ts (+78 -0)
➕ src/process/ProviderResponseLines.ts (+99 -0)
📝 tests/boundary/mcp/ghidraFailureDiagnostics.test.ts (+55 -1)
📝 tests/boundary/providers/ghidra/ghidraClient.test.ts (+38 -0)
📝 tests/boundary/providers/hopper/hopperClient.protocol.test.ts (+42 -0)

📄 Description

Summary

The Hopper and Ghidra clients each had their own copy of the newline splitter for bridge replies, and neither had any limit. When a single reply line grows past V8's maximum string length (536,870,888 UTF-16 code units on Node 24), the join inside push() throws RangeError: Invalid string length. That happens inside the socket data listener and nothing catches it, so the whole REA process exits. In MCP mode that takes every other open session and the in-memory Evidence with it.

This moves line splitting into one shared ProviderResponseLines in src/process/. It never assembles a line past that limit. It skips the oversized line through its newline and keeps reading, and the client fails only the request that line belonged to, as a resource_constraint. The session and its analysis stay usable.

No linked issue. I found this while reading the provider transports.

Problem and expected behavior

Trigger: one bridge reply line longer than buffer.constants.MAX_STRING_LENGTH. The realistic sources are list_strings, list_procedures and the searches on very large targets, which return the complete collection as one line by design (see docs/ghidra-provider.md).

Expected: that one call fails with resource_constraint (resource: "transport", reported_limits.maximum_line_length) and a remediation saying the session remains available. Later calls on the same session work.

Before this change: the process exits with code 1.

Change and scope

src/process/ProviderResponseLines.ts splits decoded socket text into lines. It tracks the pending line length per fragment. When a line would pass the limit, it drops the buffered prefix, skips the rest of that line through its newline (across chunks if needed) and resumes after it. oversizedProviderResponse() builds the shared AnalysisResourceConstraintError.
Ghidra and Hopper both send one request at a time (GhidraRequestQueue is a FIFO with one active request, and Hopper keeps its active request until the reply arrives), so an oversized reply can only belong to the active request. Each client settles just that request:
- GhidraClient settles the sole pending request with a new resource failure kind, and GhidraProviderClient projects it with the real operation name.
- HopperClient settles the active request with the shared error. HopperRequestResult now names that error type explicitly.
If the reply cannot be tied to exactly one pending request, the client ends the session with a protocol error, as it does for any other unattributable reply.
GhidraResponseBuffer.ts is removed, and HopperResponseStream keeps its message validation and delegates framing.

About the limit: I did not bring back a byte cap. The 10 MiB (Hopper) and 64 MiB (Ghidra) caps were removed in 2f08c7c93, and the boundary tests still check that replies above them work. The only limit here is the runtime's own string length, so every reply that can be assembled today still is. Whether these inventories should be paged is a separate contract question and not part of this PR.

Not included: readOwner in HopperTargetLease.ts also reads a newline-terminated message without a limit, but that is a one-line owner record behind a 250 ms timeout. Cutter and JADX already enforce their own frame limits.

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: provider transport framing in src/process, used by the Ghidra and Hopper clients.
Contribution scope fit and linked maintainer decision: not applicable, this is a fix within existing boundaries.
CLI and MCP/tool-catalog contract: no schema change. An oversized reply now returns the existing resource_constraint error for that call instead of the process exiting.
Provider, bridge, target-format, or platform compatibility: no bridge or wire protocol change. Replies up to the runtime string limit behave as before.
Evidence, artifact, provenance, or reconstruction contract: unchanged. A failed call creates no Evidence.
Target selection, authority, upstream behavior, and cleanup details (if applicable): the session is kept. When the reply cannot be attributed, the existing protocol-abort path runs (fails pending requests and destroys the socket).
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects: none of the target interactions below change. This only affects how the client reads replies it already receives.

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:
- src/process/ProviderResponseLines.test.ts: a line exactly at the limit across fragments, skipping only the oversized line and delivering the next frame in the same chunk (abcdefghi\n{"id":2}\n), skipping across later chunks until the newline, and dropping the rest when the client ends the session.
- tests/boundary/providers/hopper/hopperClient.protocol.test.ts and tests/boundary/providers/ghidra/ghidraClient.test.ts: through the real fixture sockets, the oversized request fails as a resource error and the next request on the same bridge process succeeds.
- tests/boundary/mcp/ghidraFailureDiagnostics.test.ts: the MCP projection (resource_constraint, transport, reported limit, remediation) and a follow-up call on the same session.
- The real limit needs about 512 MB of socket traffic, so these tests lower MAX_STRING_LENGTH through a node:buffer mock instead of adding a test-only option to production code. The client tests only lower it for their own case.

Base reproduction (executed on main, Node 24.18.0, Windows 11): a loopback server writes MAX_STRING_LENGTH + 64 MiB bytes of a and then \n, read with setEncoding("utf8") into GhidraResponseBuffer:

RangeError: Invalid string length
    at Array.join (<anonymous>)
    at GhidraResponseBuffer.push (src/ghidra/GhidraResponseBuffer.ts:20:37)
    at Socket.<anonymous> (repro-line-overflow.mjs:34:39)
    at Socket.emit (node:events:509:28)
    at addChunk (node:internal/streams/readable:563:12)
    ...
    at TCP.onStreamRead (node:internal/stream_base_commons:189:23)
process exit code 1

User-visible CLI/MCP output (if applicable): the call fails with resource_constraint, details.resource: "transport", details.reported_limits.maximum_line_length, and remediation "Request a narrower result, such as a search instead of a complete listing. The analysis session remains available."
Remaining proof gaps: the Hopper and Ghidra client boundary suites cannot pass on my Windows host (the Hopper fixtures stop at startup on win32, and the fake Ghidra fixture fails with EBUSY on rename before the bridge starts). The two new client tests fail there for the same reason as their neighbours. Linux CI is their first real run. There was no run against real Ghidra or Hopper.

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

npm run check: passed (typecheck, lint including module boundaries, format, knip, metadata, architecture guards)
npx vitest run --project adapters src/process/ProviderResponseLines.test.ts: 5 passed
npm run test:focused -- tests/boundary/mcp/ghidraFailureDiagnostics.test.ts: 2 passed
npm run test:focused -- on the 16 Ghidra and Hopper client boundary files: the same 81 tests fail on main and on this branch, all from the host environment above, plus the 2 new client tests for the same reason
Earlier revision, through a real socket in a throwaway local test that is not committed: more than MAX_STRING_LENGTH streamed into HopperResponseStream and the shared reader reported one overflow without throwing

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: none. Windows 11 locally, Linux through CI.
Package or release metadata impact: none.
Security, privacy, process, or containment review: a misbehaving or compromised bridge can no longer take down the REA process with an oversized or unterminated line. An oversized reply fails only its own request; an unattributable one ends only its own session.

Author readiness

Human owner: @shivsin25

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/3444 **Author:** [@shivsin25](https://github.com/shivsin25) **Created:** 10/14/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/bridge-response-line-limit` --- ### 📝 Commits (4) - [`cabbe2c`](https://github.com/morluto/rea/commit/cabbe2cebfae9cb35f4c6ef827e77c28f6150d9d) fix(process): bound bridge response framing - [`bc19465`](https://github.com/morluto/rea/commit/bc1946595b9b458d880a5603814857e3d40be7bf) fix(process): fail only the request with an oversized reply - [`0d4708c`](https://github.com/morluto/rea/commit/0d4708ca0de9470bc948689d154b0ac5d25ed747) Merge branch 'main' into fix/bridge-response-line-limit - [`b465765`](https://github.com/morluto/rea/commit/b465765b8648ef7bfb6a07895747d750984b36dd) Merge branch 'main' into fix/bridge-response-line-limit ### 📊 Changes **11 files changed** (+393 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/ghidra/GhidraClient.ts` (+23 -0) 📝 `src/ghidra/GhidraProviderClient.ts` (+22 -0) 📝 `src/ghidra/GhidraSessionError.ts` (+1 -0) 📝 `src/hopper/HopperClient.ts` (+21 -0) 📝 `src/hopper/HopperRequestQueue.ts` (+6 -3) 📝 `src/hopper/HopperResponseStream.ts` (+8 -0) ➕ `src/process/ProviderResponseLines.test.ts` (+78 -0) ➕ `src/process/ProviderResponseLines.ts` (+99 -0) 📝 `tests/boundary/mcp/ghidraFailureDiagnostics.test.ts` (+55 -1) 📝 `tests/boundary/providers/ghidra/ghidraClient.test.ts` (+38 -0) 📝 `tests/boundary/providers/hopper/hopperClient.protocol.test.ts` (+42 -0) </details> ### 📄 Description ## Summary The Hopper and Ghidra clients each had their own copy of the newline splitter for bridge replies, and neither had any limit. When a single reply line grows past V8's maximum string length (536,870,888 UTF-16 code units on Node 24), the `join` inside `push()` throws `RangeError: Invalid string length`. That happens inside the socket `data` listener and nothing catches it, so the whole REA process exits. In MCP mode that takes every other open session and the in-memory Evidence with it. This moves line splitting into one shared `ProviderResponseLines` in `src/process/`. It never assembles a line past that limit. It skips the oversized line through its newline and keeps reading, and the client fails only the request that line belonged to, as a `resource_constraint`. The session and its analysis stay usable. No linked issue. I found this while reading the provider transports. ## Problem and expected behavior Trigger: one bridge reply line longer than `buffer.constants.MAX_STRING_LENGTH`. The realistic sources are `list_strings`, `list_procedures` and the searches on very large targets, which return the complete collection as one line by design (see `docs/ghidra-provider.md`). Expected: that one call fails with `resource_constraint` (`resource: "transport"`, `reported_limits.maximum_line_length`) and a remediation saying the session remains available. Later calls on the same session work. Before this change: the process exits with code 1. ## Change and scope - `src/process/ProviderResponseLines.ts` splits decoded socket text into lines. It tracks the pending line length per fragment. When a line would pass the limit, it drops the buffered prefix, skips the rest of that line through its newline (across chunks if needed) and resumes after it. `oversizedProviderResponse()` builds the shared `AnalysisResourceConstraintError`. - Ghidra and Hopper both send one request at a time (`GhidraRequestQueue` is a FIFO with one active request, and Hopper keeps its active request until the reply arrives), so an oversized reply can only belong to the active request. Each client settles just that request: - `GhidraClient` settles the sole pending request with a new `resource` failure kind, and `GhidraProviderClient` projects it with the real operation name. - `HopperClient` settles the active request with the shared error. `HopperRequestResult` now names that error type explicitly. - If the reply cannot be tied to exactly one pending request, the client ends the session with a protocol error, as it does for any other unattributable reply. - `GhidraResponseBuffer.ts` is removed, and `HopperResponseStream` keeps its message validation and delegates framing. About the limit: I did not bring back a byte cap. The 10 MiB (Hopper) and 64 MiB (Ghidra) caps were removed in 2f08c7c93, and the boundary tests still check that replies above them work. The only limit here is the runtime's own string length, so every reply that can be assembled today still is. Whether these inventories should be paged is a separate contract question and not part of this PR. Not included: `readOwner` in `HopperTargetLease.ts` also reads a newline-terminated message without a limit, but that is a one-line owner record behind a 250 ms timeout. Cutter and JADX already enforce their own frame limits. 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: provider transport framing in `src/process`, used by the Ghidra and Hopper clients. - Contribution scope fit and linked maintainer decision: not applicable, this is a fix within existing boundaries. - CLI and MCP/tool-catalog contract: no schema change. An oversized reply now returns the existing `resource_constraint` error for that call instead of the process exiting. - Provider, bridge, target-format, or platform compatibility: no bridge or wire protocol change. Replies up to the runtime string limit behave as before. - Evidence, artifact, provenance, or reconstruction contract: unchanged. A failed call creates no Evidence. - Target selection, authority, upstream behavior, and cleanup details (if applicable): the session is kept. When the reply cannot be attributed, the existing protocol-abort path runs (fails pending requests and destroys the socket). - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects: none of the target interactions below change. This only affects how the client reads replies it already receives. - [ ] 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: - `src/process/ProviderResponseLines.test.ts`: a line exactly at the limit across fragments, skipping only the oversized line and delivering the next frame in the same chunk (`abcdefghi\n{"id":2}\n`), skipping across later chunks until the newline, and dropping the rest when the client ends the session. - `tests/boundary/providers/hopper/hopperClient.protocol.test.ts` and `tests/boundary/providers/ghidra/ghidraClient.test.ts`: through the real fixture sockets, the oversized request fails as a resource error and the next request on the same bridge process succeeds. - `tests/boundary/mcp/ghidraFailureDiagnostics.test.ts`: the MCP projection (`resource_constraint`, `transport`, reported limit, remediation) and a follow-up call on the same session. - The real limit needs about 512 MB of socket traffic, so these tests lower `MAX_STRING_LENGTH` through a `node:buffer` mock instead of adding a test-only option to production code. The client tests only lower it for their own case. - Base reproduction (executed on main, Node 24.18.0, Windows 11): a loopback server writes `MAX_STRING_LENGTH + 64 MiB` bytes of `a` and then `\n`, read with `setEncoding("utf8")` into `GhidraResponseBuffer`: ``` RangeError: Invalid string length at Array.join (<anonymous>) at GhidraResponseBuffer.push (src/ghidra/GhidraResponseBuffer.ts:20:37) at Socket.<anonymous> (repro-line-overflow.mjs:34:39) at Socket.emit (node:events:509:28) at addChunk (node:internal/streams/readable:563:12) ... at TCP.onStreamRead (node:internal/stream_base_commons:189:23) process exit code 1 ``` - User-visible CLI/MCP output (if applicable): the call fails with `resource_constraint`, `details.resource: "transport"`, `details.reported_limits.maximum_line_length`, and remediation "Request a narrower result, such as a search instead of a complete listing. The analysis session remains available." - Remaining proof gaps: the Hopper and Ghidra client boundary suites cannot pass on my Windows host (the Hopper fixtures stop at startup on win32, and the fake Ghidra fixture fails with `EBUSY` on rename before the bridge starts). The two new client tests fail there for the same reason as their neighbours. Linux CI is their first real run. There was no run against real Ghidra or Hopper. 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 - `npm run check`: passed (typecheck, lint including module boundaries, format, knip, metadata, architecture guards) - `npx vitest run --project adapters src/process/ProviderResponseLines.test.ts`: 5 passed - `npm run test:focused -- tests/boundary/mcp/ghidraFailureDiagnostics.test.ts`: 2 passed - `npm run test:focused --` on the 16 Ghidra and Hopper client boundary files: the same 81 tests fail on main and on this branch, all from the host environment above, plus the 2 new client tests for the same reason - Earlier revision, through a real socket in a throwaway local test that is not committed: more than `MAX_STRING_LENGTH` streamed into `HopperResponseStream` and the shared reader reported one overflow without throwing ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: none. Windows 11 locally, Linux through CI. - Package or release metadata impact: none. - Security, privacy, process, or containment review: a misbehaving or compromised bridge can no longer take down the REA process with an oversized or unterminated line. An oversized reply fails only its own request; an unattributable one ends only its own session. ## Author readiness Human owner: @shivsin25 - [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. - [ ] 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. - [ ] 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 added the

pull-request

label

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

bsakaguchi referenced this issue

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

[GH-ISSUE #3796] [Tracker] Open feature requests by area (October 2026) #1471

bsakaguchi referenced this issue

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

[GH-ISSUE #5653] [Tracker] New issue intake by area (October 2026) #3423

bsakaguchi referenced this issue

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