[PR #4965] [CLOSED] fix(plist): ignore processing instructions between container members #6698

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4965
Author: @Mangolycheematcha
Created: 10/18/2026
Status: ❌ Closed

Base: main ← Head: codex/plist-processing-instructions


📝 Commits (2)

  • cbea8e0 fix(plist): ignore processing instructions between container members
  • d34d704 test(plist): cover document and scalar instruction boundaries

📊 Changes

3 files changed (+97 additions, -1 deletions)

View changed files

📝 src/application/AppBundleExecutable.test.ts (+27 -0)
📝 src/domain/propertyListXml.test.ts (+68 -0)
📝 src/domain/propertyListXml.ts (+2 -1)

📄 Description

Summary

Ignore XML processing instructions between plist/container members. Previously a legal <?rea ignored?> could reach value decoding, fail, and prevent an app's Info.plist from resolving its executable.

Fixes #4964.

Problem and expected behavior

<plist><?rea ignored?><string>demo</string></plist> should decode as "demo", following Apple's CFPropertyList member reader. The shared decoder instead interpreted the instruction's target as a plist tag.

Change and scope

Exclude PROCESSING_INSTRUCTION_NODE at the existing members() boundary. Add container regressions and filesystem-backed app-resolution checks for macOS-style and flat iOS-style directory layouts on Windows. Scalar content rules and XML well-formedness checks remain unchanged. This is a routine compatibility correction within existing static reads; no separate maintainer scope decision is required.

Contract and boundary impact

  • Semantic owner: shared XML plist container-member decoding.

  • Contribution scope: existing local static analysis; no new provider or execution authority.

  • CLI/MCP schemas, generated catalogs, package metadata, and installation: unchanged.

  • Evidence: valid instruction nodes no longer masquerade as data or prevent decoding; source bytes and provenance handling are unchanged.

  • Target effects: existing file reads only; no target execution, mutation, or new persistence.

  • 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

  • Before the correction: 5 new failures and 35 passing controls on source 6b180783ed04ebc87548c8385c80e05725804c47.

  • After the correction: 107 tests passed across the shared XML decoder, key handling, app resolution, and native plist result parser.

  • The regressions cover plist/array/dict positions, instructions named like plist tags, dictionary key/value boundaries, malformed instructions, and real unsupported elements.

  • Default-filesystem app resolution succeeds for both authored bundle layouts. Fixture program files are inert text and were never executed.

  • Python 3.12.14 plistlib independently accepts the matching valid XML fixtures. Apple's source is a reference comparison, not an executed Apple runtime.

  • Observed, derived, and inferred claims remain distinguishable.

  • Artifact identity, source provenance, and failed attempts remain preserved.

  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Review follow-up

In response to @XinwangTao's boundary-coverage suggestion, d34d704 adds four cases covering an XML declaration with ordinary processing instructions before/after the root, literal instruction text in CDATA/escaped content, and existing incomplete integer handling with PI/comment markup. These protect existing boundary behavior; no further production change was needed. Strict malformed-XML rejection is unchanged.

Validation performed

Windows x64, Node 24.18.0, npm 11.16.0.

  • npm run test:local -- src/domain/propertyListXml.test.ts src/application/AppBundleExecutable.test.ts src/domain/propertyListKeys.test.ts src/native/parsers/plist.test.ts — 107 passed.
  • npm run verify:test-discovery — all 1,578 test files discovered exactly once.
  • npm run check:fast — typecheck and lint passed.
  • npm run format:check — all 4,017 matched files passed.
  • npm run metadata:check — passed.
  • npm run verify:architecture-guards — passed across 1,506 source files.
  • git diff --check — passed.
  • npm run check — could not start the Windows artifact-lock wrapper (missing matching native addon).
  • npm run knip — one unresolved existing package import, #javascript-analysis-worker, whose target is the unbuilt dist/composition/javascriptAnalysisWorkerProcess.js. No imports in the production change were added or modified.

Compatibility, safety, and release

No new dependencies, migrations, tool schemas, or target effects. Actual macOS/iOS execution and plutil were not tested. Full npm run check could not start its artifact-lock wrapper because the source checkout lacks the matching 6.6.0 Windows native addon; that exact npm version was unavailable when checked. Source-level checks are recorded separately. No performance improvement or production prevalence is claimed.

Author readiness

Human owner: @Mangolycheematcha

  • 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/4965 **Author:** [@Mangolycheematcha](https://github.com/Mangolycheematcha) **Created:** 10/18/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `codex/plist-processing-instructions` --- ### 📝 Commits (2) - [`cbea8e0`](https://github.com/morluto/rea/commit/cbea8e0779bf2f8dd0fefbbfadb582384ef53c39) fix(plist): ignore processing instructions between container members - [`d34d704`](https://github.com/morluto/rea/commit/d34d704e11b29870e1412b68c9eb8584fc54c56f) test(plist): cover document and scalar instruction boundaries ### 📊 Changes **3 files changed** (+97 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/application/AppBundleExecutable.test.ts` (+27 -0) 📝 `src/domain/propertyListXml.test.ts` (+68 -0) 📝 `src/domain/propertyListXml.ts` (+2 -1) </details> ### 📄 Description ## Summary Ignore XML processing instructions between plist/container members. Previously a legal `<?rea ignored?>` could reach value decoding, fail, and prevent an app's `Info.plist` from resolving its executable. Fixes #4964. ## Problem and expected behavior `<plist><?rea ignored?><string>demo</string></plist>` should decode as `"demo"`, following Apple's [CFPropertyList member reader](https://github.com/apple-oss-distributions/CF/blob/5b29c619aaf0cf1042bcfefc4bb10ef15b89dc11/CFPropertyList.c#L911). The shared decoder instead interpreted the instruction's target as a plist tag. ## Change and scope Exclude `PROCESSING_INSTRUCTION_NODE` at the existing `members()` boundary. Add container regressions and filesystem-backed app-resolution checks for macOS-style and flat iOS-style directory layouts on Windows. Scalar content rules and XML well-formedness checks remain unchanged. This is a routine compatibility correction within existing static reads; no separate maintainer scope decision is required. ## Contract and boundary impact - Semantic owner: shared XML plist container-member decoding. - Contribution scope: existing local static analysis; no new provider or execution authority. - CLI/MCP schemas, generated catalogs, package metadata, and installation: unchanged. - Evidence: valid instruction nodes no longer masquerade as data or prevent decoding; source bytes and provenance handling are unchanged. - Target effects: existing file reads only; no target execution, mutation, or new persistence. - [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 - Before the correction: 5 new failures and 35 passing controls on source `6b180783ed04ebc87548c8385c80e05725804c47`. - After the correction: 107 tests passed across the shared XML decoder, key handling, app resolution, and native plist result parser. - The regressions cover plist/array/dict positions, instructions named like plist tags, dictionary key/value boundaries, malformed instructions, and real unsupported elements. - Default-filesystem app resolution succeeds for both authored bundle layouts. Fixture program files are inert text and were never executed. - Python 3.12.14 `plistlib` independently accepts the matching valid XML fixtures. Apple's source is a reference comparison, not an executed Apple runtime. - [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. ### Review follow-up In response to [@XinwangTao's boundary-coverage suggestion](https://github.com/morluto/rea/issues/4964#issuecomment-6247537812), [d34d704](https://github.com/Mangolycheematcha/rea/commit/d34d704e11b29870e1412b68c9eb8584fc54c56f) adds four cases covering an XML declaration with ordinary processing instructions before/after the root, literal instruction text in CDATA/escaped content, and existing incomplete integer handling with PI/comment markup. These protect existing boundary behavior; no further production change was needed. Strict malformed-XML rejection is unchanged. ## Validation performed Windows x64, Node 24.18.0, npm 11.16.0. - `npm run test:local -- src/domain/propertyListXml.test.ts src/application/AppBundleExecutable.test.ts src/domain/propertyListKeys.test.ts src/native/parsers/plist.test.ts` — 107 passed. - `npm run verify:test-discovery` — all 1,578 test files discovered exactly once. - `npm run check:fast` — typecheck and lint passed. - `npm run format:check` — all 4,017 matched files passed. - `npm run metadata:check` — passed. - `npm run verify:architecture-guards` — passed across 1,506 source files. - `git diff --check` — passed. - `npm run check` — could not start the Windows artifact-lock wrapper (missing matching native addon). - `npm run knip` — one unresolved existing package import, `#javascript-analysis-worker`, whose target is the unbuilt `dist/composition/javascriptAnalysisWorkerProcess.js`. No imports in the production change were added or modified. ## Compatibility, safety, and release No new dependencies, migrations, tool schemas, or target effects. Actual macOS/iOS execution and `plutil` were not tested. Full `npm run check` could not start its artifact-lock wrapper because the source checkout lacks the matching 6.6.0 Windows native addon; that exact npm version was unavailable when checked. Source-level checks are recorded separately. No performance improvement or production prevalence is claimed. ## Author readiness Human owner: @Mangolycheematcha - [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. - [ ] 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:42:23 +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#6698
No description provided.