morluto/rea

Fork 0

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

[PR #6917] [MERGED] fix(ghidra): enforce request limits before encoding #7445

New issue

Closed

opened 2026-11-01 21:47:55 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6917
Author: @DeryFerd
Created: 10/23/2026
Status: ✅ Merged
Merged: 10/24/2026
Merged by: @gensokyo-dev

Base: main ← Head: fix/ghidra-bridge-request-ceiling

📝 Commits (3)

1120154 fix(ghidra): enforce the bridge request ceiling before writing
d8a5603 chore: merge main for Ghidra request review
a60cb44 fix(ghidra): preflight request bytes before encoding

📊 Changes

9 files changed (+336 additions, -22 deletions)

View changed files

📝 docs/ghidra-provider.md (+12 -0)
📝 src/config/bridgeMaxRequestBytes.ts (+17 -7)
📝 src/ghidra/GhidraClient.ts (+3 -0)
📝 src/ghidra/GhidraClientTypes.ts (+3 -0)
➕ src/ghidra/GhidraClientWire.test.ts (+141 -0)
📝 src/ghidra/GhidraClientWire.ts (+35 -15)
📝 src/ghidra/GhidraProviderClient.ts (+13 -0)
📝 src/ghidra/GhidraSessionError.ts (+1 -0)
➕ tests/boundary/mcp/ghidraRequestCeilingMcp.test.ts (+111 -0)

📄 Description

Summary

Ghidra omitted the shared request ceiling. Requests now use REA_BRIDGE_MAX_REQUEST_BYTES before JSON encoding, pending-reply registration or socket writes. Oversize calls report a typed resource_constraint with the effective setting and exact encoded size. Previously unbounded Ghidra requests now inherit the existing 16 MiB default; callers can adjust the shared setting.

Approach

Carry the parsed ceiling from provider composition into the Ghidra client and wire; reuse the shared recovery/error owner without adding a setting or Java-side change.
Count UTF-8 JSON and escaping without allocating its serialized form. Complete counting preserves an exact payload_bytes value; admitted requests encode once. The payload ceiling excludes the framing LF, matching the shared limit contract.
Classify local refusal as admission, keeping it out of process-output drains and memory-evidence collection. Public error projection preserves its resource constraint and operation. Existing main source-origin and decompiler-timeout semantics are retained.
Document the actual client-only bound and recovery. The Java bridge does not enforce this setting.

Validation

Head a60cb4452d0ca8678700a269131d242bae172a32, integrated with main 98ec00f40:

343 tests across 13 files, npm run check, and npm run docs:check passed. Coverage includes UTF-8/escaping/lone-surrogate byte boundaries, exact-limit acceptance, refusal before encoding/pending/write, Ghidra session errors/queues, existing provider behavior and cancellation, and shared setting/peer behavior.
Two regressions fail before their fixes: the request encoder ran on refused input; SDK MCP reported the local refusal as a process failure.
Production SDK MCP registration → shared application/provider → actual GhidraClient → real authenticated loopback TCP/owned Node producer: configured 8192-byte ceiling is honored, escaped input is refused without dispatch, resource_constraint/admission and Ghidra-specific recovery survive projection, the same warm session handles the next call, authentication is redacted, and child/runtime/endpoint cleanup completes. The producer simulates Ghidra; this is not real-engine qualification.

Three alternating fresh-process pairs, two CPUs, 768 MiB Node heap, 2 GiB family stop and warm filesystem: each completed task starts a client on identical real Node executable bytes, accepts 12 small calls, refuses five 4 MiB NUL patterns (about 24 MiB encoded each), then closes. Warm refusal phase median 1.777 → 0.376 s; ten warm small calls 12.8 → 8.8 ms. Whole-task elapsed median 6.132 → 4.155 s (ranges 5.600–7.937 → 3.875–7.102 s), CPU median 2.699 → 2.531 s, maximum sampled family peak 251.0 → 175.5 MiB (30 ms sampling). All six tasks completed, identical typed constraint payloads were checked, no oversized request reached the producer, and cleanup succeeded. Baseline is d8a5603f0 (original PR integrated with the same main). These results concern client admission against a simulated producer; whole-task timing ranges overlap and do not establish a native-Ghidra speedup. Model tokens, schema/discovery overhead and OS-cold runs are unmeasured; no token reduction is claimed.

Native Ghidra/JDK 21 verification was not run: this environment has Java 17 and no selected GHIDRA_INSTALL_DIR/JAVA_HOME. No installation was attempted. No Java bridge, provider authority, tool schema, dependency or generated catalog change is included. CI preview c54057942 combines this head with actual main e3a6ce40f. Build, documentation and all eight portable test shards passed. Two failures also occur in that exact main’s own CI run:

Static checks: releaseProposal.test.ts:378 passes includeFiles outside CommitIteratorOptions. A separate detached exact-main npm run typecheck reproduced the identical TS2353; this PR does not change the failing file.
Source tests: worker exit 1 in javascriptSemanticAnalysis.reference_lifetimes.test.ts, matching this PR’s first CI attempt. That test is unchanged by this PR; the underlying worker-exit cause is unknown. The CI retry failed at the same file with the same worker-exit error; no underlying cause is asserted.

The user explicitly authorizes merging past verified main-baseline CI failures. Conditional/blocked platform and deep-check lanes are not counted as passed. Greptile’s current-head review confirms both findings are fixed and reports no new actionable issue; both threads are resolved. The additional Codex review could not run because the service reports exhausted review quota.

Post-merge verification on a45ee8116: the same 343 focused tests, documentation check (including nine README tests), and compiled TCP/admission/session-cleanup workflow passed in a fresh detached checkout. All nine PR files match the tested CI preview. check:fast still encounters the documented baseline TS2353; it is not counted as passed. Full post-merge evidence.

Author readiness

Human owner: @DeryFerd

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.

The author's existing initial-readiness declaration is retained. Follow-up review and validation by gensokyo-dev are AI-assisted; no new independent human review is asserted.

_{🔄 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/6917 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/23/2026 **Status:** ✅ Merged **Merged:** 10/24/2026 **Merged by:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Base:** `main` ← **Head:** `fix/ghidra-bridge-request-ceiling` --- ### 📝 Commits (3) - [`1120154`](https://github.com/morluto/rea/commit/112015449312996bfa1b0511b33e2700686cfed4) fix(ghidra): enforce the bridge request ceiling before writing - [`d8a5603`](https://github.com/morluto/rea/commit/d8a5603f05ef55d387cec9aeb02c9ac031c60160) chore: merge main for Ghidra request review - [`a60cb44`](https://github.com/morluto/rea/commit/a60cb4452d0ca8678700a269131d242bae172a32) fix(ghidra): preflight request bytes before encoding ### 📊 Changes **9 files changed** (+336 additions, -22 deletions) <details> <summary>View changed files</summary> 📝 `docs/ghidra-provider.md` (+12 -0) 📝 `src/config/bridgeMaxRequestBytes.ts` (+17 -7) 📝 `src/ghidra/GhidraClient.ts` (+3 -0) 📝 `src/ghidra/GhidraClientTypes.ts` (+3 -0) ➕ `src/ghidra/GhidraClientWire.test.ts` (+141 -0) 📝 `src/ghidra/GhidraClientWire.ts` (+35 -15) 📝 `src/ghidra/GhidraProviderClient.ts` (+13 -0) 📝 `src/ghidra/GhidraSessionError.ts` (+1 -0) ➕ `tests/boundary/mcp/ghidraRequestCeilingMcp.test.ts` (+111 -0) </details> ### 📄 Description ## Summary Ghidra omitted the shared request ceiling. Requests now use `REA_BRIDGE_MAX_REQUEST_BYTES` before JSON encoding, pending-reply registration or socket writes. Oversize calls report a typed `resource_constraint` with the effective setting and exact encoded size. Previously unbounded Ghidra requests now inherit the existing **16 MiB default**; callers can adjust the shared setting. ## Approach - Carry the parsed ceiling from provider composition into the Ghidra client and wire; reuse the shared recovery/error owner without adding a setting or Java-side change. - Count UTF-8 JSON and escaping without allocating its serialized form. Complete counting preserves an exact `payload_bytes` value; admitted requests encode once. The payload ceiling excludes the framing LF, matching the shared limit contract. - Classify local refusal as `admission`, keeping it out of process-output drains and memory-evidence collection. Public error projection preserves its resource constraint and operation. Existing main source-origin and decompiler-timeout semantics are retained. - Document the actual client-only bound and recovery. The Java bridge does not enforce this setting. ## Validation Head `a60cb4452d0ca8678700a269131d242bae172a32`, integrated with main `98ec00f40`: - **343 tests across 13 files**, **`npm run check`**, and **`npm run docs:check`** passed. Coverage includes UTF-8/escaping/lone-surrogate byte boundaries, exact-limit acceptance, refusal before encoding/pending/write, Ghidra session errors/queues, existing provider behavior and cancellation, and shared setting/peer behavior. - Two regressions fail before their fixes: the request encoder ran on refused input; SDK MCP reported the local refusal as a process failure. - Production SDK MCP registration → shared application/provider → actual GhidraClient → real authenticated loopback TCP/owned Node producer: configured 8192-byte ceiling is honored, escaped input is refused without dispatch, `resource_constraint`/`admission` and Ghidra-specific recovery survive projection, the same warm session handles the next call, authentication is redacted, and child/runtime/endpoint cleanup completes. The producer simulates Ghidra; this is not real-engine qualification. Three alternating fresh-process pairs, two CPUs, 768 MiB Node heap, 2 GiB family stop and warm filesystem: each completed task starts a client on identical real Node executable bytes, accepts 12 small calls, refuses five 4 MiB NUL patterns (about 24 MiB encoded each), then closes. Warm refusal phase median **1.777 → 0.376 s**; ten warm small calls **12.8 → 8.8 ms**. Whole-task elapsed median **6.132 → 4.155 s** (ranges **5.600–7.937 → 3.875–7.102 s**), CPU median **2.699 → 2.531 s**, maximum sampled family peak **251.0 → 175.5 MiB** (30 ms sampling). All six tasks completed, identical typed constraint payloads were checked, no oversized request reached the producer, and cleanup succeeded. Baseline is `d8a5603f0` (original PR integrated with the same main). These results concern client admission against a simulated producer; whole-task timing ranges overlap and do not establish a native-Ghidra speedup. Model tokens, schema/discovery overhead and OS-cold runs are unmeasured; no token reduction is claimed. Native Ghidra/JDK 21 verification was not run: this environment has Java 17 and no selected `GHIDRA_INSTALL_DIR`/`JAVA_HOME`. No installation was attempted. No Java bridge, provider authority, tool schema, dependency or generated catalog change is included. CI preview `c54057942` combines this head with actual main `e3a6ce40f`. Build, documentation and all eight portable test shards passed. Two failures also occur in that exact main’s own [CI run](https://github.com/morluto/rea/actions/runs/40079461941): - [Static checks](https://github.com/morluto/rea/actions/runs/40079461941/job/120829870149): `releaseProposal.test.ts:378` passes `includeFiles` outside `CommitIteratorOptions`. A separate detached exact-main `npm run typecheck` reproduced the identical TS2353; this PR does not change the failing file. - [Source tests](https://github.com/morluto/rea/actions/runs/40079461941/job/120829870212): worker exit 1 in `javascriptSemanticAnalysis.reference_lifetimes.test.ts`, matching this PR’s first CI attempt. That test is unchanged by this PR; the underlying worker-exit cause is unknown. The CI retry failed at the same file with the same worker-exit error; no underlying cause is asserted. The user explicitly authorizes merging past verified main-baseline CI failures. Conditional/blocked platform and deep-check lanes are not counted as passed. Greptile’s current-head review confirms both findings are fixed and reports no new actionable issue; both threads are resolved. The additional Codex review could not run because the service reports exhausted review quota. Post-merge verification on `a45ee8116`: the same 343 focused tests, documentation check (including nine README tests), and compiled TCP/admission/session-cleanup workflow passed in a fresh detached checkout. All nine PR files match the tested CI preview. `check:fast` still encounters the documented baseline TS2353; it is not counted as passed. [Full post-merge evidence](https://github.com/morluto/rea/pull/6917#issuecomment-6382505888). ## Author readiness Human owner: @DeryFerd - [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. The author's existing initial-readiness declaration is retained. Follow-up review and validation by gensokyo-dev are AI-assisted; no new independent human review is asserted. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-03 00:29:32 +00:00

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

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

No description provided.

Rows
Columns