morluto/rea

Fork 0

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

[PR #4048] [MERGED] fix(native): retain every repeated otool load-command field #6016

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/4048
Author: @kaoru0822-kitauji
Created: 10/15/2026
Status: ✅ Merged
Merged: 10/15/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/4008-macho-linker-options

📝 Commits (1)

f6c07f3 fix(native): retain every repeated otool load-command field

📊 Changes

4 files changed (+175 additions, -12 deletions)

View changed files

📝 docs/native-investigation.md (+3 -1)
📝 src/native/parsers/otool.ts (+43 -10)
➕ tests/boundary/cli/nativeMachoLinkerOptionsCli.test.ts (+68 -0)
📝 tests/boundary/providers/native/nativeLoadKinds.golden.test.ts (+61 -1)

📄 Description

Summary

Fixes #4008. inspect-macho lost every LC_LINKER_OPTION string except the last one. The surviving value kept otool's #n prefix, and the collection still reported exhaustive: true with no limitations. Each string is now retained exactly, in order, and checked against the declared count. The same root cause also dropped all but the last tool from a multi-tool LC_BUILD_VERSION record, and that is fixed too.

Problem and expected behavior

parseFields in src/native/parsers/otool.ts turned every otool -l row into one entry of a flat key → value record, so a key printed twice overwrote the earlier value. Reproduced on main with the issue's command and a real clang object:

{"cmd":"LC_LINKER_OPTION","cmdsize":40,"count":2,"string":"#2 Foundation"}

otool -l prints string #1 -framework and string #2 Foundation. A three-tool LC_BUILD_VERSION (built with vtool) kept only tool: 2, version: "6.0" in fields. Its build_metadata was already complete, because that path re-reads the block.

Change and scope

At the producing boundary (parseFields / parseField):
- Linker-option rows keep otool's own ordinal label as the key (string #1, string #2, …), at every count. The value is the exact option text, including surrounding spaces, and is never coerced to a number.
- Any other key printed more than once is labeled key #n in print order. Keys printed once are unchanged, so existing single-tool and other records keep their shape.
New validateLinkerOptionLines: count must be unique, and the remaining rows must be exactly string #1..count, in order. A mismatch, a repeated or out-of-order ordinal, or an unlabeled row throws, the same way the existing build-tool, section-count and dylib framing checks do.
validateDylibCommandLines now checks encoding-field presence on the parsed rows rather than the labeled record. Repeated options rows are still rejected with the existing reason.
docs/native-investigation.md documents the name #n and string #n keys.
No schema change: fields remains record<string, string | number | null>.

Contract and boundary impact

Semantic owner and earliest changed stage: src/native/parsers/otool.ts (parseOtoolLoadCommands), which owns the parsing of otool load-command text. No downstream code reads fields keys.
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: output schema unchanged. load_commands.items[].fields now keeps every repeated row. A single string key on LC_LINKER_OPTION becomes string #n.
Provider, bridge, target-format, or platform compatibility: macOS native provider (otool) only.
Evidence, artifact, provenance, or reconstruction contract: removes silent loss from exhaustive Evidence. Malformed linker-option framing now fails closed instead of returning a lossy record. On such input llvm-nm already fails the inspection.
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/cli/nativeMachoLinkerOptionsCli.test.ts (macOS only): a public CLI workflow on a real xcrun clang -c object with two linker options.
- tests/boundary/providers/native/nativeLoadKinds.golden.test.ts:
  - exact, ordered strings, including surrounding spaces and a numeric-looking option;
  - rejection of fewer or more strings than count, out-of-order or repeated ordinals, and an unlabeled row;
  - full fields of the existing two-tool LC_BUILD_VERSION case, plus the unchanged single-tool shape.
Base reproduction: with main's otool.ts restored, these tests fail (8 failed / 47 passed across the two files). With the fix, they pass.
User-visible CLI output, real object from the issue plus a three-string option, inspect-macho --json:

	before	after
`LC_LINKER_OPTION` (count 2)	`"string":"#2 Foundation"`	`"string #1":"-framework","string #2":"Foundation"`
`LC_LINKER_OPTION` (count 3)	`"string":"#3 -ObjC"`	`"string #1":"-weak_framework","string #2":"Security","string #3":"-ObjC"`
`LC_BUILD_VERSION` (3 tools, `vtool`)	`"tool":2,"version":"6.0"`	`"tool #1":3,"version #1":"1100.1",…,"tool #3":2,"version #3":"6.0"`
Evidence size (`options.o`)	7,093 bytes	7,171 bytes (+78, the previously lost strings)

Performance: parseOtoolLoadCommands on /opt/homebrew/bin/node's otool -h -l output (45 commands, 364 lines, no repeated keys) has a median of 0.234 ms on base and 0.294 ms with the fix, over 200 runs after warm-up. The serialized commands are byte-identical (9,542 bytes). The CLI inspection takes ~1.1 s either way, dominated by the external tools.
Remaining proof gaps: no CI coverage on Intel macOS. LC_LINKER_OPTION strings containing a newline (which otool prints ambiguously) now fail closed rather than being guessed. That behavior is exercised only through the unlabeled-row test.

Validation performed

npm run test:focused -- <8 native Mach-O parser/CLI test files> — 126 passed.
npx vitest run tests/boundary/providers/native tests/boundary/native tests/boundary/cli/nativeMacho tests/boundary/cli/dylib tests/boundary/mcp/analysisView — 38 files, 375 passed.
The same two new or extended test files against main's parser — 8 failed, as expected.
npm run check:fast, npx oxfmt --check on the changed files, npm run docs:check — passed.
Real macOS arm64 workflow: Apple clang .linker_option object, a vtool three-tool executable, and /opt/homebrew/bin/node, run through the built CLI (node scripts/rea.mjs inspect-macho … --json).

Compatibility, safety, and release

Breaking changes or migration steps: none to the advertised schema. fields is an untyped raw-row record. LC_LINKER_OPTION records now carry string #1..count instead of one string key. That key only ever held the last option, with otool's #n prefix, so it never carried a complete value. Records whose keys appear once are unchanged. Maintainers may prefer a ! marker; I can add one on request.
Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 with the Command Line Tools otool and llvm-nm.
Package or release metadata impact: none.
Security, privacy, process, or containment review: static parsing only. No new processes or file effects.

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/4048 **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/4008-macho-linker-options` --- ### 📝 Commits (1) - [`f6c07f3`](https://github.com/morluto/rea/commit/f6c07f311bba6718e456312316ab763267f497ae) fix(native): retain every repeated otool load-command field ### 📊 Changes **4 files changed** (+175 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `docs/native-investigation.md` (+3 -1) 📝 `src/native/parsers/otool.ts` (+43 -10) ➕ `tests/boundary/cli/nativeMachoLinkerOptionsCli.test.ts` (+68 -0) 📝 `tests/boundary/providers/native/nativeLoadKinds.golden.test.ts` (+61 -1) </details> ### 📄 Description ## Summary Fixes #4008. `inspect-macho` lost every `LC_LINKER_OPTION` string except the last one. The surviving value kept otool's `#n` prefix, and the collection still reported `exhaustive: true` with no limitations. Each string is now retained exactly, in order, and checked against the declared `count`. The same root cause also dropped all but the last tool from a multi-tool `LC_BUILD_VERSION` record, and that is fixed too. ## Problem and expected behavior `parseFields` in `src/native/parsers/otool.ts` turned every `otool -l` row into one entry of a flat `key → value` record, so a key printed twice overwrote the earlier value. Reproduced on main with the issue's command and a real clang object: ```json {"cmd":"LC_LINKER_OPTION","cmdsize":40,"count":2,"string":"#2 Foundation"} ``` `otool -l` prints `string #1 -framework` and `string #2 Foundation`. A three-tool `LC_BUILD_VERSION` (built with `vtool`) kept only `tool: 2, version: "6.0"` in `fields`. Its `build_metadata` was already complete, because that path re-reads the block. ## Change and scope - At the producing boundary (`parseFields` / `parseField`): - Linker-option rows keep otool's own ordinal label as the key (`string #1`, `string #2`, …), at every count. The value is the exact option text, including surrounding spaces, and is never coerced to a number. - Any other key printed more than once is labeled `key #n` in print order. Keys printed once are unchanged, so existing single-tool and other records keep their shape. - New `validateLinkerOptionLines`: `count` must be unique, and the remaining rows must be exactly `string #1..count`, in order. A mismatch, a repeated or out-of-order ordinal, or an unlabeled row throws, the same way the existing build-tool, section-count and dylib framing checks do. - `validateDylibCommandLines` now checks encoding-field presence on the parsed rows rather than the labeled record. Repeated `options` rows are still rejected with the existing reason. - `docs/native-investigation.md` documents the `name #n` and `string #n` keys. - No schema change: `fields` remains `record<string, string | number | null>`. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/native/parsers/otool.ts` (`parseOtoolLoadCommands`), which owns the parsing of otool load-command text. No downstream code reads `fields` keys. - 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: output schema unchanged. `load_commands.items[].fields` now keeps every repeated row. A single `string` key on `LC_LINKER_OPTION` becomes `string #n`. - Provider, bridge, target-format, or platform compatibility: macOS native provider (`otool`) only. - Evidence, artifact, provenance, or reconstruction contract: removes silent loss from exhaustive Evidence. Malformed linker-option framing now fails closed instead of returning a lossy record. On such input `llvm-nm` already fails the inspection. - 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/cli/nativeMachoLinkerOptionsCli.test.ts` (macOS only): a public CLI workflow on a real `xcrun clang -c` object with two linker options. - `tests/boundary/providers/native/nativeLoadKinds.golden.test.ts`: - exact, ordered strings, including surrounding spaces and a numeric-looking option; - rejection of fewer or more strings than `count`, out-of-order or repeated ordinals, and an unlabeled row; - full `fields` of the existing two-tool `LC_BUILD_VERSION` case, plus the unchanged single-tool shape. - Base reproduction: with main's `otool.ts` restored, these tests fail (8 failed / 47 passed across the two files). With the fix, they pass. - User-visible CLI output, real object from the issue plus a three-string option, `inspect-macho --json`: | | before | after | |---|---|---| | `LC_LINKER_OPTION` (count 2) | `"string":"#2 Foundation"` | `"string #1":"-framework","string #2":"Foundation"` | | `LC_LINKER_OPTION` (count 3) | `"string":"#3 -ObjC"` | `"string #1":"-weak_framework","string #2":"Security","string #3":"-ObjC"` | | `LC_BUILD_VERSION` (3 tools, `vtool`) | `"tool":2,"version":"6.0"` | `"tool #1":3,"version #1":"1100.1",…,"tool #3":2,"version #3":"6.0"` | | Evidence size (`options.o`) | 7,093 bytes | 7,171 bytes (+78, the previously lost strings) | - Performance: `parseOtoolLoadCommands` on `/opt/homebrew/bin/node`'s `otool -h -l` output (45 commands, 364 lines, no repeated keys) has a median of 0.234 ms on base and 0.294 ms with the fix, over 200 runs after warm-up. The serialized commands are byte-identical (9,542 bytes). The CLI inspection takes ~1.1 s either way, dominated by the external tools. - Remaining proof gaps: no CI coverage on Intel macOS. `LC_LINKER_OPTION` strings containing a newline (which otool prints ambiguously) now fail closed rather than being guessed. That behavior is exercised only through the unlabeled-row test. ## Validation performed - `npm run test:focused -- <8 native Mach-O parser/CLI test files>` — 126 passed. - `npx vitest run tests/boundary/providers/native tests/boundary/native tests/boundary/cli/nativeMacho tests/boundary/cli/dylib tests/boundary/mcp/analysisView` — 38 files, 375 passed. - The same two new or extended test files against main's parser — 8 failed, as expected. - `npm run check:fast`, `npx oxfmt --check` on the changed files, `npm run docs:check` — passed. - Real macOS arm64 workflow: Apple clang `.linker_option` object, a `vtool` three-tool executable, and `/opt/homebrew/bin/node`, run through the built CLI (`node scripts/rea.mjs inspect-macho … --json`). ## Compatibility, safety, and release - Breaking changes or migration steps: none to the advertised schema. `fields` is an untyped raw-row record. `LC_LINKER_OPTION` records now carry `string #1..count` instead of one `string` key. That key only ever held the last option, with otool's `#n` prefix, so it never carried a complete value. Records whose keys appear once are unchanged. Maintainers may prefer a `!` marker; I can add one on request. - Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 with the Command Line Tools otool and llvm-nm. - Package or release metadata impact: none. - Security, privacy, process, or containment review: static parsing only. No new processes or file effects. ## 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:33 +00:00

[PR #6016] [MERGED] fix(browser)!: keep malformed console argument inventories unknown #7121

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

No description provided.

Rows
Columns