morluto/rea

Fork 0

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

[PR #5860] [MERGED] fix(errors): name fixed plist decode allowances instead of host memory in recovery #7090

New issue

Closed

opened 2026-11-01 21:45:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5860
Author: @kaoru0822-kitauji
Created: 10/21/2026
Status: ✅ Merged
Merged: 10/21/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/plist-fixed-allowance-guidance

📝 Commits (3)

85595a3 fix(errors): derive fixed-allowance recovery from a typed refusal field
060ac79 Merge remote-tracking branch 'origin/main' into fix/plist-fixed-allowance-guidance
5163f22 Merge remote-tracking branch 'origin/main' into fix/plist-fixed-allowance-guidance

📊 Changes

16 files changed (+364 additions, -72 deletions)

View changed files

📝 docs/native-investigation.md (+7 -2)
📝 src/artifacts/apple/PropertyListDecodeBudget.ts (+4 -1)
📝 src/domain/analysisErrorCore.ts (+47 -4)
📝 src/domain/analysisErrorPresentation.ts (+15 -9)
📝 src/domain/explicitSensitiveFailure.ts (+2 -9)
📝 src/domain/fixedAllowanceRemediation.ts (+9 -2)
📝 src/ida/IdaSessionClient.ts (+14 -11)
📝 src/native/NativePlistSource.ts (+24 -10)
📝 src/native/parsers/plist.ts (+20 -2)
📝 src/providers/AnalysisClientFailure.ts (+2 -9)
📝 tests/boundary/captures/captureProvider.test.ts (+3 -2)
📝 tests/boundary/cli/nativePlistSelectionFailureCli.test.ts (+76 -1)
📝 tests/boundary/il2cpp/il2cppGhidraSeeds.test.ts (+14 -7)
➕ tests/boundary/providers/fixedAllowanceRecovery.test.ts (+55 -0)
📝 tests/boundary/providers/native/nativePlistBinarySource.test.ts (+69 -1)
📝 tests/boundary/signals/recordingProvider.test.ts (+3 -2)

📄 Description

What broke: inspect_plist refusals at fixed allowances (64 MiB decode estimate, 128-level nesting, 64 MiB source size) told callers to add worker/host memory or raise a worker file-size allowance. Neither raises these allowances.
Fix: AnalysisResourceConstraintError now has a typed fixedAllowance { name, kind } field, and the shared presentation builds the recovery text from it. fixedAllowanceOptions sets this field, so all existing fixed-allowance producers use it. The plist decoder and source reader now set it too.
Risk: Low. Wording and reported_limits change for the plist refusals only. Codes, categories and retryable: false stay the same. Three re-attribution/redaction copy sites now carry the field through one helper.

Summary

Fixes #5847

Before this change, inspect_plist refused a valid binary plist whose decode estimate was over the fixed 64 MiB allowance. The refusal said resource: "memory" and gave no recovery, so the projection fell back to "Review the reported worker memory limits and available host memory. Retry with sufficient memory…". Repeating the request after adding memory fails the same way. Now the recovery names the fixed allowance and says that memory changes don't raise it.

Problem and expected behavior

Trigger: a 23,068,738-byte binary plist with one 22 MiB data leaf. It is below the 64 MiB input ceiling, but 3× the source length is above the 64 MiB decode estimate.
Invariant broken: recovery advice has to match the limit that actually refused the request. A fixed allowance needs fixed-allowance recovery.
Related cases at the same producer, also reproduced:
- Nesting deeper than 128 levels got the same host-memory advice. It also reported maximum_decoded_bytes, which is a limit it did not exceed.
- A source above 64 MiB got "Retry with a sufficient file-size allowance for the evidence reply".

Change and scope

src/domain/analysisErrorCore.ts: AnalysisResourceConstraintError.fixedAllowance holds the refusal's allowance. The recovery options are a union: either a bespoke remediationAction or a fixedAllowance, never both. resourceConstraintRecovery() copies the recovery onto a re-attributed or redacted error, with optional text redaction.
src/domain/analysisErrorPresentation.ts: the recovery text now comes from fixedAllowance (through the existing fixedAllowanceRemediation) before falling back to the resource kind. The text no longer lives inside each error as a prebuilt string.
src/domain/fixedAllowanceRemediation.ts: fixedAllowanceOptions returns { retryable: false, fixedAllowance }. All ~30 existing call sites (pcap, tshark, adb, apktool, sigrok, Go, Rust, IL2CPP, WASM, Electron, managed PE, native capture, …) keep their wording and now share the typed field.
Copy sites: src/providers/AnalysisClientFailure.ts, src/ida/IdaSessionClient.ts and src/domain/explicitSensitiveFailure.ts now use the one helper. Before, each rebuilt the options by hand. Redaction still applies to the allowance name.
Plist producers:
- src/native/parsers/plist.ts: decode and nesting refusals set the field. Nesting now reports maximum_nesting_depth: 128, exported as MAX_BINARY_PLIST_NESTING_DEPTH from the shared budget walker.
- src/native/NativePlistSource.ts: the source-size refusal uses the input allowance kind.
docs/native-investigation.md: documents the three reported limits and that they are fixed.
Contribution scope: an ordinary fix within existing boundaries. Maintainer decision: not applicable.
Not included:
- The error contract does not expose fixedAllowance as a public details field. It stays an internal type that drives remediation.action.
- Related issues are left alone:
  - #5846 and #5848 have different producers, which return unreadable_output.
  - #3538 already has open PR #3550.
  - The #5631 sites already call fixedAllowanceOptions on main. They now pick up the typed field with unchanged wording.

Contract and boundary impact

Semantic owner and earliest changed stage: AnalysisResourceConstraintError (domain) and the plist decode/source producers.
Contribution scope fit and linked maintainer decision: existing boundary; not applicable.
CLI and MCP/tool-catalog contract: remediation.action text changes for inspect_plist fixed-allowance refusals. A nesting refusal now reports reported_limits.maximum_nesting_depth instead of maximum_decoded_bytes. No schema or catalog change.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details: unchanged.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.

Evidence and regression coverage

Tests added or updated:
- tests/boundary/cli/nativePlistSelectionFailureCli.test.ts (darwin CLI workflow): runs the 22 MiB decode refusal twice and checks identical JSON with the exact allowance recovery. Also covers the input-ceiling refusal.
- tests/boundary/providers/native/nativePlistBinarySource.test.ts (all platforms): decode and nesting refusals, with typed field, reported limits and projected recovery. The source-size test now also checks the projected recovery.
- tests/boundary/providers/fixedAllowanceRecovery.test.ts: the typed allowance and its recovery survive operation re-attribution and explicit redaction.
- The capture, sigrok and IL2CPP tests used to read the prebuilt remediationAction string. They now check the derived recovery.
Fails on base: I reverted src/ to origin/main and kept the new tests. Both new CLI tests failed. The provider-boundary file failed on the missing nesting-depth export, which is part of the fix.
Before (real CLI, node scripts/rea.mjs inspect-plist large.plist --json, main @ f801b2fbf):

{"code":"resource_constraint","message":"binary plist object table exceeds the native plist decode budget","retryable":false,"remediation":{"action":"Review the reported worker memory limits and available host memory. Retry with sufficient memory or a smaller artifact; REA retains tighter inherited limits."},"details":{"resource":"memory","reported_limits":{"maximum_decoded_bytes":67108864}}}

After (same input, two runs, identical output, exit 1, empty stderr):

{"code":"resource_constraint","message":"binary plist object table exceeds the native plist decode budget","retryable":false,"remediation":{"action":"The 64 MiB native binary plist decode allowance is a fixed allowance. Worker memory limits, inherited limits, and additional host memory do not raise it. Select another complete input within the allowance when that preserves the needed observation. Repeating the unchanged request fails the same way."},"details":{"resource":"memory","reported_limits":{"maximum_decoded_bytes":67108864}}}

After, 140-level nesting: "The 128-level native binary plist nesting allowance is a fixed allowance. …" with reported_limits: {"maximum_nesting_depth":128}. Before, this got the host-memory advice and maximum_decoded_bytes.
After, 68,157,506-byte source: "Select a suitable complete artifact within the fixed 64 MiB native plist input allowance. Increasing an evidence-reply file-size allowance does not admit this input. …". Before: "Review the reported worker file-size limits…".
Control: a 70-byte binary plist still exits 0 and returns the exact data leaf.
Remaining proof gaps:
- No live MCP client run. MCP shares the same projection, which projectAnalysisError assertions cover.
- CLI tests run only on darwin. The provider-boundary tests run everywhere.
- I did not measure tokens. The recovery text is about 50 bytes longer.
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

Real CLI repro before and after on macOS arm64, Node v26.5.0, with fixtures generated by python3 -I + plistlib:
- Large/deep/huge inputs: before, wrong recovery; after, fixed-allowance recovery as shown above.
- Small control exits 0.
- Elapsed 1.2–1.8 s and max RSS 343–366 MB, unchanged from before.
npm run test:focused on the plist CLI/provider/MCP/acceptance suites and the new recovery test: 7 files, 50 tests passed.
npm run test:focused on all 356 src test files under domain, go, rust, flutter, electron, dotnet, ida, pcap, apktool, adb, native, captures, signals and kuna: 7151 passed.
npm run test:focused on 110 boundary files under go, rust, il2cpp, pcap, signals, captures, providers/{ida,kuna,native}, plus the plist MCP and acceptance tests: 1231 passed, 44 skipped.
Base check with src/ reverted: the 2 new CLI tests failed, and the provider-boundary file failed to load.
npm run check:fast — passed
npm run format:check — passed
npm run docs:check — passed

Compatibility, safety, and release

Breaking changes or migration steps: none. Internal callers that read AnalysisResourceConstraintError.remediationAction for fixed allowances should use analysisErrorRemediationAction(error) instead. All in-repo readers are updated.
Real Hopper/Ghidra, browser, or OS coverage: the native macOS provider only. No external engine.
Package or release metadata impact: none.
Security, privacy, process, or containment review: redaction still covers the allowance name. Process and file behavior are unchanged.

Author readiness

Human owner: @kaoru0822-kitauji

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.

🤖 Generated with Claude Code

_{🔄 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/5860 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/21/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/plist-fixed-allowance-guidance` --- ### 📝 Commits (3) - [`85595a3`](https://github.com/morluto/rea/commit/85595a342d42bc45f15a1c1909444c3eb48f9869) fix(errors): derive fixed-allowance recovery from a typed refusal field - [`060ac79`](https://github.com/morluto/rea/commit/060ac7924e42528fca4cc6478bcc03381b3c0945) Merge remote-tracking branch 'origin/main' into fix/plist-fixed-allowance-guidance - [`5163f22`](https://github.com/morluto/rea/commit/5163f22e7027a477f1559374c9b22e7606b81e59) Merge remote-tracking branch 'origin/main' into fix/plist-fixed-allowance-guidance ### 📊 Changes **16 files changed** (+364 additions, -72 deletions) <details> <summary>View changed files</summary> 📝 `docs/native-investigation.md` (+7 -2) 📝 `src/artifacts/apple/PropertyListDecodeBudget.ts` (+4 -1) 📝 `src/domain/analysisErrorCore.ts` (+47 -4) 📝 `src/domain/analysisErrorPresentation.ts` (+15 -9) 📝 `src/domain/explicitSensitiveFailure.ts` (+2 -9) 📝 `src/domain/fixedAllowanceRemediation.ts` (+9 -2) 📝 `src/ida/IdaSessionClient.ts` (+14 -11) 📝 `src/native/NativePlistSource.ts` (+24 -10) 📝 `src/native/parsers/plist.ts` (+20 -2) 📝 `src/providers/AnalysisClientFailure.ts` (+2 -9) 📝 `tests/boundary/captures/captureProvider.test.ts` (+3 -2) 📝 `tests/boundary/cli/nativePlistSelectionFailureCli.test.ts` (+76 -1) 📝 `tests/boundary/il2cpp/il2cppGhidraSeeds.test.ts` (+14 -7) ➕ `tests/boundary/providers/fixedAllowanceRecovery.test.ts` (+55 -0) 📝 `tests/boundary/providers/native/nativePlistBinarySource.test.ts` (+69 -1) 📝 `tests/boundary/signals/recordingProvider.test.ts` (+3 -2) </details> ### 📄 Description **What broke:** `inspect_plist` refusals at fixed allowances (64 MiB decode estimate, 128-level nesting, 64 MiB source size) told callers to add worker/host memory or raise a worker file-size allowance. Neither raises these allowances. **Fix:** `AnalysisResourceConstraintError` now has a typed `fixedAllowance { name, kind }` field, and the shared presentation builds the recovery text from it. `fixedAllowanceOptions` sets this field, so all existing fixed-allowance producers use it. The plist decoder and source reader now set it too. **Risk:** Low. Wording and `reported_limits` change for the plist refusals only. Codes, categories and `retryable: false` stay the same. Three re-attribution/redaction copy sites now carry the field through one helper. ## Summary Fixes #5847 Before this change, `inspect_plist` refused a valid binary plist whose decode estimate was over the fixed 64 MiB allowance. The refusal said `resource: "memory"` and gave no recovery, so the projection fell back to "Review the reported worker memory limits and available host memory. Retry with sufficient memory…". Repeating the request after adding memory fails the same way. Now the recovery names the fixed allowance and says that memory changes don't raise it. ## Problem and expected behavior - Trigger: a 23,068,738-byte binary plist with one 22 MiB data leaf. It is below the 64 MiB input ceiling, but 3× the source length is above the 64 MiB decode estimate. - Invariant broken: recovery advice has to match the limit that actually refused the request. A fixed allowance needs fixed-allowance recovery. - Related cases at the same producer, also reproduced: - Nesting deeper than 128 levels got the same host-memory advice. It also reported `maximum_decoded_bytes`, which is a limit it did not exceed. - A source above 64 MiB got "Retry with a sufficient file-size allowance for the evidence reply". ## Change and scope - `src/domain/analysisErrorCore.ts`: `AnalysisResourceConstraintError.fixedAllowance` holds the refusal's allowance. The recovery options are a union: either a bespoke `remediationAction` or a `fixedAllowance`, never both. `resourceConstraintRecovery()` copies the recovery onto a re-attributed or redacted error, with optional text redaction. - `src/domain/analysisErrorPresentation.ts`: the recovery text now comes from `fixedAllowance` (through the existing `fixedAllowanceRemediation`) before falling back to the resource kind. The text no longer lives inside each error as a prebuilt string. - `src/domain/fixedAllowanceRemediation.ts`: `fixedAllowanceOptions` returns `{ retryable: false, fixedAllowance }`. All ~30 existing call sites (pcap, tshark, adb, apktool, sigrok, Go, Rust, IL2CPP, WASM, Electron, managed PE, native capture, …) keep their wording and now share the typed field. - Copy sites: `src/providers/AnalysisClientFailure.ts`, `src/ida/IdaSessionClient.ts` and `src/domain/explicitSensitiveFailure.ts` now use the one helper. Before, each rebuilt the options by hand. Redaction still applies to the allowance name. - Plist producers: - `src/native/parsers/plist.ts`: decode and nesting refusals set the field. Nesting now reports `maximum_nesting_depth: 128`, exported as `MAX_BINARY_PLIST_NESTING_DEPTH` from the shared budget walker. - `src/native/NativePlistSource.ts`: the source-size refusal uses the `input` allowance kind. - `docs/native-investigation.md`: documents the three reported limits and that they are fixed. - Contribution scope: an ordinary fix within existing boundaries. Maintainer decision: not applicable. - Not included: - The error contract does not expose `fixedAllowance` as a public `details` field. It stays an internal type that drives `remediation.action`. - Related issues are left alone: - #5846 and #5848 have different producers, which return `unreadable_output`. - #3538 already has open PR #3550. - The #5631 sites already call `fixedAllowanceOptions` on main. They now pick up the typed field with unchanged wording. ## Contract and boundary impact - Semantic owner and earliest changed stage: `AnalysisResourceConstraintError` (domain) and the plist decode/source producers. - Contribution scope fit and linked maintainer decision: existing boundary; not applicable. - CLI and MCP/tool-catalog contract: `remediation.action` text changes for `inspect_plist` fixed-allowance refusals. A nesting refusal now reports `reported_limits.maximum_nesting_depth` instead of `maximum_decoded_bytes`. No schema or catalog change. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: unchanged. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/cli/nativePlistSelectionFailureCli.test.ts` (darwin CLI workflow): runs the 22 MiB decode refusal twice and checks identical JSON with the exact allowance recovery. Also covers the input-ceiling refusal. - `tests/boundary/providers/native/nativePlistBinarySource.test.ts` (all platforms): decode and nesting refusals, with typed field, reported limits and projected recovery. The source-size test now also checks the projected recovery. - `tests/boundary/providers/fixedAllowanceRecovery.test.ts`: the typed allowance and its recovery survive operation re-attribution and explicit redaction. - The capture, sigrok and IL2CPP tests used to read the prebuilt `remediationAction` string. They now check the derived recovery. - Fails on base: I reverted `src/` to `origin/main` and kept the new tests. Both new CLI tests failed. The provider-boundary file failed on the missing nesting-depth export, which is part of the fix. - Before (real CLI, `node scripts/rea.mjs inspect-plist large.plist --json`, main @ f801b2fbf): ```json {"code":"resource_constraint","message":"binary plist object table exceeds the native plist decode budget","retryable":false,"remediation":{"action":"Review the reported worker memory limits and available host memory. Retry with sufficient memory or a smaller artifact; REA retains tighter inherited limits."},"details":{"resource":"memory","reported_limits":{"maximum_decoded_bytes":67108864}}} ``` - After (same input, two runs, identical output, exit 1, empty stderr): ```json {"code":"resource_constraint","message":"binary plist object table exceeds the native plist decode budget","retryable":false,"remediation":{"action":"The 64 MiB native binary plist decode allowance is a fixed allowance. Worker memory limits, inherited limits, and additional host memory do not raise it. Select another complete input within the allowance when that preserves the needed observation. Repeating the unchanged request fails the same way."},"details":{"resource":"memory","reported_limits":{"maximum_decoded_bytes":67108864}}} ``` - After, 140-level nesting: `"The 128-level native binary plist nesting allowance is a fixed allowance. …"` with `reported_limits: {"maximum_nesting_depth":128}`. Before, this got the host-memory advice and `maximum_decoded_bytes`. - After, 68,157,506-byte source: `"Select a suitable complete artifact within the fixed 64 MiB native plist input allowance. Increasing an evidence-reply file-size allowance does not admit this input. …"`. Before: "Review the reported worker file-size limits…". - Control: a 70-byte binary plist still exits 0 and returns the exact data leaf. - Remaining proof gaps: - No live MCP client run. MCP shares the same projection, which `projectAnalysisError` assertions cover. - CLI tests run only on darwin. The provider-boundary tests run everywhere. - I did not measure tokens. The recovery text is about 50 bytes longer. - [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 - Real CLI repro before and after on macOS arm64, Node v26.5.0, with fixtures generated by `python3 -I` + `plistlib`: - Large/deep/huge inputs: before, wrong recovery; after, fixed-allowance recovery as shown above. - Small control exits 0. - Elapsed 1.2–1.8 s and max RSS 343–366 MB, unchanged from before. - `npm run test:focused` on the plist CLI/provider/MCP/acceptance suites and the new recovery test: 7 files, 50 tests passed. - `npm run test:focused` on all 356 `src` test files under domain, go, rust, flutter, electron, dotnet, ida, pcap, apktool, adb, native, captures, signals and kuna: 7151 passed. - `npm run test:focused` on 110 boundary files under go, rust, il2cpp, pcap, signals, captures, providers/{ida,kuna,native}, plus the plist MCP and acceptance tests: 1231 passed, 44 skipped. - Base check with `src/` reverted: the 2 new CLI tests failed, and the provider-boundary file failed to load. - `npm run check:fast` — passed - `npm run format:check` — passed - `npm run docs:check` — passed ## Compatibility, safety, and release - Breaking changes or migration steps: none. Internal callers that read `AnalysisResourceConstraintError.remediationAction` for fixed allowances should use `analysisErrorRemediationAction(error)` instead. All in-repo readers are updated. - Real Hopper/Ghidra, browser, or OS coverage: the native macOS provider only. No external engine. - Package or release metadata impact: none. - Security, privacy, process, or containment review: redaction still covers the allowance name. Process and file behavior are unchanged. ## Author readiness Human owner: @kaoru0822-kitauji - [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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-03 13:39:15 +00:00

[PR #7090] [MERGED] fix(doctor): classify an unreadable skill identity as unknown #7634

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

No description provided.

Rows
Columns