[PR #4067] [MERGED] fix(paths): select caller paths through symlinks and .. as the filesystem does #6032

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/3864-symlink-parent-paths

📝 Commits (3)

c92a375 fix: select caller paths through symlinks and .. as the filesystem does
a384b89 fix: keep firmware, WASM list and output-root paths on the selected file
1653bb5 fix: keep directory requirements and relative cwd in caller paths

📊 Changes

25 files changed (+352 additions, -54 deletions)

View changed files

📝 src/application/BinaryTargetResolver.ts (+3 -2)
📝 src/application/reverse/ReverseEngineeringService.ts (+3 -2)
📝 src/artifacts/SafeOutputTree.ts (+4 -2)
📝 src/cli/binaryDiagnosticsCommands.ts (+4 -4)
📝 src/cli/captureCommands.ts (+2 -2)
📝 src/cli/coreBinaryCommands.ts (+2 -2)
📝 src/cli/evmCommands.ts (+5 -2)
📝 src/cli/firmwareCommands.ts (+4 -4)
📝 src/cli/goCommands.ts (+6 -2)
📝 src/cli/rustCommands.ts (+2 -2)
📝 src/cli/signalCommands.ts (+2 -2)
📝 src/cli/sqliteCommands.ts (+2 -2)
📝 src/cli/wasmCommands.ts (+6 -4)
➕ src/filesystem/callerPath.ts (+32 -0)
📝 src/firmware/FirmwareFiles.ts (+3 -2)
📝 src/objdump/ObjdumpCommand.ts (+5 -3)
📝 src/rizin/RizinCommand.ts (+2 -3)
📝 src/rizin/RizinDebugSessionManager.ts (+2 -2)
📝 src/wasm/WabtOutput.test.ts (+20 -0)
📝 src/wasm/WasmGlueAdapter.ts (+13 -6)

...and 5 more files

📄 Description

Summary

Fixes #3864, fixes #3911, fixes #3912. On POSIX, a caller path such as link/../sample, where link is a directory symlink, makes the filesystem follow link first and then apply ... REA made caller paths absolute with path.resolve, which collapses link/.. lexically. As a result, several commands inspected, hashed or launched a sibling file and returned internally consistent, successful Evidence about the wrong artifact. This PR moves that step into one owner that never changes which file a path names.

Problem and expected behavior

I reproduced this on main with the built CLI and a real Mach-O (gh):

target/sample is a copy of gh.
The sibling ./sample is the same copy with one byte appended.
link -> target/child.

command	`link/../sample` (relative)	`$PWD/link/../sample` (absolute)
`inspect-go-binary`	sibling `2ba6b7…` ✗	sibling ✗
`inspect-rust-binary`	sibling ✗	sibling ✗
`inspect-with-objdump … file_headers`	sibling ✗	sibling ✗
`inspect-macho`, `list-architectures`, `inspect-signature`	sibling ✗	intended ✓

The intended file is db2578…. Every command exited 0 in every case.

There are two lexical producers:

Relative paths: CLI adapters (resolve(args.path)) and parseBinaryTarget (resolve(cwd, input)) build the absolute path lexically.
Absolute paths: they are re-resolved lexically in the Go and Rust CLI adapters, the objdump and Rizin command builders, RizinDebugSessionManager launch, and ReverseEngineeringService, which hashes objdump and Rizin inputs. Because of the builders and the service, the objdump and Rizin bug also reaches MCP callers.

Change and scope

New src/filesystem/callerPath.ts exports absoluteCallerPath(path, { cwd?, platform? }):
- POSIX: a relative path is prefixed with the working directory (a relative cwd is made absolute the same way). Only interior . segments and repeated separators are removed, which never change the named file. A trailing / or /. keeps its directory requirement as one trailing /. Every .. is kept, so the filesystem resolves symlinks and .. in order. ./module.wasm from /app is /app/module.wasm; link/../x stays /app/link/../x.
- Win32: keeps win32.resolve, because Windows resolves .. lexically, so that normalization is exact there.
All of the producers above now call it, and their now-unused resolve/isAbsolute imports are removed:
- 13 CLI sites in the binaryDiagnostics, capture, coreBinary, evm, firmware (input and output directory), go, rust, signal, sqlite and wasm command modules;
- parseBinaryTarget, ReverseEngineeringService, objdumpCommand, rizinCommand and the Rizin debug launch;
- the WASM --glue_paths/--candidate_paths lists, snapshotFirmware (firmware input) and SafeOutputTree.create (extraction output root). These were review follow-ups: the CLI conversion alone did not reach these owners. SafeOutputTree uses host path syntax; its platform argument selects only the name policy.
Evidence local_path now keeps the caller's spelling (for example …/link/../sample) when the provider reports it. It no longer reports a different file's path.
Remaining cleanup, not changed here because these sites are outside the reported binary-inspection paths and each needs its own verification:
- JsonFiles, EvidenceBundleFiles and JavaScriptRecoveryService still call resolve(path) on caller-selected JSON, bundle and source paths.

Contract and boundary impact

Semantic owner and earliest changed stage: absoluteCallerPath owns conversion of a caller-selected local path to an absolute path. The CLI adapters, target resolution, provider command builders and the hashing service consume it.
Contribution scope fit and linked maintainer decision: a bug fix within the existing boundaries; not applicable.
CLI and MCP/tool-catalog contract: schemas unchanged. The same request now selects the file the filesystem names.
Provider, bridge, target-format, or platform compatibility:
- POSIX: behavior changes only for paths with a parent component (..) after a symlink, or with ./duplicate separators, which are now passed through unnormalized.
- Windows: unchanged.
Evidence, artifact, provenance, or reconstruction contract:
- Subject digest, artifact facts and local_path now describe the selected file.
- Rizin debug launch_provenance.resolved_path now records the absolute path actually launched, which names the same file as caller_selected_path.
Target selection, authority, upstream behavior, and cleanup details: the Rizin debug launch no longer starts a sibling program for such a path. There is no other change to execution authority.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.
Launch or attach to a process. (Only the existing Rizin debug launch path; this PR changes which path it receives.)

Evidence and regression coverage

Tests added or updated:
- New tests/boundary/cli/callerPathSymlinkParentCli.test.ts (POSIX): public CLI runs of inspect-go-binary, inspect-rust-binary and inspect-pe-resources with relative and absolute link/../… spellings, asserting the selected file's SHA-256. It uses the repository's Go and PE resource fixture writers.
- New tests/boundary/filesystem/callerPath.test.ts: POSIX and win32 helper semantics, including which spellings are normalized.
- New tests/boundary/filesystem/symlinkParentOwners.test.ts (POSIX): snapshotFirmware digests target/image.bin for link/../image.bin, and SafeOutputTree.create("…/link/../out") creates target/out and not ./out. Both fail on the previous commit.
- src/wasm/WabtOutput.test.ts: a CLI-selected ./module.wasm keeps an exact local-path-candidate glue match against a competing /other/module.wasm. This fails with the first version of the helper.
- RizinDebugSessionReviewRegressions.test.ts (3 cases) and rizinDebuggerCancellationMcp.test.ts derived their expected resolved_path with lexical resolve. They now derive it from the owner.
Base reproduction: with main's src restored, the new CLI test fails in both cases. Go selects the sibling, and PE resources selects exactly the sibling digest predicted in #3911 (9a251f9f…). With the fix, both pass.
User-visible CLI output after the fix, same layout, real gh copy: every command in the table above selects db2578… for both spellings. execute-rizin-command is unavailable on this host (no Rizin), and gh is not a PE file.
Timing, macOS arm64, median of 5 runs, built CLI:

run	`inspect-go-binary link/../sample`	`inspect-go-binary target/sample`	`inspect-macho link/../sample`	`inspect-macho target/sample`
main	414 ms, 62,695 bytes, wrong file	406 ms, 62,723 bytes	1,054 ms, 25,940 bytes, wrong file	1,118 ms, 25,947 bytes
fix (run 1)	496 ms, 62,727 bytes, correct	473 ms, 62,723 bytes	946 ms, 25,947 bytes, correct	930 ms, 25,947 bytes
fix (run 2)	385 ms	387 ms	997 ms	933 ms

The helper does a string check and a concatenation. The spread between the two fixed runs (385–496 ms) is host noise; this is not a performance change. Output for the link/../ spelling grows by 4 bytes, because local_path keeps the caller's spelling instead of the collapsed sibling path.

Remaining proof gaps:
- Rizin and GNU objdump command lines were verified through tests and the objdump CLI run, not with a real Rizin install.
- The Windows behavior is covered only by the helper's win32 unit case.
- The sites listed under remaining cleanup are unverified.

Validation performed

npm run test:focused -- tests/boundary/filesystem/callerPath.test.ts tests/boundary/cli/callerPathSymlinkParentCli.test.ts — 4 passed. The CLI test against main's src: 2 failed, as expected.
npx vitest run tests/boundary/cli tests/boundary/providers tests/boundary/filesystem tests/boundary/native src/application tests/acceptance/goBinaryCli.test.ts tests/acceptance/rustBinaryCli.test.ts tests/acceptance/applications/peResources.test.ts — 465 files: 5,294 passed, 3 failed. All three were the Rizin expectations described above; after updating them the file passes (15 passed).
npx vitest run tests/boundary/mcp tests/acceptance — 231 files: 1,249 passed, 1 failed. It was the Rizin MCP expectation described above; after updating it the file passes (3 passed).
After the review follow-up: npx vitest run tests/boundary/cli tests/boundary/providers tests/boundary/filesystem tests/boundary/native tests/boundary/mcp tests/boundary/firmware src/application src/wasm tests/acceptance — 700 files, 6,562 passed, 93 skipped, 0 failed. Linux-only firmware integration cases were skipped on this host.
npm run check:fast — passed. npm run knip — clean.

Compatibility, safety, and release

Breaking changes or migration steps: none. Requests that previously selected the wrong file now select the one the caller named.
Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 with real native tools (otool, lipo, codesign, objdump). No Linux or Windows host runs.
Package or release metadata impact: none.
Security, privacy, process, or containment review: this closes a path where evidence and a debugger launch could be bound to a file other than the one selected. No new process or network 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/4067 **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/3864-symlink-parent-paths` --- ### 📝 Commits (3) - [`c92a375`](https://github.com/morluto/rea/commit/c92a375dc53b5c8d9cd8910e85e68e15466ad1ca) fix: select caller paths through symlinks and .. as the filesystem does - [`a384b89`](https://github.com/morluto/rea/commit/a384b892b0d7957656394c2896830724eac318c3) fix: keep firmware, WASM list and output-root paths on the selected file - [`1653bb5`](https://github.com/morluto/rea/commit/1653bb58f32b1087f9634613f242c7b87ba4d63e) fix: keep directory requirements and relative cwd in caller paths ### 📊 Changes **25 files changed** (+352 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `src/application/BinaryTargetResolver.ts` (+3 -2) 📝 `src/application/reverse/ReverseEngineeringService.ts` (+3 -2) 📝 `src/artifacts/SafeOutputTree.ts` (+4 -2) 📝 `src/cli/binaryDiagnosticsCommands.ts` (+4 -4) 📝 `src/cli/captureCommands.ts` (+2 -2) 📝 `src/cli/coreBinaryCommands.ts` (+2 -2) 📝 `src/cli/evmCommands.ts` (+5 -2) 📝 `src/cli/firmwareCommands.ts` (+4 -4) 📝 `src/cli/goCommands.ts` (+6 -2) 📝 `src/cli/rustCommands.ts` (+2 -2) 📝 `src/cli/signalCommands.ts` (+2 -2) 📝 `src/cli/sqliteCommands.ts` (+2 -2) 📝 `src/cli/wasmCommands.ts` (+6 -4) ➕ `src/filesystem/callerPath.ts` (+32 -0) 📝 `src/firmware/FirmwareFiles.ts` (+3 -2) 📝 `src/objdump/ObjdumpCommand.ts` (+5 -3) 📝 `src/rizin/RizinCommand.ts` (+2 -3) 📝 `src/rizin/RizinDebugSessionManager.ts` (+2 -2) 📝 `src/wasm/WabtOutput.test.ts` (+20 -0) 📝 `src/wasm/WasmGlueAdapter.ts` (+13 -6) _...and 5 more files_ </details> ### 📄 Description ## Summary Fixes #3864, fixes #3911, fixes #3912. On POSIX, a caller path such as `link/../sample`, where `link` is a directory symlink, makes the filesystem follow `link` first and then apply `..`. REA made caller paths absolute with `path.resolve`, which collapses `link/..` lexically. As a result, several commands inspected, hashed or launched a sibling file and returned internally consistent, successful Evidence about the wrong artifact. This PR moves that step into one owner that never changes which file a path names. ## Problem and expected behavior I reproduced this on main with the built CLI and a real Mach-O (`gh`): - `target/sample` is a copy of `gh`. - The sibling `./sample` is the same copy with one byte appended. - `link -> target/child`. | command | `link/../sample` (relative) | `$PWD/link/../sample` (absolute) | |---|---|---| | `inspect-go-binary` | sibling `2ba6b7…` ✗ | sibling ✗ | | `inspect-rust-binary` | sibling ✗ | sibling ✗ | | `inspect-with-objdump … file_headers` | sibling ✗ | sibling ✗ | | `inspect-macho`, `list-architectures`, `inspect-signature` | sibling ✗ | intended ✓ | The intended file is `db2578…`. Every command exited 0 in every case. There are two lexical producers: 1. **Relative paths:** CLI adapters (`resolve(args.path)`) and `parseBinaryTarget` (`resolve(cwd, input)`) build the absolute path lexically. 2. **Absolute paths:** they are re-resolved lexically in the Go and Rust CLI adapters, the objdump and Rizin command builders, `RizinDebugSessionManager` launch, and `ReverseEngineeringService`, which hashes objdump and Rizin inputs. Because of the builders and the service, the objdump and Rizin bug also reaches MCP callers. ## Change and scope - New `src/filesystem/callerPath.ts` exports `absoluteCallerPath(path, { cwd?, platform? })`: - **POSIX:** a relative path is prefixed with the working directory (a relative `cwd` is made absolute the same way). Only interior `.` segments and repeated separators are removed, which never change the named file. A trailing `/` or `/.` keeps its directory requirement as one trailing `/`. Every `..` is kept, so the filesystem resolves symlinks and `..` in order. `./module.wasm` from `/app` is `/app/module.wasm`; `link/../x` stays `/app/link/../x`. - **Win32:** keeps `win32.resolve`, because Windows resolves `..` lexically, so that normalization is exact there. - All of the producers above now call it, and their now-unused `resolve`/`isAbsolute` imports are removed: - 13 CLI sites in the `binaryDiagnostics`, `capture`, `coreBinary`, `evm`, `firmware` (input and output directory), `go`, `rust`, `signal`, `sqlite` and `wasm` command modules; - `parseBinaryTarget`, `ReverseEngineeringService`, `objdumpCommand`, `rizinCommand` and the Rizin debug launch; - the WASM `--glue_paths`/`--candidate_paths` lists, `snapshotFirmware` (firmware input) and `SafeOutputTree.create` (extraction output root). These were review follow-ups: the CLI conversion alone did not reach these owners. `SafeOutputTree` uses host path syntax; its `platform` argument selects only the name policy. - Evidence `local_path` now keeps the caller's spelling (for example `…/link/../sample`) when the provider reports it. It no longer reports a different file's path. - Remaining cleanup, not changed here because these sites are outside the reported binary-inspection paths and each needs its own verification: - `JsonFiles`, `EvidenceBundleFiles` and `JavaScriptRecoveryService` still call `resolve(path)` on caller-selected JSON, bundle and source paths. ## Contract and boundary impact - Semantic owner and earliest changed stage: `absoluteCallerPath` owns conversion of a caller-selected local path to an absolute path. The CLI adapters, target resolution, provider command builders and the hashing service consume it. - Contribution scope fit and linked maintainer decision: a bug fix within the existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: schemas unchanged. The same request now selects the file the filesystem names. - Provider, bridge, target-format, or platform compatibility: - POSIX: behavior changes only for paths with a parent component (`..`) after a symlink, or with `.`/duplicate separators, which are now passed through unnormalized. - Windows: unchanged. - Evidence, artifact, provenance, or reconstruction contract: - Subject digest, artifact facts and `local_path` now describe the selected file. - Rizin debug `launch_provenance.resolved_path` now records the absolute path actually launched, which names the same file as `caller_selected_path`. - Target selection, authority, upstream behavior, and cleanup details: the Rizin debug launch no longer starts a sibling program for such a path. There is no other change to execution authority. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. - [x] Launch or attach to a process. (Only the existing Rizin debug launch path; this PR changes which path it receives.) ## Evidence and regression coverage - Tests added or updated: - New `tests/boundary/cli/callerPathSymlinkParentCli.test.ts` (POSIX): public CLI runs of `inspect-go-binary`, `inspect-rust-binary` and `inspect-pe-resources` with relative and absolute `link/../…` spellings, asserting the selected file's SHA-256. It uses the repository's Go and PE resource fixture writers. - New `tests/boundary/filesystem/callerPath.test.ts`: POSIX and win32 helper semantics, including which spellings are normalized. - New `tests/boundary/filesystem/symlinkParentOwners.test.ts` (POSIX): `snapshotFirmware` digests `target/image.bin` for `link/../image.bin`, and `SafeOutputTree.create("…/link/../out")` creates `target/out` and not `./out`. Both fail on the previous commit. - `src/wasm/WabtOutput.test.ts`: a CLI-selected `./module.wasm` keeps an exact `local-path-candidate` glue match against a competing `/other/module.wasm`. This fails with the first version of the helper. - `RizinDebugSessionReviewRegressions.test.ts` (3 cases) and `rizinDebuggerCancellationMcp.test.ts` derived their expected `resolved_path` with lexical `resolve`. They now derive it from the owner. - Base reproduction: with main's `src` restored, the new CLI test fails in both cases. Go selects the sibling, and PE resources selects exactly the sibling digest predicted in #3911 (`9a251f9f…`). With the fix, both pass. - User-visible CLI output after the fix, same layout, real `gh` copy: every command in the table above selects `db2578…` for both spellings. `execute-rizin-command` is unavailable on this host (no Rizin), and `gh` is not a PE file. - Timing, macOS arm64, median of 5 runs, built CLI: | run | `inspect-go-binary link/../sample` | `inspect-go-binary target/sample` | `inspect-macho link/../sample` | `inspect-macho target/sample` | |---|---|---|---|---| | main | 414 ms, 62,695 bytes, wrong file | 406 ms, 62,723 bytes | 1,054 ms, 25,940 bytes, wrong file | 1,118 ms, 25,947 bytes | | fix (run 1) | 496 ms, 62,727 bytes, correct | 473 ms, 62,723 bytes | 946 ms, 25,947 bytes, correct | 930 ms, 25,947 bytes | | fix (run 2) | 385 ms | 387 ms | 997 ms | 933 ms | The helper does a string check and a concatenation. The spread between the two fixed runs (385–496 ms) is host noise; this is not a performance change. Output for the `link/../` spelling grows by 4 bytes, because `local_path` keeps the caller's spelling instead of the collapsed sibling path. - Remaining proof gaps: - Rizin and GNU objdump command lines were verified through tests and the objdump CLI run, not with a real Rizin install. - The Windows behavior is covered only by the helper's win32 unit case. - The sites listed under remaining cleanup are unverified. ## Validation performed - `npm run test:focused -- tests/boundary/filesystem/callerPath.test.ts tests/boundary/cli/callerPathSymlinkParentCli.test.ts` — 4 passed. The CLI test against main's `src`: 2 failed, as expected. - `npx vitest run tests/boundary/cli tests/boundary/providers tests/boundary/filesystem tests/boundary/native src/application tests/acceptance/goBinaryCli.test.ts tests/acceptance/rustBinaryCli.test.ts tests/acceptance/applications/peResources.test.ts` — 465 files: 5,294 passed, 3 failed. All three were the Rizin expectations described above; after updating them the file passes (15 passed). - `npx vitest run tests/boundary/mcp tests/acceptance` — 231 files: 1,249 passed, 1 failed. It was the Rizin MCP expectation described above; after updating it the file passes (3 passed). - After the review follow-up: `npx vitest run tests/boundary/cli tests/boundary/providers tests/boundary/filesystem tests/boundary/native tests/boundary/mcp tests/boundary/firmware src/application src/wasm tests/acceptance` — 700 files, 6,562 passed, 93 skipped, 0 failed. Linux-only firmware integration cases were skipped on this host. - `npm run check:fast` — passed. `npm run knip` — clean. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Requests that previously selected the wrong file now select the one the caller named. - Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 with real native tools (`otool`, `lipo`, `codesign`, objdump). No Linux or Windows host runs. - Package or release metadata impact: none. - Security, privacy, process, or containment review: this closes a path where evidence and a debugger launch could be bound to a file other than the one selected. No new process or network 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:20 +00:00