morluto/rea

Fork 0

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

[PR #4050] [MERGED] fix(native): keep a dylib's own install name out of its dependencies #6020

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/3872-macho-id-dylib-dependency

📝 Commits (1)

8eaa3f1 fix(native): keep a dylib's own install name out of its dependencies

📊 Changes

5 files changed (+72 additions, -21 deletions)

View changed files

📝 src/native/parsers/otool.ts (+2 -1)
➕ tests/boundary/cli/nativeMachoDylibIdentityCli.test.ts (+57 -0)
📝 tests/boundary/providers/native/nativeInventory.golden.test.ts (+2 -8)
📝 tests/boundary/providers/native/nativeLoadText.golden.test.ts (+4 -6)
📝 tests/boundary/providers/native/nativeReexport.golden.test.ts (+7 -6)

📄 Description

Summary

Fixes #3872. inspect-macho reported a dylib's own LC_ID_DYLIB install name as one of its dependencies. Every dylib therefore showed a self-dependency and an inflated exhaustive dependency count. trace-dylib-resolution and REA's own MachoLoadCommandReader already treat that command as the image's identity. Now inspect-macho does too.

Problem and expected behavior

collectDependency in src/native/parsers/otool.ts added every command in DYLIB_COMMANDS to dependencies, including LC_ID_DYLIB. That set is also used to validate dylib command framing, which is correct for LC_ID_DYLIB. Its use for dependency collection is wrong, because LC_ID_DYLIB names the image itself, not a library it loads.

Reproduced on main with the issue's real dylib (xcrun clang -dynamiclib … -Wl,-install_name,@rpath/libprobe.dylib):

{"items":[{"path":"@rpath/libprobe.dylib","kind":"LC_ID_DYLIB"},{"path":"/usr/lib/libSystem.B.dylib","kind":"LC_LOAD_DYLIB"}],"total":2,"exhaustive":true}

Change and scope

collectDependency skips LC_ID_DYLIB. The command and its exact name remain in load_commands, as the issue expects.
Framing validation for LC_ID_DYLIB is unchanged.
Not included: a new normalized install_name field. It would be the natural home for the identity, but adding a required output field changes the advertised schema and breaks view parsing of previously retained inspect_macho Evidence. That deserves its own decision.

Contract and boundary impact

Semantic owner and earliest changed stage: parseOtoolLoadCommands dependency collection. Its only consumer is NativeMachoInspection (dependencies: covered(load.dependencies, true)).
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: schema unchanged. For a dylib, dependencies no longer contains the LC_ID_DYLIB row, and its total drops by one.
Provider, bridge, target-format, or platform compatibility: macOS native provider only. trace-dylib-resolution is unaffected.
Evidence, artifact, provenance, or reconstruction contract: removes a false self-dependency from exhaustive Evidence. The identity remains observable in the raw load commands.
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:
- New tests/boundary/cli/nativeMachoDylibIdentityCli.test.ts (macOS only): a public CLI workflow on a real xcrun clang -dynamiclib image. It checks that dependencies is exactly libSystem and that the LC_ID_DYLIB raw name is retained.
- Three goldens asserted the buggy self-dependency and were updated:
  - nativeLoadText.golden.test.ts: no LC_ID_DYLIB in dependencies; the raw install-name assertion is kept.
  - nativeInventory.golden.test.ts: install-name text now asserted on the raw command; dependencies are empty.
  - nativeReexport.golden.test.ts: exact two dependencies; LC_ID_DYLIB is still among the load commands.
Base reproduction: with main's otool.ts restored, the four test files fail (9 failed / 18 passed). With the fix, they pass.
User-visible CLI output, inspect-macho --json, real images, 3 runs each on macOS arm64:

target	before	after
issue probe `libprobe.dylib`	2 deps (self + libSystem), 8,976 bytes	1 dep (libSystem), 8,864 bytes
`/Applications/mpv.app/Contents/Frameworks/libMoltenVK.dylib`	12 deps (first is its own `/opt/homebrew/opt/molten-vk/lib/libMoltenVK.dylib`), 114,880 bytes, 965–994 ms	11 deps, 114,740 bytes, 972–997 ms

Elapsed time is unchanged within noise; it is dominated by the external tools. The output shrinks by the one removed row.

Remaining proof gaps: no Intel macOS run.

Validation performed

npm run test:focused -- tests/boundary/cli/nativeMachoDylibIdentityCli.test.ts tests/boundary/providers/native/nativeReexport.golden.test.ts tests/boundary/providers/native/nativeInventory.golden.test.ts tests/boundary/providers/native/nativeLoadText.golden.test.ts — 27 passed. The same four files against main's parser: 9 failed, as expected.
npx vitest run tests/boundary/providers/native tests/boundary/native tests/boundary/cli/nativeMacho tests/boundary/cli/dylib tests/boundary/mcp/analysisView — 38 files, 369 passed.
npm run check:fast — passed.
Real macOS arm64 workflow: the issue's probe dylib and an installed app's bundled dylib, run through the built CLI.

Compatibility, safety, and release

Breaking changes or migration steps: none to the schema. Consumers that read a dylib's install name from dependencies should read the LC_ID_DYLIB load command instead; trace-dylib-resolution reports it as install_name.
Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 Command Line Tools otool.
Package or release metadata impact: none.
Security, privacy, process, or containment review: static parsing only.

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/4050 **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/3872-macho-id-dylib-dependency` --- ### 📝 Commits (1) - [`8eaa3f1`](https://github.com/morluto/rea/commit/8eaa3f168041e3f9dbfd505b6b7da1b9a65b3658) fix(native): keep a dylib's own install name out of its dependencies ### 📊 Changes **5 files changed** (+72 additions, -21 deletions) <details> <summary>View changed files</summary> 📝 `src/native/parsers/otool.ts` (+2 -1) ➕ `tests/boundary/cli/nativeMachoDylibIdentityCli.test.ts` (+57 -0) 📝 `tests/boundary/providers/native/nativeInventory.golden.test.ts` (+2 -8) 📝 `tests/boundary/providers/native/nativeLoadText.golden.test.ts` (+4 -6) 📝 `tests/boundary/providers/native/nativeReexport.golden.test.ts` (+7 -6) </details> ### 📄 Description ## Summary Fixes #3872. `inspect-macho` reported a dylib's own `LC_ID_DYLIB` install name as one of its dependencies. Every dylib therefore showed a self-dependency and an inflated exhaustive dependency count. `trace-dylib-resolution` and REA's own `MachoLoadCommandReader` already treat that command as the image's identity. Now `inspect-macho` does too. ## Problem and expected behavior `collectDependency` in `src/native/parsers/otool.ts` added every command in `DYLIB_COMMANDS` to `dependencies`, including `LC_ID_DYLIB`. That set is also used to validate dylib command framing, which is correct for `LC_ID_DYLIB`. Its use for dependency collection is wrong, because `LC_ID_DYLIB` names the image itself, not a library it loads. Reproduced on main with the issue's real dylib (`xcrun clang -dynamiclib … -Wl,-install_name,@rpath/libprobe.dylib`): ```json {"items":[{"path":"@rpath/libprobe.dylib","kind":"LC_ID_DYLIB"},{"path":"/usr/lib/libSystem.B.dylib","kind":"LC_LOAD_DYLIB"}],"total":2,"exhaustive":true} ``` ## Change and scope - `collectDependency` skips `LC_ID_DYLIB`. The command and its exact `name` remain in `load_commands`, as the issue expects. - Framing validation for `LC_ID_DYLIB` is unchanged. - Not included: a new normalized `install_name` field. It would be the natural home for the identity, but adding a required output field changes the advertised schema and breaks view parsing of previously retained `inspect_macho` Evidence. That deserves its own decision. ## Contract and boundary impact - Semantic owner and earliest changed stage: `parseOtoolLoadCommands` dependency collection. Its only consumer is `NativeMachoInspection` (`dependencies: covered(load.dependencies, true)`). - 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: schema unchanged. For a dylib, `dependencies` no longer contains the `LC_ID_DYLIB` row, and its `total` drops by one. - Provider, bridge, target-format, or platform compatibility: macOS native provider only. `trace-dylib-resolution` is unaffected. - Evidence, artifact, provenance, or reconstruction contract: removes a false self-dependency from exhaustive Evidence. The identity remains observable in the raw load commands. - 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: - New `tests/boundary/cli/nativeMachoDylibIdentityCli.test.ts` (macOS only): a public CLI workflow on a real `xcrun clang -dynamiclib` image. It checks that `dependencies` is exactly libSystem and that the `LC_ID_DYLIB` raw `name` is retained. - Three goldens asserted the buggy self-dependency and were updated: - `nativeLoadText.golden.test.ts`: no `LC_ID_DYLIB` in dependencies; the raw install-name assertion is kept. - `nativeInventory.golden.test.ts`: install-name text now asserted on the raw command; dependencies are empty. - `nativeReexport.golden.test.ts`: exact two dependencies; `LC_ID_DYLIB` is still among the load commands. - Base reproduction: with main's `otool.ts` restored, the four test files fail (9 failed / 18 passed). With the fix, they pass. - User-visible CLI output, `inspect-macho --json`, real images, 3 runs each on macOS arm64: | target | before | after | |---|---|---| | issue probe `libprobe.dylib` | 2 deps (self + libSystem), 8,976 bytes | 1 dep (libSystem), 8,864 bytes | | `/Applications/mpv.app/Contents/Frameworks/libMoltenVK.dylib` | 12 deps (first is its own `/opt/homebrew/opt/molten-vk/lib/libMoltenVK.dylib`), 114,880 bytes, 965–994 ms | 11 deps, 114,740 bytes, 972–997 ms | Elapsed time is unchanged within noise; it is dominated by the external tools. The output shrinks by the one removed row. - Remaining proof gaps: no Intel macOS run. ## Validation performed - `npm run test:focused -- tests/boundary/cli/nativeMachoDylibIdentityCli.test.ts tests/boundary/providers/native/nativeReexport.golden.test.ts tests/boundary/providers/native/nativeInventory.golden.test.ts tests/boundary/providers/native/nativeLoadText.golden.test.ts` — 27 passed. The same four files against main's parser: 9 failed, as expected. - `npx vitest run tests/boundary/providers/native tests/boundary/native tests/boundary/cli/nativeMacho tests/boundary/cli/dylib tests/boundary/mcp/analysisView` — 38 files, 369 passed. - `npm run check:fast` — passed. - Real macOS arm64 workflow: the issue's probe dylib and an installed app's bundled dylib, run through the built CLI. ## Compatibility, safety, and release - Breaking changes or migration steps: none to the schema. Consumers that read a dylib's install name from `dependencies` should read the `LC_ID_DYLIB` load command instead; `trace-dylib-resolution` reports it as `install_name`. - Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 Command Line Tools `otool`. - Package or release metadata impact: none. - Security, privacy, process, or containment review: static parsing only. ## 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:12 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #6020] [MERGED] fix(process): keep structural capture issues alongside semantic ones #7125

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

No description provided.

Rows
Columns