morluto/rea

Fork 0

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

[PR #6394] [MERGED] fix(plist): recognize OpenStep files with leading comments #7333

New issue

Closed

opened 2026-11-01 21:47:07 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:47:07 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6394
Author: @Plastic-time
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @morluto

Base: main ← Head: codex/rea-review-20261022

📝 Commits (1)

c6824e6 fix(plist): recognize OpenStep files with leading comments

📊 Changes

3 files changed (+84 additions, -7 deletions)

View changed files

➕ src/application/AppBundleExecutable.openStep.test.ts (+31 -0)
📝 src/domain/propertyListKeys.ts (+12 -7)
➕ src/domain/propertyListOpenStepComments.test.ts (+41 -0)

📄 Description

Summary

Recognize OpenStep property lists whose first container follows whitespace or comments. A valid commented Info.plist can now resolve an app bundle's executable instead of being sent to the XML decoder.

Problem and expected behavior

On base 46217c6, parseXmlPropertyList only skips whitespace before choosing its decoder. For example:

/* App metadata */
{ CFBundleExecutable = Fixture; }

The pinned OpenStep decoder accepts this input, but REA reports malformed XML and app-bundle executable resolution fails. Leading block, line, and mixed comments should behave like the uncommented equivalent.

No issue is linked: this was found and reproduced during source review. A search of existing issues and PRs found no matching leading-comment fix; #4047 addresses XML real character data instead.

Change and scope

Reuse the existing OpenStep trivia scanner for format dispatch and make it consume contiguous whitespace/comments. The original source still goes to the pinned decoder. Add production-boundary app-bundle regressions and focused parser edge cases.

Contract and boundary impact

Semantic owner and earliest changed stage: shared property-list format dispatch in src/domain/propertyListKeys.ts.
Contribution scope fit and linked maintainer decision: ordinary compatibility fix within an existing parser boundary; additional scope decision not applicable.
CLI and MCP/tool-catalog contract: no schema or command changes.
Provider, bridge, target-format, or platform compatibility: restores already-supported commented OpenStep containers, including app Info.plist files.
Evidence, artifact, provenance, or reconstruction contract: original source is preserved; prototype-key omission, depth checks, and numeric incompleteness reporting remain in place.
Target selection, authority, upstream behavior, and cleanup: existing bundle paths and executable-selection policy are unchanged; the tests use automatically cleaned temporary fixtures.
Generated metadata, package, or installation impact: none; no dependency changes.

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

Executed on Windows with Node 24.18.0 and npm 11.16.0:

Before the fix, the new bundle regression passed without a comment and failed for all three commented variants. Failure was a BinaryTargetError with an XML well-formedness error.
After the fix, all four variants resolve the expected executable and plist paths. Each input is also checked against the pinned OpenStep decoder.
Additional cases cover BOM/mixed trivia, array roots, comment-like string contents, prototype-key omission, nesting limits, invalid/rootless input, and continued XML-comment handling.
User-visible CLI/MCP output: not exercised; reproduction invokes the production bundle resolver with real temporary files.
Remaining proof gaps: no real macOS application launch or provider-backed workflow; the executable fixture is never executed.

Validation performed

npm run test:focused -- src/application/AppBundleExecutable.openStep.test.ts src/application/AppBundleExecutable.test.ts src/domain/propertyListOpenStepComments.test.ts src/domain/propertyListKeys.test.ts src/native/parsers/plist.test.ts — 100 tests passed in 5 files.
npm run verify:test-discovery — 2,083 test files assigned exactly once.
npm run check — passed, including architecture guards.
npm run check:fast — passed.
npx --no-install lint-staged — passed.
git diff --check — passed.

The full test suite and optional check:pr gate were not run.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: Windows filesystem fixtures only; no provider or real macOS launch verification.
Package or release metadata impact: none.
Security/privacy/process review: a functional parsing correction, with no demonstrated security exploit. Depth/prototype protections have regression coverage. No target execution is added.

Author readiness

Human owner: @Plastic-time

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. (CLI/MCP workflows were not exercised; production-boundary evidence is recorded above.)
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/6394 **Author:** [@Plastic-time](https://github.com/Plastic-time) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `codex/rea-review-20261022` --- ### 📝 Commits (1) - [`c6824e6`](https://github.com/morluto/rea/commit/c6824e6604e218570410baf7ccc69dc677611c70) fix(plist): recognize OpenStep files with leading comments ### 📊 Changes **3 files changed** (+84 additions, -7 deletions) <details> <summary>View changed files</summary> ➕ `src/application/AppBundleExecutable.openStep.test.ts` (+31 -0) 📝 `src/domain/propertyListKeys.ts` (+12 -7) ➕ `src/domain/propertyListOpenStepComments.test.ts` (+41 -0) </details> ### 📄 Description ## Summary Recognize OpenStep property lists whose first container follows whitespace or comments. A valid commented `Info.plist` can now resolve an app bundle's executable instead of being sent to the XML decoder. ## Problem and expected behavior On base `46217c6`, `parseXmlPropertyList` only skips whitespace before choosing its decoder. For example: ```text /* App metadata */ { CFBundleExecutable = Fixture; } ``` The pinned OpenStep decoder accepts this input, but REA reports malformed XML and app-bundle executable resolution fails. Leading block, line, and mixed comments should behave like the uncommented equivalent. No issue is linked: this was found and reproduced during source review. A search of existing issues and PRs found no matching leading-comment fix; #4047 addresses XML real character data instead. ## Change and scope Reuse the existing OpenStep trivia scanner for format dispatch and make it consume contiguous whitespace/comments. The original source still goes to the pinned decoder. Add production-boundary app-bundle regressions and focused parser edge cases. ## Contract and boundary impact - Semantic owner and earliest changed stage: shared property-list format dispatch in `src/domain/propertyListKeys.ts`. - Contribution scope fit and linked maintainer decision: ordinary compatibility fix within an existing parser boundary; additional scope decision not applicable. - CLI and MCP/tool-catalog contract: no schema or command changes. - Provider, bridge, target-format, or platform compatibility: restores already-supported commented OpenStep containers, including app `Info.plist` files. - Evidence, artifact, provenance, or reconstruction contract: original source is preserved; prototype-key omission, depth checks, and numeric incompleteness reporting remain in place. - Target selection, authority, upstream behavior, and cleanup: existing bundle paths and executable-selection policy are unchanged; the tests use automatically cleaned temporary fixtures. - Generated metadata, package, or installation impact: none; no dependency changes. 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 Executed on Windows with Node 24.18.0 and npm 11.16.0: - Before the fix, the new bundle regression passed without a comment and failed for all three commented variants. Failure was a `BinaryTargetError` with an XML well-formedness error. - After the fix, all four variants resolve the expected executable and plist paths. Each input is also checked against the pinned OpenStep decoder. - Additional cases cover BOM/mixed trivia, array roots, comment-like string contents, prototype-key omission, nesting limits, invalid/rootless input, and continued XML-comment handling. - User-visible CLI/MCP output: not exercised; reproduction invokes the production bundle resolver with real temporary files. - Remaining proof gaps: no real macOS application launch or provider-backed workflow; the executable fixture is never executed. ## Validation performed - `npm run test:focused -- src/application/AppBundleExecutable.openStep.test.ts src/application/AppBundleExecutable.test.ts src/domain/propertyListOpenStepComments.test.ts src/domain/propertyListKeys.test.ts src/native/parsers/plist.test.ts` — 100 tests passed in 5 files. - `npm run verify:test-discovery` — 2,083 test files assigned exactly once. - `npm run check` — passed, including architecture guards. - `npm run check:fast` — passed. - `npx --no-install lint-staged` — passed. - `git diff --check` — passed. The full test suite and optional `check:pr` gate were not run. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: Windows filesystem fixtures only; no provider or real macOS launch verification. - Package or release metadata impact: none. - Security/privacy/process review: a functional parsing correction, with no demonstrated security exploit. Depth/prototype protections have regression coverage. No target execution is added. ## Author readiness Human owner: @Plastic-time - [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. (CLI/MCP workflows were not exercised; production-boundary evidence is recorded above.) - [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:47:07 +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#7333

No description provided.

Rows
Columns