morluto/rea

Fork 0

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

[PR #3430] [CLOSED] fix(native): preserve whitespace in otool load-command strings #5741

New issue

Closed

opened 2026-10-31 01:09:06 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:06 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3430
Author: @bharadwajdaya
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/3384-macho-install-name-whitespace

📝 Commits (1)

0909726 fix(native): preserve whitespace in otool load-command strings

📊 Changes

2 files changed (+50 additions, -1 deletions)

View changed files

📝 src/native/parsers/otool.ts (+11 -1)
📝 tests/boundary/providers/native/nativeLoadKinds.golden.test.ts (+39 -0)

📄 Description

Summary

Fixes #3384. inspect-macho stripped surrounding whitespace from dylib install names, so the valid LC_LOAD_DYLIB name " leading.dylib " was reported as "leading.dylib" while the import source kept the spaced name. With this change the dependency path and the raw load-command name keep the recorded bytes.

Problem and expected behavior

otool prints load-command strings as <key> %s (offset %u), with exactly one separator space on each side of the recorded string. parseField trimmed the value (dropping leading spaces), and stripOffsetSuffix removed the suffix with \s+\(offset\s+\d+\) (dropping trailing spaces). Leading and trailing whitespace is part of the install name, can change loader resolution, and must survive per AGENTS.md identity/source-value preservation.

Change and scope

src/native/parsers/otool.ts: name/path fields that end in (offset N) are parsed by their exact otool format, without trimming the value. stripOffsetSuffix now removes only the single-space (offset N) suffix. This keeps normalization at the owning parser (the only producer of dependencies[].path), so no consumer changes are needed.
This also stops a name like version 2.dylib from being misread as a name version key, since the exact field match runs before the generic key regex.
LC_RPATH path gets the same treatment in raw fields, since otool prints it in the same format.
Not included: names containing CR/LF (separate line-framing issue, cf. #3390).

Contract and boundary impact

Semantic owner and earliest changed stage: otool load-command parser (parseOtoolLoadCommands).
Contribution scope fit and linked maintainer decision: focused bug fix inside existing boundaries; not applicable.
CLI and MCP/tool-catalog contract: unchanged schema; values now preserve whitespace.
Provider, bridge, target-format, or platform compatibility: macOS otool output only.
Evidence, artifact, provenance, or reconstruction contract: dependency path and raw load-command fields now match native output.
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: nativeLoadKinds.golden.test.ts — preserves dependency install-name whitespace for a spaced name and an ordinary-name control, checking dependencies[].path and the raw name/path fields.
Base reproduction: with the parser change stashed, the new spaced-name case fails (1 failed | 49 passed) and the control passes.
User-visible CLI output (issue reproduction, executed on macOS arm64 against the built runtime):

otool:  ·········name···leading.dylib···(offset·24)
exit=0
.normalized_result.load_commands.items[13].fields.name '  leading.dylib   (offset 24)'
.normalized_result.dependencies.items[0].path '  leading.dylib  '

Remaining proof gaps: did not run the full check:pr gate; Linux/Windows otool equivalents not exercised.

Validation performed

npm run test:focused -- tests/boundary/providers/native/native{LoadKinds,LoadText,Inventory,MachoHeader,TextRobustness}*.test.ts — 5 files, 103 tests passed
npm run check:fast — typecheck and lint passed
Issue reproduction via node scripts/rea.mjs inspect-macho consumer --json — exit 0, spaced install name preserved (output above)

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 with Apple otool/clang.
Package or release metadata impact: none.
Security, privacy, process, or containment review: static parsing only; no new side effects.

Author readiness

Human owner: @bharadwajdaya

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/3430 **Author:** [@bharadwajdaya](https://github.com/bharadwajdaya) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/3384-macho-install-name-whitespace` --- ### 📝 Commits (1) - [`0909726`](https://github.com/morluto/rea/commit/09097262bfeee5092be514b39057b24bee7088fc) fix(native): preserve whitespace in otool load-command strings ### 📊 Changes **2 files changed** (+50 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/native/parsers/otool.ts` (+11 -1) 📝 `tests/boundary/providers/native/nativeLoadKinds.golden.test.ts` (+39 -0) </details> ### 📄 Description ## Summary Fixes #3384. `inspect-macho` stripped surrounding whitespace from dylib install names, so the valid `LC_LOAD_DYLIB` name `" leading.dylib "` was reported as `"leading.dylib"` while the import source kept the spaced name. With this change the dependency path and the raw load-command `name` keep the recorded bytes. ## Problem and expected behavior otool prints load-command strings as `<key> %s (offset %u)`, with exactly one separator space on each side of the recorded string. `parseField` trimmed the value (dropping leading spaces), and `stripOffsetSuffix` removed the suffix with `\s+\(offset\s+\d+\)` (dropping trailing spaces). Leading and trailing whitespace is part of the install name, can change loader resolution, and must survive per AGENTS.md identity/source-value preservation. ## Change and scope - `src/native/parsers/otool.ts`: `name`/`path` fields that end in ` (offset N)` are parsed by their exact otool format, without trimming the value. `stripOffsetSuffix` now removes only the single-space ` (offset N)` suffix. This keeps normalization at the owning parser (the only producer of `dependencies[].path`), so no consumer changes are needed. - This also stops a name like `version 2.dylib` from being misread as a `name version` key, since the exact field match runs before the generic key regex. - `LC_RPATH` `path` gets the same treatment in raw fields, since otool prints it in the same format. - Not included: names containing CR/LF (separate line-framing issue, cf. #3390). ## Contract and boundary impact - Semantic owner and earliest changed stage: `otool` load-command parser (`parseOtoolLoadCommands`). - Contribution scope fit and linked maintainer decision: focused bug fix inside existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: unchanged schema; values now preserve whitespace. - Provider, bridge, target-format, or platform compatibility: macOS `otool` output only. - Evidence, artifact, provenance, or reconstruction contract: dependency path and raw load-command fields now match native output. - 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: `nativeLoadKinds.golden.test.ts` — `preserves dependency install-name whitespace` for a spaced name and an ordinary-name control, checking `dependencies[].path` and the raw `name`/`path` fields. - Base reproduction: with the parser change stashed, the new spaced-name case fails (`1 failed | 49 passed`) and the control passes. - User-visible CLI output (issue reproduction, executed on macOS arm64 against the built runtime): ```text otool: ·········name···leading.dylib···(offset·24) exit=0 .normalized_result.load_commands.items[13].fields.name ' leading.dylib (offset 24)' .normalized_result.dependencies.items[0].path ' leading.dylib ' ``` - Remaining proof gaps: did not run the full `check:pr` gate; Linux/Windows otool equivalents not exercised. ## Validation performed - `npm run test:focused -- tests/boundary/providers/native/native{LoadKinds,LoadText,Inventory,MachoHeader,TextRobustness}*.test.ts` — 5 files, 103 tests passed - `npm run check:fast` — typecheck and lint passed - Issue reproduction via `node scripts/rea.mjs inspect-macho consumer --json` — exit 0, spaced install name preserved (output above) ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 with Apple `otool`/clang. - Package or release metadata impact: none. - Security, privacy, process, or containment review: static parsing only; no new side effects. ## Author readiness Human owner: @bharadwajdaya - [ ] 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 - [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. - [ ] 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:09:06 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:10:53 +00:00

[GH-ISSUE #3582] [Tracker] Ready to close or merge: issues and pull requests (October 2026) #1330

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

No description provided.

Rows
Columns