[PR #4290] [MERGED] fix(native): report PE input byte budgets as file-size limits #6161

Closed
opened 2026-10-31 01:12:20 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4290
Author: @Henglie
Created: 10/16/2026
Status: ✅ Merged
Merged: 10/16/2026
Merged by: @morluto

Base: main ← Head: fix/pe-input-byte-budget-file-size


📝 Commits (3)

  • ac530e7 fix(native): report PE input byte budgets as file-size limits
  • 65b6bfc fix(native): keep caller-selected PE budget refusals actionable
  • 830950a fix(native): preserve budget retryability and name the schema ceiling

📊 Changes

10 files changed (+265 additions, -11 deletions)

View changed files

📝 src/artifacts/ArtifactReader.ts (+16 -0)
📝 src/artifacts/readStableArtifact.ts (+2 -0)
📝 src/domain/analysisErrorCore.ts (+11 -1)
📝 src/domain/analysisErrorProjection.ts (+1 -1)
📝 src/domain/explicitSensitiveFailure.ts (+3 -0)
📝 src/domain/native/peResources.ts (+7 -2)
📝 src/kuna/KunaQueryErrors.ts (+6 -1)
📝 src/native/pe/PeResourceParser.ts (+6 -3)
➕ src/native/pe/PeResourcesProvider.test.ts (+174 -0)
📝 src/native/pe/PeResourcesProvider.ts (+39 -3)

📄 Description

Summary

inspect_pe_resources refused an image above the caller's max_file_bytes with
details.resource: "memory" and the memory remediation advice. The bound that
was actually exceeded is the input file's byte length, so callers were told to
review worker/host memory for a budget they can raise with a flag.

The reader now records which bound it hit, and the PE provider reports that
bound instead of assuming memory.

Fixes #4192.

Problem and expected behavior

PeResourcesProvider translated every ArtifactReaderFailure with
reason: "limit" into AnalysisResourceConstraintError(operation, "memory", …).
Two different limits reach that line:

  • the input byte budget, from readStableArtifact(path, input.max_file_bytes, …)
    — "Selected artifact exceeds N bytes" and "Selected artifact grew beyond its
    byte limit";
  • the parser's own budgets — the 8 MiB metadata allocation and the
    max_entries directory-entry budget.

Reporting the first as memory sends callers to the wrong remediation, and it
was reproduced through both the compiled CLI and production stdio MCP for the
PE32 and PE32+ fixtures. Expected: a caller-selected input byte budget is
reported as a file-size bound, an entry budget as a capacity bound, and the
metadata budget stays memory.

Change and scope

  • src/artifacts/ArtifactReader.ts: ArtifactReaderFailure gains an optional
    boundary (file-size | capacity | memory) on its options, mirroring the
    typed boundary already used by ElectronFuseReader. Readers that do not set
    it are unaffected, and existing consumers that ignore it keep their behaviour.
  • src/artifacts/readStableArtifact.ts: both input-byte-budget refusals declare
    boundary: "file-size".
  • src/native/pe/PeResourceParser.ts: the metadata budget and the final result
    budget declare "memory"; the max_entries budget declares "capacity".
  • src/native/pe/PeResourcesProvider.ts: reports cause.boundary ?? "memory"
    instead of the hard-coded "memory".
  • src/native/pe/PeResourcesProvider.test.ts (new): three regression tests at
    the provider boundary.

No schema, tool contract, CLI flag or provider identity changes. This is a fix
inside existing boundaries, so no maintainer scope decision is required
("not applicable").

Contract and boundary impact

  • Semantic owner and earliest changed stage: provider failure translation in
    inspect_pe_resources, after artifact reading and parsing.
  • Contribution scope fit and linked maintainer decision: not applicable.
  • CLI and MCP/tool-catalog contract: the error envelope is unchanged in shape.
    The observable difference is the value of details.resource for two refusals:
    memory → file-size for the input byte budget, and memory → capacity
    for the entry budget. reportedLimits still carries max_file_bytes and
    max_entries.
  • Provider, bridge, target-format, or platform compatibility: none; the
    readers stay portable and the classification is provider-neutral.
  • Evidence, artifact, provenance, or reconstruction contract: unchanged. The
    failure still carries the underlying ArtifactReaderFailure as its cause and
    still returns no partial inventory.
  • Target selection, authority, upstream behavior, and cleanup details: not
    applicable; no execution, no writes.
  • Generated metadata: none.

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.

Evidence and regression coverage

  • Tests added: src/native/pe/PeResourcesProvider.test.ts (new, 3 tests),
    asserting the reported resource and message for the input byte budget and for
    the entry budget, and that a file-size refusal no longer mentions memory.
  • Base reproduction: reverting only the provider translation makes all three
    fail with resource: "memory"; restoring it makes all three pass. I ran both
    directions.
  • Regression coverage of the shared reader:
    tests/boundary/filesystem/stableArtifact.test.ts (5 tests, 1 pre-existing
    skip), src/native/pe/PeResourceParser.test.ts (28),
    src/application/binaryDiagnostics/PeResourcesService.test.ts (5) and
    tests/boundary/filesystem/peResources.test.ts (3) all still pass — 43
    passed, 1 skipped.
  • Remaining proof gaps: not re-run through the compiled CLI and stdio MCP here
    (test:acceptance needs a build in this environment); the reproduction is at
    the provider boundary both adapters share, and the issue's own report
    confirms both adapters reproduced the old text.

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 test:focused -- src/native/pe/PeResourcesProvider.test.ts — 3
    passed; all 3 fail when the fix is reverted.
  • npm run test:focused -- src/native/pe/PeResourceParser.test.ts tests/boundary/filesystem/stableArtifact.test.ts tests/boundary/filesystem/peResources.test.ts src/application/binaryDiagnostics/PeResourcesService.test.ts — 43 passed,
    1 pre-existing skip.
  • npx tsc --noEmit — no errors in the changed files.
  • npx oxlint src/native/pe/ src/artifacts/ — 0 errors.
  • npx oxfmt <changed files> — applied.

Not run, and not claimed as passing: npm run check:fast (its deps:check
pre-step fails in this environment) and npm run test:acceptance (requires a
build). Expected to be covered by CI.

Compatibility, safety, and release

  • Breaking changes or migration steps: none, but two refusals change their
    reported resource kind. Callers that branch on
    details.resource === "memory" for this operation would now see
    file-size or capacity; that is the point of the change, and both kinds are
    already declared in ANALYSIS_RESOURCE_KINDS.
  • Real Hopper/Ghidra, browser, or OS coverage: not required; no provider engine
    is involved.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: classification of an
    existing refusal only; no new reads, writes or execution.

Author readiness

Human owner: @Henglie

  • 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/4290 **Author:** [@Henglie](https://github.com/Henglie) **Created:** 10/16/2026 **Status:** ✅ Merged **Merged:** 10/16/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/pe-input-byte-budget-file-size` --- ### 📝 Commits (3) - [`ac530e7`](https://github.com/morluto/rea/commit/ac530e7d1ccfd7cf359c5e2dba8e2baec3c0707f) fix(native): report PE input byte budgets as file-size limits - [`65b6bfc`](https://github.com/morluto/rea/commit/65b6bfc70a408d51fe887eedd69f6567428c5a22) fix(native): keep caller-selected PE budget refusals actionable - [`830950a`](https://github.com/morluto/rea/commit/830950ac796a635897442947513025a181505907) fix(native): preserve budget retryability and name the schema ceiling ### 📊 Changes **10 files changed** (+265 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/ArtifactReader.ts` (+16 -0) 📝 `src/artifacts/readStableArtifact.ts` (+2 -0) 📝 `src/domain/analysisErrorCore.ts` (+11 -1) 📝 `src/domain/analysisErrorProjection.ts` (+1 -1) 📝 `src/domain/explicitSensitiveFailure.ts` (+3 -0) 📝 `src/domain/native/peResources.ts` (+7 -2) 📝 `src/kuna/KunaQueryErrors.ts` (+6 -1) 📝 `src/native/pe/PeResourceParser.ts` (+6 -3) ➕ `src/native/pe/PeResourcesProvider.test.ts` (+174 -0) 📝 `src/native/pe/PeResourcesProvider.ts` (+39 -3) </details> ### 📄 Description ## Summary `inspect_pe_resources` refused an image above the caller's `max_file_bytes` with `details.resource: "memory"` and the memory remediation advice. The bound that was actually exceeded is the input file's byte length, so callers were told to review worker/host memory for a budget they can raise with a flag. The reader now records which bound it hit, and the PE provider reports that bound instead of assuming memory. Fixes #4192. ## Problem and expected behavior `PeResourcesProvider` translated every `ArtifactReaderFailure` with `reason: "limit"` into `AnalysisResourceConstraintError(operation, "memory", …)`. Two different limits reach that line: - the input byte budget, from `readStableArtifact(path, input.max_file_bytes, …)` — "Selected artifact exceeds N bytes" and "Selected artifact grew beyond its byte limit"; - the parser's own budgets — the 8 MiB metadata allocation and the `max_entries` directory-entry budget. Reporting the first as `memory` sends callers to the wrong remediation, and it was reproduced through both the compiled CLI and production stdio MCP for the PE32 and PE32+ fixtures. Expected: a caller-selected input byte budget is reported as a file-size bound, an entry budget as a capacity bound, and the metadata budget stays memory. ## Change and scope - `src/artifacts/ArtifactReader.ts`: `ArtifactReaderFailure` gains an optional `boundary` (`file-size` | `capacity` | `memory`) on its options, mirroring the typed `boundary` already used by `ElectronFuseReader`. Readers that do not set it are unaffected, and existing consumers that ignore it keep their behaviour. - `src/artifacts/readStableArtifact.ts`: both input-byte-budget refusals declare `boundary: "file-size"`. - `src/native/pe/PeResourceParser.ts`: the metadata budget and the final result budget declare `"memory"`; the `max_entries` budget declares `"capacity"`. - `src/native/pe/PeResourcesProvider.ts`: reports `cause.boundary ?? "memory"` instead of the hard-coded `"memory"`. - `src/native/pe/PeResourcesProvider.test.ts` (new): three regression tests at the provider boundary. No schema, tool contract, CLI flag or provider identity changes. This is a fix inside existing boundaries, so no maintainer scope decision is required ("not applicable"). ## Contract and boundary impact - Semantic owner and earliest changed stage: provider failure translation in `inspect_pe_resources`, after artifact reading and parsing. - Contribution scope fit and linked maintainer decision: not applicable. - CLI and MCP/tool-catalog contract: the error envelope is unchanged in shape. The observable difference is the value of `details.resource` for two refusals: `memory` → `file-size` for the input byte budget, and `memory` → `capacity` for the entry budget. `reportedLimits` still carries `max_file_bytes` and `max_entries`. - Provider, bridge, target-format, or platform compatibility: none; the readers stay portable and the classification is provider-neutral. - Evidence, artifact, provenance, or reconstruction contract: unchanged. The failure still carries the underlying `ArtifactReaderFailure` as its cause and still returns no partial inventory. - Target selection, authority, upstream behavior, and cleanup details: not applicable; no execution, no writes. - Generated metadata: none. 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. ## Evidence and regression coverage - Tests added: `src/native/pe/PeResourcesProvider.test.ts` (new, 3 tests), asserting the reported resource and message for the input byte budget and for the entry budget, and that a file-size refusal no longer mentions memory. - Base reproduction: reverting only the provider translation makes all three fail with `resource: "memory"`; restoring it makes all three pass. I ran both directions. - Regression coverage of the shared reader: `tests/boundary/filesystem/stableArtifact.test.ts` (5 tests, 1 pre-existing skip), `src/native/pe/PeResourceParser.test.ts` (28), `src/application/binaryDiagnostics/PeResourcesService.test.ts` (5) and `tests/boundary/filesystem/peResources.test.ts` (3) all still pass — 43 passed, 1 skipped. - Remaining proof gaps: not re-run through the compiled CLI and stdio MCP here (`test:acceptance` needs a build in this environment); the reproduction is at the provider boundary both adapters share, and the issue's own report confirms both adapters reproduced the old text. For evidence-bearing changes: - [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 - `npm run test:focused -- src/native/pe/PeResourcesProvider.test.ts` — 3 passed; all 3 fail when the fix is reverted. - `npm run test:focused -- src/native/pe/PeResourceParser.test.ts tests/boundary/filesystem/stableArtifact.test.ts tests/boundary/filesystem/peResources.test.ts src/application/binaryDiagnostics/PeResourcesService.test.ts` — 43 passed, 1 pre-existing skip. - `npx tsc --noEmit` — no errors in the changed files. - `npx oxlint src/native/pe/ src/artifacts/` — 0 errors. - `npx oxfmt <changed files>` — applied. Not run, and not claimed as passing: `npm run check:fast` (its `deps:check` pre-step fails in this environment) and `npm run test:acceptance` (requires a build). Expected to be covered by CI. ## Compatibility, safety, and release - Breaking changes or migration steps: none, but two refusals change their reported resource kind. Callers that branch on `details.resource === "memory"` for this operation would now see `file-size` or `capacity`; that is the point of the change, and both kinds are already declared in `ANALYSIS_RESOURCE_KINDS`. - Real Hopper/Ghidra, browser, or OS coverage: not required; no provider engine is involved. - Package or release metadata impact: none. - Security, privacy, process, or containment review: classification of an existing refusal only; no new reads, writes or execution. ## Author readiness Human owner: @Henglie - [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. - [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-10-31 01:12:20 +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#6161
No description provided.