morluto/rea

Fork 0

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

[PR #4114] [MERGED] fix(apple): bound Interface Builder XML plists by shared value depth #6065

New issue

Closed

opened 2026-10-31 01:11:34 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:11:34 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/3949-ib-xml-plist-depth

📝 Commits (2)

cf045f7 fix(apple): bound Interface Builder XML plists by shared value depth
d246c40 fix(apple): return the decode reservation of a depth-rejected archive

📊 Changes

3 files changed (+89 additions, -19 deletions)

View changed files

📝 src/artifacts/apple/InterfaceBuilderAnalysis.ts (+25 -1)
📝 src/artifacts/apple/InterfaceBuilderDecodeBudget.ts (+11 -17)
📝 tests/boundary/filesystem/interfaceBuilderBundle.test.ts (+53 -1)

📄 Description

Summary

Fixes #3949. Interface Builder decoding omitted valid XML plist archives as over budget even though keyed-archive inspection decodes the same files. Its preflight counted raw XML element depth against 128, while REA's shared plist parser bounds zero-based value depth at 128. The plist envelope and dictionary keys add elements but not value depth. The duplicate rule is removed, and the shared parser's limit now decides, so both commands agree.

Problem and expected behavior

The cause was in estimateXmlPlistExpansion (src/artifacts/apple/InterfaceBuilderDecodeBudget.ts). It threw aggregate_decode_budget_exhausted when the open-element depth exceeded 128. At the issue's depth 124, the deepest value sits at plist value depth 127 but XML element depth 129. At depth 125 those are 128 and 130.

Reproduced with the issue's script through the built CLI (decode-interface-builder):

nesting	main	this PR
123	1 document, Build control, `archive_decode` complete	unchanged
124 (value depth 127)	0 documents, `partial`, `aggregate_decode_budget_exhausted`, omitted 1	1 document, Build control, complete
125 (value depth 128)	0 documents, `partial`, budget exhausted	1 document, Build control, complete
126 (value depth 129)	0 documents, `partial`, budget exhausted	unchanged: explicit budget omission

At 126, inspect-keyed-archive also refuses the archive (Plist exceeds 128 nesting levels), so both commands now share one limit.

Change and scope

Removed the element-depth check from the IB XML preflight, along with its now-unused closing/selfClosing token fields. The preflight still applies the representation-size estimate and the DOCTYPE internal-subset refusal.
The value-depth limit belongs to parseXmlPropertyList, which already bounds nesting before its recursive decode. IB's XML decode path converts that RangeError into the existing InterfaceBuilderDecodeBudgetExceeded("aggregate_decode_budget_exhausted", …). A too-deep archive therefore stays an explicit omission with partial coverage, not an "invalid" archive. The repository already treats a plist decode RangeError as a resource limit (KeyedArchiveReader).
Binary plists and NIBArchive budgets are unchanged.

Contract and boundary impact

Semantic owner and earliest changed stage: plist value depth is owned by the shared parser (src/domain/propertyListKeys.ts). The IB preflight no longer reinterprets it.
Contribution scope fit and linked maintainer decision: a bug fix within the existing static, read-only boundary; not applicable.
CLI and MCP/tool-catalog contract: schemas unchanged.
Provider, bridge, target-format, or platform compatibility: Interface Builder XML plist archives only.
Evidence, artifact, provenance, or reconstruction contract: valid archives are decoded with complete coverage. Over-limit archives keep the documented budget omission and partial coverage.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.

Evidence and regression coverage

Tests added or updated: tests/boundary/filesystem/interfaceBuilderBundle.test.ts adds "bounds XML archives by plist value depth, not element depth". One XML archive has its deepest value at exactly value depth 128 and must be decoded. A second, one level deeper, must be omitted with aggregate_decode_budget_exhausted and partial coverage (examined 2, omitted 1). It fails on main, which omits both, and passes with the fix.
Real applications, decode-interface-builder, median of 3 runs, main vs this PR:

app	main	this PR
`/System/Applications/TextEdit.app` (8 documents, 1,428 nodes, complete)	581 ms, 1,460,792 bytes, result `18d1acdefd399d53`	536 ms, identical bytes and result
`/Applications/XLD.app` (64 documents, 6,528 nodes, partial 64/109)	2,796 ms / 2,116 ms (two runs)	1,940 ms / 2,192 ms (two runs), identical 6,731,008 bytes and result `6676c9d4384bb05c`

Output is byte-identical. Timing differences are within run-to-run variance; the removed per-element depth bookkeeping is not a measurable speedup.

Remaining proof gaps: no real shipped nib nests deeply enough to cross either rule, so the boundary case is covered by the issue's fixtures and the new test.

Validation performed

npx vitest run tests/boundary/filesystem/interfaceBuilderBundle.test.ts — 45 passed, 1 skipped. Against main, the new case fails.
npx vitest run tests/boundary/filesystem/interfaceBuilderBundle.test.ts src/artifacts/apple src/domain/propertyListKeys.test.ts tests/boundary/cli/interfaceBuilder tests/acceptance/applications — 63 files, 424 passed, 1 skipped.
npm run check:fast — passed. My first test placement exceeded the 150-line describe limit; I moved the test into its own describe block.
Issue reproduction and real-app runs, as tabulated.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: macOS system and third-party app bundles.
Package or release metadata impact: none.
Security, privacy, process, or containment review: recursion stays bounded. The shared parser's iterative depth preflight runs before the recursive plist decode, as it already did for keyed archives.

Author readiness

Human 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.

🤖 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/4114 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/3949-ib-xml-plist-depth` --- ### 📝 Commits (2) - [`cf045f7`](https://github.com/morluto/rea/commit/cf045f7fb989728c2754809bd72a2573fb6f3ac1) fix(apple): bound Interface Builder XML plists by shared value depth - [`d246c40`](https://github.com/morluto/rea/commit/d246c40151c72e9fc5e967d6016c55a9c9b6feb0) fix(apple): return the decode reservation of a depth-rejected archive ### 📊 Changes **3 files changed** (+89 additions, -19 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/apple/InterfaceBuilderAnalysis.ts` (+25 -1) 📝 `src/artifacts/apple/InterfaceBuilderDecodeBudget.ts` (+11 -17) 📝 `tests/boundary/filesystem/interfaceBuilderBundle.test.ts` (+53 -1) </details> ### 📄 Description ## Summary Fixes #3949. Interface Builder decoding omitted valid XML plist archives as over budget even though keyed-archive inspection decodes the same files. Its preflight counted raw XML element depth against 128, while REA's shared plist parser bounds zero-based *value* depth at 128. The plist envelope and dictionary keys add elements but not value depth. The duplicate rule is removed, and the shared parser's limit now decides, so both commands agree. ## Problem and expected behavior The cause was in `estimateXmlPlistExpansion` (`src/artifacts/apple/InterfaceBuilderDecodeBudget.ts`). It threw `aggregate_decode_budget_exhausted` when the open-element depth exceeded 128. At the issue's depth 124, the deepest value sits at plist value depth 127 but XML element depth 129. At depth 125 those are 128 and 130. Reproduced with the issue's script through the built CLI (`decode-interface-builder`): | nesting | main | this PR | |---|---|---| | 123 | 1 document, Build control, `archive_decode` complete | unchanged | | 124 (value depth 127) | **0 documents**, `partial`, `aggregate_decode_budget_exhausted`, omitted 1 | **1 document, Build control, complete** | | 125 (value depth 128) | **0 documents**, `partial`, budget exhausted | **1 document, Build control, complete** | | 126 (value depth 129) | 0 documents, `partial`, budget exhausted | unchanged: explicit budget omission | At 126, `inspect-keyed-archive` also refuses the archive (`Plist exceeds 128 nesting levels`), so both commands now share one limit. ## Change and scope - Removed the element-depth check from the IB XML preflight, along with its now-unused `closing`/`selfClosing` token fields. The preflight still applies the representation-size estimate and the DOCTYPE internal-subset refusal. - The value-depth limit belongs to `parseXmlPropertyList`, which already bounds nesting before its recursive decode. IB's XML decode path converts that `RangeError` into the existing `InterfaceBuilderDecodeBudgetExceeded("aggregate_decode_budget_exhausted", …)`. A too-deep archive therefore stays an explicit omission with partial coverage, not an "invalid" archive. The repository already treats a plist decode `RangeError` as a resource limit (`KeyedArchiveReader`). - Binary plists and NIBArchive budgets are unchanged. ## Contract and boundary impact - Semantic owner and earliest changed stage: plist value depth is owned by the shared parser (`src/domain/propertyListKeys.ts`). The IB preflight no longer reinterprets it. - Contribution scope fit and linked maintainer decision: a bug fix within the existing static, read-only boundary; not applicable. - CLI and MCP/tool-catalog contract: schemas unchanged. - Provider, bridge, target-format, or platform compatibility: Interface Builder XML plist archives only. - Evidence, artifact, provenance, or reconstruction contract: valid archives are decoded with complete coverage. Over-limit archives keep the documented budget omission and partial coverage. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/filesystem/interfaceBuilderBundle.test.ts` adds "bounds XML archives by plist value depth, not element depth". One XML archive has its deepest value at exactly value depth 128 and must be decoded. A second, one level deeper, must be omitted with `aggregate_decode_budget_exhausted` and partial coverage (examined 2, omitted 1). It fails on main, which omits both, and passes with the fix. - Real applications, `decode-interface-builder`, median of 3 runs, main vs this PR: | app | main | this PR | |---|---|---| | `/System/Applications/TextEdit.app` (8 documents, 1,428 nodes, complete) | 581 ms, 1,460,792 bytes, result `18d1acdefd399d53` | 536 ms, identical bytes and result | | `/Applications/XLD.app` (64 documents, 6,528 nodes, partial 64/109) | 2,796 ms / 2,116 ms (two runs) | 1,940 ms / 2,192 ms (two runs), identical 6,731,008 bytes and result `6676c9d4384bb05c` | Output is byte-identical. Timing differences are within run-to-run variance; the removed per-element depth bookkeeping is not a measurable speedup. - Remaining proof gaps: no real shipped nib nests deeply enough to cross either rule, so the boundary case is covered by the issue's fixtures and the new test. ## Validation performed - `npx vitest run tests/boundary/filesystem/interfaceBuilderBundle.test.ts` — 45 passed, 1 skipped. Against main, the new case fails. - `npx vitest run tests/boundary/filesystem/interfaceBuilderBundle.test.ts src/artifacts/apple src/domain/propertyListKeys.test.ts tests/boundary/cli/interfaceBuilder tests/acceptance/applications` — 63 files, 424 passed, 1 skipped. - `npm run check:fast` — passed. My first test placement exceeded the 150-line `describe` limit; I moved the test into its own `describe` block. - Issue reproduction and real-app runs, as tabulated. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: macOS system and third-party app bundles. - Package or release metadata impact: none. - Security, privacy, process, or containment review: recursion stays bounded. The shared parser's iterative depth preflight runs before the recursive `plist` decode, as it already did for keyed archives. ## Author readiness Human 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. 🤖 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-10-31 01:11:34 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns