[PR #5290] [MERGED] fix(rizin): keep protocol failures outside exit recovery #6860

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5290
Author: @gensokyo-dev
Created: 10/19/2026
Status: ✅ Merged
Merged: 10/19/2026
Merged by: @gensokyo-dev

Base: main ← Head: fix/5282-rizin-protocol-exit-boundary


📝 Commits (1)

  • e23fbb3 fix(rizin): keep protocol failures outside exit recovery

📊 Changes

2 files changed (+42 additions, -20 deletions)

View changed files

📝 src/rizin/RizinDebugSessionManager.ts (+12 -6)
📝 tests/boundary/providers/RizinInferiorExit.test.ts (+30 -14)

📄 Description

Summary

Fixes #5282 and the P1 review finding on #5281.

An inferior ending during an ambiguous final e dbg.exitkills reply could make close incorrectly succeed. The process-exit catch covered a helper that mixed native policy readback with Linux descriptor checks.

Split the policy and descriptor helpers, remove the combined helper, and require final protocol readback outside the process-only recovery block. Exit proof can no longer hide protocol failures; the debugger remains owned on failure. CLI and MCP share this implementation. No new API, authority, schema, dependency or cache.

Evidence and regression coverage

  • Executed the new deterministic case on merged main e84e17e30: 1 genuine regression failure, 2 controls passed. The injected native producer waits for a real owned inferior to exit during the final policy query, then emits an ambiguous value. Close incorrectly returned ok: true.
  • Fixed regression also asserts the debugger was not stopped. Final focused run: 207 passed, 1 skipped across 16 files. npm run check and npm run docs:check passed.
  • Actual Rizin 0.9.1 Linux x86-64 stdio-MCP/CLI controls remain valid: short-lived target closes on the first call; CLI returns 0 with complete command Evidence; a live source-controlled program writes both stdout/stderr to its selected PTY after debugger close. Target digests unchanged. The ambiguous-provider negative is simulated, not a claim of real Rizin emitting that failure.
  • Owned process-family cleanup verified; OOM kills 0. CPU1, one heavy job, at most 8 GiB hard limit (public runs 2 GiB); largest validation peak 4.57 GiB. Counters observed at payload exit with the observer alive, not final whole-scope counters.

Performance and limits

ABBA fresh known-name SDK sessions, warm OS file cache: elapsed 4.189–4.606 → 4.181–4.201 s, CPU 3.508–3.903 → 3.520–3.537 s, peak 318.6–320.3 → 317.1–320.3 MiB. No general speed or memory improvement claimed. Both completed tasks require two MCP calls; the extra final native policy readback is required for correctness.

Raw recorded request/reply estimates (cl100k_base, tiktoken 0.12.0): 218–218 → 215–218. No token reduction claimed. No explicit discovery; protocol/init/catalog and actual model usage unmeasured. Other engine versions/platforms and real-provider fault injection remain unverified. The existing optional Rizin 0.7.4 test was skipped.

The existing documentation already requires ambiguous readback to retain ownership; this restores that contract. No related boundary cleanup remains.

Author readiness

Human owner: @gensokyo-dev

The owner explicitly authorized ready submission and merging, including CI bypass, in this session. Implementation, technical review and executed validation are recorded above; manual human-review declarations remain for the 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.

🔄 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/5290 **Author:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Created:** 10/19/2026 **Status:** ✅ Merged **Merged:** 10/19/2026 **Merged by:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Base:** `main` ← **Head:** `fix/5282-rizin-protocol-exit-boundary` --- ### 📝 Commits (1) - [`e23fbb3`](https://github.com/morluto/rea/commit/e23fbb37132c454bbdac70f4d289c302bf4225b1) fix(rizin): keep protocol failures outside exit recovery ### 📊 Changes **2 files changed** (+42 additions, -20 deletions) <details> <summary>View changed files</summary> 📝 `src/rizin/RizinDebugSessionManager.ts` (+12 -6) 📝 `tests/boundary/providers/RizinInferiorExit.test.ts` (+30 -14) </details> ### 📄 Description ## Summary Fixes #5282 and the P1 [review finding on #5281](https://github.com/morluto/rea/pull/5281#discussion_r4317377510). An inferior ending during an ambiguous final `e dbg.exitkills` reply could make close incorrectly succeed. The process-exit catch covered a helper that mixed native policy readback with Linux descriptor checks. Split the policy and descriptor helpers, remove the combined helper, and require final protocol readback outside the process-only recovery block. Exit proof can no longer hide protocol failures; the debugger remains owned on failure. CLI and MCP share this implementation. No new API, authority, schema, dependency or cache. ## Evidence and regression coverage - Executed the new deterministic case on merged main `e84e17e30`: **1 genuine regression failure, 2 controls passed**. The injected native producer waits for a real owned inferior to exit during the final policy query, then emits an ambiguous value. Close incorrectly returned `ok: true`. - Fixed regression also asserts the debugger was not stopped. Final focused run: **207 passed, 1 skipped across 16 files**. `npm run check` and `npm run docs:check` passed. - Actual Rizin 0.9.1 Linux x86-64 stdio-MCP/CLI controls remain valid: short-lived target closes on the first call; CLI returns 0 with complete command Evidence; a live source-controlled program writes both stdout/stderr to its selected PTY after debugger close. Target digests unchanged. The ambiguous-provider negative is simulated, not a claim of real Rizin emitting that failure. - Owned process-family cleanup verified; OOM kills 0. CPU1, one heavy job, at most 8 GiB hard limit (public runs 2 GiB); largest validation peak 4.57 GiB. Counters observed at payload exit with the observer alive, not final whole-scope counters. ## Performance and limits ABBA fresh known-name SDK sessions, warm OS file cache: elapsed **4.189–4.606 → 4.181–4.201 s**, CPU **3.508–3.903 → 3.520–3.537 s**, peak **318.6–320.3 → 317.1–320.3 MiB**. No general speed or memory improvement claimed. Both completed tasks require two MCP calls; the extra final native policy readback is required for correctness. Raw recorded request/reply estimates (`cl100k_base`, tiktoken 0.12.0): **218–218 → 215–218**. No token reduction claimed. No explicit discovery; protocol/init/catalog and actual model usage unmeasured. Other engine versions/platforms and real-provider fault injection remain unverified. The existing optional Rizin 0.7.4 test was skipped. The existing documentation already requires ambiguous readback to retain ownership; this restores that contract. No related boundary cleanup remains. ## Author readiness Human owner: @gensokyo-dev The owner explicitly authorized ready submission and merging, including CI bypass, in this session. Implementation, technical review and executed validation are recorded above; manual human-review declarations remain for the 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 - [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-11-01 21:43:36 +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#6860
No description provided.