morluto/rea

Fork 0

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

[PR #3562] [CLOSED] fix(reference): preserve physical symlink targets #5827

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/reference-selected-path

📝 Commits (10+)

08e16bd fix(reference): preserve selected roots and symlink targets
8bf5c41 test(cli): allow headroom for shared profile lifecycle
f90fb7a fix(reference): reconcile path selection with upstream race handling
76b2847 fix(reference): validate native link targets and restore schema compatibility
67daaf0 chore: sync main and preserve reference path regressions
855c549 chore: sync main into reference path fix
60e20e8 test(cli): restore analysis view cancellation injection after refactor
9d81b1e chore: sync main and retain upstream cancellation tests
ee3d8a0 fix(reference): retain physical symlink targets after upstream sync
d43268d chore: sync main to restore reference PR CI

📊 Changes

5 files changed (+149 additions, -20 deletions)

View changed files

📝 src/application/ReferenceSourceImportEntries.ts (+14 -6)
📝 src/reference/ReferenceSourceReaderEntries.ts (+23 -13)
📝 src/reference/ReferenceSourceReaderTypes.ts (+9 -1)
📝 tests/boundary/filesystem/referenceSourceImport.test.ts (+100 -0)
📝 tests/process-global/referenceSourceSymlinkRace.test.ts (+3 -0)

📄 Description

Closed: core fix superseded upstream

Closing this PR without merging after verifying upstream main a2e2c29b59e8775b0e6aedb09a9eb16adf32646b on 2026-10-27.

The core physical symlink target fixes are now implemented upstream in 09acf217: preserve native path traversal, validate it with stat before realpath, classify containment from the actual destination, and retain unresolved path spelling. The earlier selected-root issue was already fixed in 5e8f8b31.

Supersession verification

Executed on macOS x64 with Node 24.22.1 / npm 11.16.0, using an isolated checkout of that upstream commit with no production changes:

Built the upstream runtime and passed 82 tests across 7 focused files: the reader, import projection, graph schema, filesystem imports, symlink races, upstream compiled CLI symlink-path regression, and a temporary replay of this PR's original 12-link fixture.
The original regression body was first run unchanged and failed at its old unknown/null expectation. This is recorded as a failed run, not an unchanged-test pass.
Upstream intentionally reports those four invalid paths as missing with their uncollapsed absolute spelling. A separate contract-aligned replay verified those exact paths, confirmed that native stat rejects both the links and returned paths, and checked partial inventory and schema validity. Every other original assertion remained: correct internal/external targets, external paths returning inside the root, ordinary and backslash-containing missing paths, the exact file inventory, and no outside-file contents.
Independent comparison confirmed that the replay preserved the original fixtures and all non-representation assertions. The unchanged upstream CLI regression additionally passed retargeting, deletion, and movement outside the root.

The core user-visible bug is therefore covered upstream. This does not claim the two implementations are identical in every representation choice. Linux, Windows, and the complete suite were not rerun for this closure; no new implementation was pushed.

Earlier proposal and validation, retained for history

Summary

Resolve imported symlinks through the filesystem before recording their target. With current -> actual/nested, a link to current/../target.js points to actual/target.js; main currently records the unrelated target.js. The same premature normalization can misclassify targets as internal/external or make an invalid path appear valid.

The reader validates native lookup with stat, then resolves the observed link itself. Ordinary missing targets retain their spelling; paths whose intent cannot be preserved in a normalized identity remain unknown with the original target and error code. Resolution reads metadata, not target contents. Existing identity checks, cleanup, and upstream raw-byte/UTF-8/BOM handling are retained.

The diff against main is limited to five symlink implementation/test files. The selected-root fix is already upstream in 5e8f8b3163. The race fixture timestamp adjustment makes directory replacement observable on coarse filesystems. No public schema or CLI/MCP option changes.

CI repair

Synced main 1173c0a9dc1b352a60943a491462294db8f353ae to include two upstream corrections for the previous CI failures:

#6945 aligns Apple metadata tests with the bound-target AnalysisArtifactChangedError contract.
#6951 fixes the invalid Webpack factory fixture and executes the complete registered factory in its independent oracle.

No new production changes or weakened assertions were added for these failures.

Validation

Current head: 1a1955244869cfefdfde810a6b34013320163d5b (same tested tree as d43268de). Local macOS x64, Node 24.22.1, npm 11.16.0:

Runtime build passed. 232 tests passed, 2 platform skips across 16 focused files (15 passed files, one platform-skipped file): reference reader/import/graph, filesystem imports and failures, symlink races, compiled CLI directory and UTF-8 cases, Apple metadata and real MCP bound-target recovery, and JavaScript factory export/index analysis.
npm run check, npm run check:fast, and diff whitespace checks passed. The normal pre-commit hook passed; its unrelated upstream import cleanups were excluded. The published tree exactly matches the focused-test and full-check tree.
Independent review found no actionable integration issue in the retained five-file diff.
The complete applicationWorkflowCli.test.ts run could not finish locally because the host Swift SDK/compiler reports a duplicate SwiftBridging module when compiling the process helper. This was reproduced with a direct compiler invocation; the affected run was stopped and the remaining 16 files were rerun successfully. Hosted Linux CI subsequently passed that complete CLI regression.
Native Windows and the complete deterministic suite were not run locally.
The first synchronized CI run passed 18,921 source tests, static/build/native package checks, then failed one MCP recovery assertion in javascriptAnalysisResources.test.ts: parent_result_budget_bytes: 0 after retaining the oversized timeout observation. Other shards were cancelled by fail-fast. This is the intermittent parent-capacity failure also documented in #7029 and #5672; the same main 1173c0a9 has a successful selected CI run. The current empty verification commit triggers one controlled repeat with identical code, limits, fixtures, and assertions. The identical-tree repeat passed; this does not establish that the upstream flake is fixed.
Final CI run 40257362462 passed on exact head 1a1955244869cfefdfde810a6b34013320163d5b: 33,855 tests passed, zero failed, 237 skipped, all eight built-test shards, source/static/build/native package checks, Portable tests, and CI required succeeded. The Apple error-contract tests, complete application CLI suite (including the Webpack factory case), and the previously intermittent MCP timeout/recovery case passed. Runtime limits, deadlines, fixture sizes, and assertions remain unchanged.

Author readiness

Human owner: @shoyann

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.

_{🔄 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/3562 **Author:** [@shoyann](https://github.com/shoyann) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/reference-selected-path` --- ### 📝 Commits (10+) - [`08e16bd`](https://github.com/morluto/rea/commit/08e16bd67119e910104a70f3667f379f7c86ecdd) fix(reference): preserve selected roots and symlink targets - [`8bf5c41`](https://github.com/morluto/rea/commit/8bf5c418c508f836372a2b3014a6c6a545d29e16) test(cli): allow headroom for shared profile lifecycle - [`f90fb7a`](https://github.com/morluto/rea/commit/f90fb7ae25bc7bae7e80de1e2a3d3969328eaa73) fix(reference): reconcile path selection with upstream race handling - [`76b2847`](https://github.com/morluto/rea/commit/76b2847cbfcb6d5280810c8bf8cb2e68439046f4) fix(reference): validate native link targets and restore schema compatibility - [`67daaf0`](https://github.com/morluto/rea/commit/67daaf013e8ebf80d8385f7bfaed116e2035eba0) chore: sync main and preserve reference path regressions - [`855c549`](https://github.com/morluto/rea/commit/855c549ce3408638eb9b7d61f80d9a0da59a4adc) chore: sync main into reference path fix - [`60e20e8`](https://github.com/morluto/rea/commit/60e20e8ad6370d97559e67d87516ecbc81fd6ff9) test(cli): restore analysis view cancellation injection after refactor - [`9d81b1e`](https://github.com/morluto/rea/commit/9d81b1eaa7ffccb6e1148285b5726e8306d1991b) chore: sync main and retain upstream cancellation tests - [`ee3d8a0`](https://github.com/morluto/rea/commit/ee3d8a060a91abea829efd6975f1c58c8a61d585) fix(reference): retain physical symlink targets after upstream sync - [`d43268d`](https://github.com/morluto/rea/commit/d43268dec4862dfb5b06380bee51e8fc5d4ff0be) chore: sync main to restore reference PR CI ### 📊 Changes **5 files changed** (+149 additions, -20 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ReferenceSourceImportEntries.ts` (+14 -6) 📝 `src/reference/ReferenceSourceReaderEntries.ts` (+23 -13) 📝 `src/reference/ReferenceSourceReaderTypes.ts` (+9 -1) 📝 `tests/boundary/filesystem/referenceSourceImport.test.ts` (+100 -0) 📝 `tests/process-global/referenceSourceSymlinkRace.test.ts` (+3 -0) </details> ### 📄 Description ## Closed: core fix superseded upstream Closing this PR without merging after verifying upstream main `a2e2c29b59e8775b0e6aedb09a9eb16adf32646b` on 2026-10-27. The core physical symlink target fixes are now implemented upstream in [09acf217](https://github.com/morluto/rea/commit/09acf217d946c7ca3659abe308896e010e9cc764): preserve native path traversal, validate it with `stat` before `realpath`, classify containment from the actual destination, and retain unresolved path spelling. The earlier selected-root issue was already fixed in [5e8f8b31](https://github.com/morluto/rea/commit/5e8f8b3163e4ff731b2c391a897e5b7e6a0f1a90). ### Supersession verification Executed on macOS x64 with Node 24.22.1 / npm 11.16.0, using an isolated checkout of that upstream commit with **no production changes**: - Built the upstream runtime and passed **82 tests across 7 focused files**: the reader, import projection, graph schema, filesystem imports, symlink races, upstream [compiled CLI symlink-path regression](https://github.com/morluto/rea/blob/a2e2c29b59e8775b0e6aedb09a9eb16adf32646b/tests/boundary/cli/referenceSourceSymlinkPaths.test.ts), and a temporary replay of this PR's original 12-link fixture. - The original regression body was first run unchanged and failed at its old `unknown/null` expectation. This is recorded as a failed run, not an unchanged-test pass. - Upstream intentionally reports those four invalid paths as `missing` with their uncollapsed absolute spelling. A separate contract-aligned replay verified those exact paths, confirmed that native `stat` rejects both the links and returned paths, and checked partial inventory and schema validity. Every other original assertion remained: correct internal/external targets, external paths returning inside the root, ordinary and backslash-containing missing paths, the exact file inventory, and no outside-file contents. - Independent comparison confirmed that the replay preserved the original fixtures and all non-representation assertions. The unchanged upstream CLI regression additionally passed retargeting, deletion, and movement outside the root. The core user-visible bug is therefore covered upstream. This does not claim the two implementations are identical in every representation choice. Linux, Windows, and the complete suite were not rerun for this closure; no new implementation was pushed. <details> <summary>Earlier proposal and validation, retained for history</summary> ## Summary Resolve imported symlinks through the filesystem before recording their target. With `current -> actual/nested`, a link to `current/../target.js` points to `actual/target.js`; main currently records the unrelated `target.js`. The same premature normalization can misclassify targets as internal/external or make an invalid path appear valid. The reader validates native lookup with `stat`, then resolves the observed link itself. Ordinary missing targets retain their spelling; paths whose intent cannot be preserved in a normalized identity remain `unknown` with the original target and error code. Resolution reads metadata, not target contents. Existing identity checks, cleanup, and upstream raw-byte/UTF-8/BOM handling are retained. The diff against main is limited to five symlink implementation/test files. The selected-root fix is already upstream in 5e8f8b3163e4ff731b2c391a897e5b7e6a0f1a90. The race fixture timestamp adjustment makes directory replacement observable on coarse filesystems. No public schema or CLI/MCP option changes. ## CI repair Synced main `1173c0a9dc1b352a60943a491462294db8f353ae` to include two upstream corrections for the previous CI failures: - #6945 aligns Apple metadata tests with the bound-target `AnalysisArtifactChangedError` contract. - #6951 fixes the invalid Webpack factory fixture and executes the complete registered factory in its independent oracle. No new production changes or weakened assertions were added for these failures. ## Validation Current head: `1a1955244869cfefdfde810a6b34013320163d5b` (same tested tree as `d43268de`). Local macOS x64, Node **24.22.1**, npm **11.16.0**: - Runtime build passed. **232 tests passed, 2 platform skips across 16 focused files** (15 passed files, one platform-skipped file): reference reader/import/graph, filesystem imports and failures, symlink races, compiled CLI directory and UTF-8 cases, Apple metadata and real MCP bound-target recovery, and JavaScript factory export/index analysis. - `npm run check`, `npm run check:fast`, and diff whitespace checks passed. The normal pre-commit hook passed; its unrelated upstream import cleanups were excluded. The published tree exactly matches the focused-test and full-check tree. - Independent review found no actionable integration issue in the retained five-file diff. - The complete `applicationWorkflowCli.test.ts` run could not finish locally because the host Swift SDK/compiler reports a duplicate `SwiftBridging` module when compiling the process helper. This was reproduced with a direct compiler invocation; the affected run was stopped and the remaining 16 files were rerun successfully. Hosted Linux CI subsequently passed that complete CLI regression. - Native Windows and the complete deterministic suite were not run locally. - The first synchronized [CI run](https://github.com/morluto/rea/actions/runs/40256150975) passed 18,921 source tests, static/build/native package checks, then failed one MCP recovery assertion in `javascriptAnalysisResources.test.ts`: `parent_result_budget_bytes: 0` after retaining the oversized timeout observation. Other shards were cancelled by fail-fast. This is the intermittent parent-capacity failure also documented in #7029 and #5672; the same main `1173c0a9` has a successful selected CI run. The current empty verification commit triggers one controlled repeat with identical code, limits, fixtures, and assertions. The identical-tree repeat passed; this does not establish that the upstream flake is fixed. - Final [CI run 40257362462](https://github.com/morluto/rea/actions/runs/40257362462) passed on exact head `1a1955244869cfefdfde810a6b34013320163d5b`: **33,855 tests passed, zero failed, 237 skipped**, all eight built-test shards, source/static/build/native package checks, `Portable tests`, and `CI required` succeeded. The Apple error-contract tests, complete application CLI suite (including the Webpack factory case), and the previously intermittent MCP timeout/recovery case passed. Runtime limits, deadlines, fixture sizes, and assertions remain unchanged. ## Author readiness Human owner: @shoyann - [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. </details> --- <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:44 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns