[PR #3087] [CLOSED] fix(pwntools): surface actionable configuration remediation #5605

New issue

Closed

opened 2026-10-31 01:07:55 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:07:55 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3087
Author: @1052326311
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/pwntools-remediation

📝 Commits (3)

f17c87a fix(pwntools): surface actionable configuration remediation
2df4973 fix(pwntools): tighten readiness documentation and exit assertions
bcc370a Merge remote-tracking branch 'origin/main' into HEAD

📊 Changes

4 files changed (+126 additions, -4 deletions)

View changed files

📝 docs/cli.md (+7 -3)
📝 src/domain/providerSelectionError.ts (+3 -1)
📝 src/native/pwntools/PwntoolsFailures.ts (+1 -0)
➕ tests/process-global/pwntoolsCliRemediation.test.ts (+115 -0)

📄 Description

Summary

Fixes #3055. Pwntools configuration failures now put their actual recovery guidance in the top-level error message and remediation, instead of recommending rea doctor, which does not inspect this adapter.

Problem and expected behavior

With REA_PWNTOOLS_PYTHON unset, both inspect-binary-layout and inspect-recorded-crash returned provider_unavailable with a generic doctor recommendation. The useful requirement was only in details.rejections. The same loss affected missing/non-executable paths and invalid input-limit configuration.

The top-level message and remediation.action now retain the producer's specific guidance, while the error code, candidate identity, original rejection diagnostics and exit status 1 are preserved. For example, an unset variable reports “Set REA_PWNTOOLS_PYTHON to an absolute caller-supplied Python executable with pwntools 4.15.0, pyelftools 0.33 and Unicorn 2.1.2; REA never installs Python or packages.”

Change and scope

ProviderSelectionError accepts an optional caller-supplied user message, following the existing tagged-error convention. pwntoolsUnavailable supplies its already-owned reason; the shared projection carries it to CLI/MCP consumers. Other provider selection errors retain their existing default messages. No provider IDs or configuration interpretation are hardcoded in the domain layer.

The CLI guide now limits doctor --provider advice to Hopper/Ghidra/IDA/JADX and directs other adapter callers to the returned configuration guidance. This is an existing diagnostic-contract repair; a new-capability scope decision is not applicable.

Contract and boundary impact

Semantic owner: pwntoolsUnavailable produces the recovery reason; ProviderSelectionError previously replaced its user-facing message with a generic fallback.
CLI/MCP contract: more specific existing message/remediation.action; error schema, codes and original rejection details retained.
Provider/bridge/platform compatibility: configuration admission and subprocess behavior unchanged; no new doctor provider support.
Evidence/provenance: original rejection configuration key/path and captured diagnostics remain preserved.
Target/cleanup authority: no new file access, process action, or installation behavior.
Generated metadata/package/installation impact: no changes required.

Expected effects of this correction:

Documentation or usability only; no target interaction.

Evidence and regression coverage

The new CLI regression exercises both commands through their real application services and pwntools providers. Only the composition entry point is injected to supply isolated configuration. Five cases per command cover unset Python, a directory, a missing file, missing execute permission, and an invalid REA_PWNTOOLS_MAX_INPUT_BYTES. Assertions check final JSON message/remediation, code, retained rejection/configuration identity and exit status 1.

On unchanged production base c7105d97, both command regressions fail because the top-level advice is generic.
Candidate focused suites: 80 tests passed, including existing projection, input-limit and CLI finalization coverage.
Environment: real macOS filesystem and permissions with the Linux x64 admission gate simulated, matching existing process-global tests. Python, pwntools and GDB were not started; this does not claim a live Linux/provider verification.

Validation performed

npm run test:focused -- tests/process-global/pwntoolsCliRemediation.test.ts tests/process-global/pwntoolsCliFinalization.test.ts src/domain/analysisErrorProjection.test.ts src/native/pwntools/PwntoolsInputLimit.test.ts — 80 passed.
npm run check — passed, including runtime build, typecheck/lint/format and architecture guards.
npm run docs:check — passed, including 5 README consistency tests.
npm run verify:test-discovery — all 1,234 test files discovered exactly once.
git diff --check — passed.

Validated against base c7105d97. Publication refresh through main 70771523 found no changes to the selected error producer, domain error, service, or guide. New unrelated dependency/owner changes were not locally revalidated as part of this candidate.

Compatibility, safety, and release

No schema, exit status, configuration, authority or dependency change. Callers should use structured error codes rather than exact prose. Windows, native Linux x64 and real pwntools/pwndbg execution were not verified for this diagnostic-only change.

Before merge

Concrete issue, focused outcome and conventional title.
Existing scope and authority preserved; no new provider/capability approval needed.
Observable CLI regressions fail on base and pass on candidate.
Owning error producer and affected consumer inspected; shared projection reused.
Documentation updated, representative output and validation limits stated.
Diff inspected for secrets, generated output and unrelated changes.

Review follow-up and hosted CI

Follow-up 2df49734 adds JADX to the guide, matching DOCTOR_PROVIDER_IDS, and resets process.exitCode to zero before every CLI invocation so each configuration independently proves its exit status. The error producer and projection are unchanged. The corrected CLI regression (2 tests covering 10 configurations), src/doctorRuntime.test.ts (supported-provider contract), npm run check, npm run docs:check, and the pre-push check:fast all passed.

On the original head f17c87ad, hosted static/build/docs, native Linux package, and both matching real binary-layout/recorded-crash lanes passed. Overall CI did not pass: the source suite has five failures in IDA admission and JavaScript graph/source semantics. I reproduced the same five failing tests with 87 passing tests on clean main 70771523, the exact base used by hosted merge cb6007a, using:

npm run test:local -- src/ida/IdaNativeProtocol.test.ts src/domain/javascript/electronWrappedCallPaths.test.ts src/domain/javascript/javascriptSemanticAnalysis.native_bindings.test.ts src/domain/javascript/javascriptSemanticAmbientWrites.test.ts src/application/javascript/JavaScriptSemanticGraphResources.test.ts

Original source-suite job. A compiled shard also reports failures in development-runner formatting assertions, source-map/contextBridge expectations, and Ghidra launch diagnostics; those additional failures have not all been reproduced locally. This is not a request to waive checks. The updated head's hosted checks are separate and must pass or receive explicit maintainer adjudication before merge.

Latest hosted result

Run 38499905582 for 2df49734 has completed. Static checks, runtime build, docs generation, Windows native build, native Linux package baseline, and both matching real binary-layout/recorded-crash lanes passed. Source tests and shard 4 failed; other shards were cancelled, so overall required CI is failing.

The source job reports the same five source failures reproduced on exact base 70771523 above. Shard 4 reports nine failures across development-runner color assertions, application/source-map expectations and Ghidra launch diagnostics; I have not locally reproduced all of them. Existing #3080 contains repairs in those owners; this PR does not duplicate that work or claim its results. Both review threads on this PR are resolved, and automated review of 2df49734 reported no new blocker.

The author of overlapping #3088 has closed it in favor of this PR. Their separately reported Linux CLI/pwntools checks concern the overlapping recovery approach, not execution of this PR's exact head.

_{🔄 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/3087 **Author:** [@1052326311](https://github.com/1052326311) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/pwntools-remediation` --- ### 📝 Commits (3) - [`f17c87a`](https://github.com/morluto/rea/commit/f17c87ad40ffd7682ab458d75a273a1860b69f7d) fix(pwntools): surface actionable configuration remediation - [`2df4973`](https://github.com/morluto/rea/commit/2df497345b72d76b0d35c99bd97f68fde1f91f5d) fix(pwntools): tighten readiness documentation and exit assertions - [`bcc370a`](https://github.com/morluto/rea/commit/bcc370a0531a7ca74817c742dd8f47db6c3a30f5) Merge remote-tracking branch 'origin/main' into HEAD ### 📊 Changes **4 files changed** (+126 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `docs/cli.md` (+7 -3) 📝 `src/domain/providerSelectionError.ts` (+3 -1) 📝 `src/native/pwntools/PwntoolsFailures.ts` (+1 -0) ➕ `tests/process-global/pwntoolsCliRemediation.test.ts` (+115 -0) </details> ### 📄 Description ## Summary Fixes #3055. Pwntools configuration failures now put their actual recovery guidance in the top-level error message and remediation, instead of recommending `rea doctor`, which does not inspect this adapter. ## Problem and expected behavior With `REA_PWNTOOLS_PYTHON` unset, both `inspect-binary-layout` and `inspect-recorded-crash` returned `provider_unavailable` with a generic doctor recommendation. The useful requirement was only in `details.rejections`. The same loss affected missing/non-executable paths and invalid input-limit configuration. The top-level `message` and `remediation.action` now retain the producer's specific guidance, while the error code, candidate identity, original rejection diagnostics and exit status 1 are preserved. For example, an unset variable reports “Set REA_PWNTOOLS_PYTHON to an absolute caller-supplied Python executable with pwntools 4.15.0, pyelftools 0.33 and Unicorn 2.1.2; REA never installs Python or packages.” ## Change and scope `ProviderSelectionError` accepts an optional caller-supplied user message, following the existing tagged-error convention. `pwntoolsUnavailable` supplies its already-owned reason; the shared projection carries it to CLI/MCP consumers. Other provider selection errors retain their existing default messages. No provider IDs or configuration interpretation are hardcoded in the domain layer. The CLI guide now limits `doctor --provider` advice to Hopper/Ghidra/IDA/JADX and directs other adapter callers to the returned configuration guidance. This is an existing diagnostic-contract repair; a new-capability scope decision is not applicable. ## Contract and boundary impact - Semantic owner: `pwntoolsUnavailable` produces the recovery reason; `ProviderSelectionError` previously replaced its user-facing message with a generic fallback. - CLI/MCP contract: more specific existing `message`/`remediation.action`; error schema, codes and original rejection details retained. - Provider/bridge/platform compatibility: configuration admission and subprocess behavior unchanged; no new doctor provider support. - Evidence/provenance: original rejection configuration key/path and captured diagnostics remain preserved. - Target/cleanup authority: no new file access, process action, or installation behavior. - Generated metadata/package/installation impact: no changes required. Expected effects of this correction: - [x] Documentation or usability only; no target interaction. ## Evidence and regression coverage The new CLI regression exercises both commands through their real application services and pwntools providers. Only the composition entry point is injected to supply isolated configuration. Five cases per command cover unset Python, a directory, a missing file, missing execute permission, and an invalid `REA_PWNTOOLS_MAX_INPUT_BYTES`. Assertions check final JSON message/remediation, code, retained rejection/configuration identity and exit status 1. - On unchanged production base `c7105d97`, both command regressions fail because the top-level advice is generic. - Candidate focused suites: 80 tests passed, including existing projection, input-limit and CLI finalization coverage. - Environment: real macOS filesystem and permissions with the Linux x64 admission gate simulated, matching existing process-global tests. Python, pwntools and GDB were not started; this does not claim a live Linux/provider verification. ## Validation performed - `npm run test:focused -- tests/process-global/pwntoolsCliRemediation.test.ts tests/process-global/pwntoolsCliFinalization.test.ts src/domain/analysisErrorProjection.test.ts src/native/pwntools/PwntoolsInputLimit.test.ts` — 80 passed. - `npm run check` — passed, including runtime build, typecheck/lint/format and architecture guards. - `npm run docs:check` — passed, including 5 README consistency tests. - `npm run verify:test-discovery` — all 1,234 test files discovered exactly once. - `git diff --check` — passed. Validated against base `c7105d97`. Publication refresh through main `70771523` found no changes to the selected error producer, domain error, service, or guide. New unrelated dependency/owner changes were not locally revalidated as part of this candidate. ## Compatibility, safety, and release No schema, exit status, configuration, authority or dependency change. Callers should use structured error codes rather than exact prose. Windows, native Linux x64 and real pwntools/pwndbg execution were not verified for this diagnostic-only change. ## Before merge - [x] Concrete issue, focused outcome and conventional title. - [x] Existing scope and authority preserved; no new provider/capability approval needed. - [x] Observable CLI regressions fail on base and pass on candidate. - [x] Owning error producer and affected consumer inspected; shared projection reused. - [x] Documentation updated, representative output and validation limits stated. - [x] Diff inspected for secrets, generated output and unrelated changes. ## Review follow-up and hosted CI Follow-up `2df49734` adds JADX to the guide, matching `DOCTOR_PROVIDER_IDS`, and resets `process.exitCode` to zero before every CLI invocation so each configuration independently proves its exit status. The error producer and projection are unchanged. The corrected CLI regression (2 tests covering 10 configurations), `src/doctorRuntime.test.ts` (supported-provider contract), `npm run check`, `npm run docs:check`, and the pre-push `check:fast` all passed. On the original head `f17c87ad`, hosted static/build/docs, native Linux package, and both matching real binary-layout/recorded-crash lanes passed. Overall CI did not pass: the source suite has five failures in IDA admission and JavaScript graph/source semantics. I reproduced the same five failing tests with 87 passing tests on clean main `70771523`, the exact base used by hosted merge `cb6007a`, using: ```text npm run test:local -- src/ida/IdaNativeProtocol.test.ts src/domain/javascript/electronWrappedCallPaths.test.ts src/domain/javascript/javascriptSemanticAnalysis.native_bindings.test.ts src/domain/javascript/javascriptSemanticAmbientWrites.test.ts src/application/javascript/JavaScriptSemanticGraphResources.test.ts ``` [Original source-suite job](https://github.com/morluto/rea/actions/runs/38499355621/job/115642439398). A compiled shard also reports failures in development-runner formatting assertions, source-map/contextBridge expectations, and Ghidra launch diagnostics; those additional failures have not all been reproduced locally. This is not a request to waive checks. The updated head's hosted checks are separate and must pass or receive explicit maintainer adjudication before merge. ### Latest hosted result Run [38499905582](https://github.com/morluto/rea/actions/runs/38499905582) for `2df49734` has completed. Static checks, runtime build, docs generation, Windows native build, native Linux package baseline, and both matching real binary-layout/recorded-crash lanes passed. Source tests and shard 4 failed; other shards were cancelled, so overall required CI is failing. The source job reports the same five source failures reproduced on exact base `70771523` above. Shard 4 reports nine failures across development-runner color assertions, application/source-map expectations and Ghidra launch diagnostics; I have not locally reproduced all of them. Existing #3080 contains repairs in those owners; this PR does not duplicate that work or claim its results. Both review threads on this PR are resolved, and automated review of `2df49734` reported no new blocker. The author of overlapping #3088 has [closed it in favor of this PR](https://github.com/morluto/rea/pull/3088#issuecomment-6159465753). Their separately reported Linux CLI/pwntools checks concern the overlapping recovery approach, not execution of this PR's exact head. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:07:55 +00:00