[PR #5571] [MERGED] fix(artifacts): report a stable symlink loop as a path resolution failure #6970

Closed
opened 2026-11-01 21:44:24 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

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

Base: main ← Head: fix/artifact-symlink-loop


📝 Commits (2)

  • 0e92e2e fix(artifacts): report a stable symlink loop as a path resolution failure
  • e7950d5 Merge remote-tracking branch 'origin/main' into fix/artifact-symlink-loop

📊 Changes

2 files changed (+69 additions, -6 deletions)

View changed files

📝 src/application/BinaryTargetResolver.ts (+34 -6)
➕ tests/acceptance/artifactSymlinkLoopCli.test.ts (+35 -0)

📄 Description

What broke: a stable symlink loop in the selected path was reported as "target changed while being opened", suggesting a race instead of a bad path (#5521).
Fix: BinaryTargetResolver tags filesystem failures by stage; ELOOP before canonicalization is a path-resolution failure, ELOOP on the canonical O_NOFOLLOW open stays "target changed".
Risk: low; message text only, error code unchanged; the read-stage race path keeps its wording.

Summary

Fixes #5521. inspect-artifact and extract-artifact reported a stable symbolic link loop in the selected path as target changed while being opened: ELOOP ..., which is not true: nothing changed. They now report target path could not be resolved: too many symbolic links or a symbolic link loop: ELOOP .... The structured code, category and retryability stay the same (target_unavailable, unavailable, retryable: false).

Problem and expected behavior

How to trigger it: ln -s loop.db loop.db; rea inspect-artifact loop.db --json, or select tree/file with tree -> tree. parseBinaryTarget fails at the first access call, before canonicalization or any open. It then sends the error through classifyBinaryTargetFilesystemFailure, which maps every ELOOP to "target changed while being opened". That wording only fits ELOOP from opening the canonical path with O_NOFOLLOW, where a symbolic link showing up means the path changed after realpath.

Change and scope

BinaryTargetResolver owns this mapping and knows which stage failed, so the fix is there. parseBinaryTarget tracks whether the failure came while resolving the caller-selected path (access/realpath) or after canonicalization. Only the resolve stage gets the new ELOOP reason. The exported classifyBinaryTargetFilesystemFailure is still the read-stage classifier, which keeps "changed" for its existing consumers. Those are ManagedSnapshot and ManagedMemberComparisonService, and both read a path that was already resolved. Genuine replacement detection is unchanged.

Scope: an ordinary fix inside existing boundaries. No maintainer scope decision is needed.

Contract and boundary impact

  • Semantic owner and earliest changed stage: src/application/BinaryTargetResolver.ts, initial target resolution.

  • Contribution scope fit and linked maintainer decision: not applicable (wording fix inside the existing error contract).

  • CLI and MCP/tool-catalog contract: only the reason/message/remediation.action text for an ELOOP during resolution changes. No schema or code changes.

  • Provider, bridge, target-format, or platform compatibility: none.

  • Evidence, artifact, provenance, or reconstruction contract: none. The selected path and the ELOOP diagnostic are kept.

  • Target selection, authority, upstream behavior, and cleanup details: unchanged.

  • Generated metadata, package, or installation impact: none.

  • Read existing files or captured data.

Evidence and regression coverage

  • Tests added or updated: tests/acceptance/artifactSymlinkLoopCli.test.ts runs the public CLI. It covers inspect-artifact on a self-loop and extract-artifact on a looping directory component. It checks the code, category, retryability, selected path and the new reason, and that both links still have their original targets. It is skipped on Windows.

  • Base reproduction: I put BinaryTargetResolver.ts back to origin/main and both new tests failed, with reason: "target changed while being opened: ELOOP: too many symbolic links encountered, access '<path>'". With the fix, both pass.

  • User-visible CLI output (macOS): "reason": "target path could not be resolved: too many symbolic links or a symbolic link loop: ELOOP: too many symbolic links encountered, access '<path>'"

  • Remaining proof gaps: MCP was not exercised directly. It uses the same shared resolver, but I did not run it. Linux and Windows were not run locally. The read-stage (O_NOFOLLOW) "changed" path has no new test.

  • Observed, derived, and inferred claims remain distinguishable.

  • Artifact identity, source provenance, and failed attempts remain preserved.

  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npm run test:focused -- tests/acceptance/artifactSymlinkLoopCli.test.ts with the source fix reverted: 2 failed, as expected
  • npm run test:focused -- tests/acceptance/artifactSymlinkLoopCli.test.ts tests/acceptance/emptyTargetPathCli.test.ts tests/boundary/providers/native/artifactProvider.failureDetail.test.ts tests/boundary/providers/managed/managedMemberComparison.test.ts: 4 files, 36 tests passed
  • npm run check:fast: typecheck and lint passed

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Only the diagnostic text changes.
  • Real Hopper/Ghidra, browser, or OS coverage: macOS (Darwin arm64) only. No provider is involved.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new filesystem access. Symbolic links are never followed for reading beyond the existing access/realpath.

Author readiness

Human owner: @kaoru0822-kitauji

  • 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/5571 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/20/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/artifact-symlink-loop` --- ### 📝 Commits (2) - [`0e92e2e`](https://github.com/morluto/rea/commit/0e92e2e4186d404c5aef18d35196fa88116fcb67) fix(artifacts): report a stable symlink loop as a path resolution failure - [`e7950d5`](https://github.com/morluto/rea/commit/e7950d58327925e7fdf31bf96f4e57333586a2a2) Merge remote-tracking branch 'origin/main' into fix/artifact-symlink-loop ### 📊 Changes **2 files changed** (+69 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `src/application/BinaryTargetResolver.ts` (+34 -6) ➕ `tests/acceptance/artifactSymlinkLoopCli.test.ts` (+35 -0) </details> ### 📄 Description **What broke:** a stable symlink loop in the selected path was reported as "target changed while being opened", suggesting a race instead of a bad path (#5521). **Fix:** `BinaryTargetResolver` tags filesystem failures by stage; ELOOP before canonicalization is a path-resolution failure, ELOOP on the canonical `O_NOFOLLOW` open stays "target changed". **Risk:** low; message text only, error code unchanged; the read-stage race path keeps its wording. ## Summary Fixes #5521. `inspect-artifact` and `extract-artifact` reported a stable symbolic link loop in the selected path as `target changed while being opened: ELOOP ...`, which is not true: nothing changed. They now report `target path could not be resolved: too many symbolic links or a symbolic link loop: ELOOP ...`. The structured code, category and retryability stay the same (`target_unavailable`, `unavailable`, `retryable: false`). ## Problem and expected behavior How to trigger it: `ln -s loop.db loop.db; rea inspect-artifact loop.db --json`, or select `tree/file` with `tree -> tree`. `parseBinaryTarget` fails at the first `access` call, before canonicalization or any open. It then sends the error through `classifyBinaryTargetFilesystemFailure`, which maps every `ELOOP` to "target changed while being opened". That wording only fits `ELOOP` from opening the canonical path with `O_NOFOLLOW`, where a symbolic link showing up means the path changed after `realpath`. ## Change and scope `BinaryTargetResolver` owns this mapping and knows which stage failed, so the fix is there. `parseBinaryTarget` tracks whether the failure came while resolving the caller-selected path (`access`/`realpath`) or after canonicalization. Only the resolve stage gets the new `ELOOP` reason. The exported `classifyBinaryTargetFilesystemFailure` is still the read-stage classifier, which keeps "changed" for its existing consumers. Those are `ManagedSnapshot` and `ManagedMemberComparisonService`, and both read a path that was already resolved. Genuine replacement detection is unchanged. Scope: an ordinary fix inside existing boundaries. No maintainer scope decision is needed. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/application/BinaryTargetResolver.ts`, initial target resolution. - Contribution scope fit and linked maintainer decision: not applicable (wording fix inside the existing error contract). - CLI and MCP/tool-catalog contract: only the `reason`/`message`/`remediation.action` text for an `ELOOP` during resolution changes. No schema or code changes. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. The selected path and the ELOOP diagnostic are kept. - Target selection, authority, upstream behavior, and cleanup details: unchanged. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: `tests/acceptance/artifactSymlinkLoopCli.test.ts` runs the public CLI. It covers `inspect-artifact` on a self-loop and `extract-artifact` on a looping directory component. It checks the code, category, retryability, selected path and the new reason, and that both links still have their original targets. It is skipped on Windows. - Base reproduction: I put `BinaryTargetResolver.ts` back to `origin/main` and both new tests failed, with `reason: "target changed while being opened: ELOOP: too many symbolic links encountered, access '<path>'"`. With the fix, both pass. - User-visible CLI output (macOS): `"reason": "target path could not be resolved: too many symbolic links or a symbolic link loop: ELOOP: too many symbolic links encountered, access '<path>'"` - Remaining proof gaps: MCP was not exercised directly. It uses the same shared resolver, but I did not run it. Linux and Windows were not run locally. The read-stage (`O_NOFOLLOW`) "changed" path has no new test. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:focused -- tests/acceptance/artifactSymlinkLoopCli.test.ts` with the source fix reverted: 2 failed, as expected - `npm run test:focused -- tests/acceptance/artifactSymlinkLoopCli.test.ts tests/acceptance/emptyTargetPathCli.test.ts tests/boundary/providers/native/artifactProvider.failureDetail.test.ts tests/boundary/providers/managed/managedMemberComparison.test.ts`: 4 files, 36 tests passed - `npm run check:fast`: typecheck and lint passed ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only the diagnostic text changes. - Real Hopper/Ghidra, browser, or OS coverage: macOS (Darwin arm64) only. No provider is involved. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new filesystem access. Symbolic links are never followed for reading beyond the existing `access`/`realpath`. ## Author readiness Human owner: @kaoru0822-kitauji - [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 - [ ] 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-11-01 21:44:24 +00:00
Sign in to join this conversation.
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#6970
No description provided.