Repository navigation
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to Links containing ../ that leave the workspace should now open as host files, and links that stay inside the workspace should still resolve as workspace files. No merge-blocking risk was identified. Architecture SummaryArchitecture risk: 🟡 Medium · up to The change affects 1 system. Changed systems: Architecture concerns Review details
🚥 Pre-merge checks | ✅ 4
✨ Finishing Touches
Comment |
| } else if (!isAbsolutePath(path)) { | ||
| const separator: "/" | "\\" = isWindowsPathStyle(cwd) ? "\\" : "/"; | ||
| resolvedPath = joinPath(cwd, path, separator); | ||
| resolvedPath = collapsePathSegments(joinPath(cwd, path, separator)); |
There was a problem hiding this comment.
🟡 Medium src/terminal-links.ts:286
Relative targets such as link/../notes.md now resolve to the wrong file when link is a symlink: the returned /workspace/notes.md bypasses the OS resolution to /outside/notes.md. collapsePathSegments removes .. lexically before the filesystem can resolve link; preserve the relative path segments when joining with cwd.
| resolvedPath = collapsePathSegments(joinPath(cwd, path, separator)); | |
| resolvedPath = joinPath(cwd, path, separator); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/terminal-links.ts around line 286:
Relative targets such as `link/../notes.md` now resolve to the wrong file when `link` is a symlink: the returned `/workspace/notes.md` bypasses the OS resolution to `/outside/notes.md`. `collapsePathSegments` removes `..` lexically before the filesystem can resolve `link`; preserve the relative path segments when joining with `cwd`.
| const collapsed = collapse(resolvedPath.split(/[\\/]+/).filter(Boolean), false); | ||
| normalized = `\\\\${collapsed.join("\\")}`; |
There was a problem hiding this comment.
🟡 Medium src/terminal-links.ts:149
UNC links containing .. resolve to the wrong host path: \\server\share\..\notes.md becomes \\server\notes.md, and \\?\C:\..\notes.md loses the device drive root. The branch collapses the server/share or device-prefix components as ordinary segments; preserve the UNC root (and the extended device root) while collapsing only the path beneath it.
- const collapsed = collapse(resolvedPath.split(/[\\/]+/).filter(Boolean), false);
- normalized = `\\\\${collapsed.join("\\")}`;
+ const components = resolvedPath.split(/[\\/]+/).filter(Boolean);
+ const rootComponentCount =
+ (components[0] === "?" || components[0] === ".") &&
+ components[1]?.toUpperCase() === "UNC"
+ ? 4
+ : 2;
+ const root = components.slice(0, rootComponentCount);
+ const collapsed = collapse(components.slice(rootComponentCount), false);
+ normalized = `\\\\${[...root, ...collapsed].join("\\")}`;🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/terminal-links.ts around lines 149-150:
UNC links containing `..` resolve to the wrong host path: `\\server\share\..\notes.md` becomes `\\server\notes.md`, and `\\?\C:\..\notes.md` loses the device drive root. The branch collapses the server/share or device-prefix components as ordinary segments; preserve the UNC root (and the extended device root) while collapsing only the path beneath it.
| : ""; | ||
| const body = tildePrefix ? resolvedPath.slice(2) : resolvedPath; | ||
| const separator = body.includes("\\") && !body.includes("/") ? "\\" : "/"; | ||
| const collapsed = collapse(body.split(/[\\/]+/), true); |
There was a problem hiding this comment.
🟡 Medium src/terminal-links.ts:170
On POSIX, a relative filename such as a\..\notes.md is collapsed to notes.md, so the caller opens ${cwd}/notes.md instead of the literal file ${cwd}/a\..\notes.md. The relative-path branch selects \ as the separator and splits on both slash types based only on the input; make separator and segment parsing platform-aware so POSIX treats only / as a separator.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/terminal-links.ts around line 170:
On POSIX, a relative filename such as `a\..\notes.md` is collapsed to `notes.md`, so the caller opens `${cwd}/notes.md` instead of the literal file `${cwd}/a\..\notes.md`. The relative-path branch selects `\` as the separator and splits on both slash types based only on the input; make separator and segment parsing platform-aware so POSIX treats only `/` as a separator.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This bug fix changes shared path resolution across terminal, Markdown, preview, and editor-opening flows with a nontrivial cross-platform normalizer. The implementation has unresolved Medium-severity concerns around symlinks, UNC roots, and POSIX backslashes, so its broader runtime impact merits human review. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
…host files A ../ link to a file outside the workspace joined to the workspace root without collapsing .. segments, so the result kept the root as a string prefix and misclassified as a workspace file. Collapse . and .. segments in resolvePathLinkTarget so outside-workspace targets fall through to host-file handling while inside-workspace ../ links keep working.
b10196d to
972954f
Compare
Collapse in resolvePathLinkTarget normalizes C:/Users/julius + notes to all-backslash form.
|
Note Written by Hi! We are cleaning up open PRs, and this one does not say which model or harness was used to create it. If this change is really important, we recommend rebuilding the PR with a newer model and noting the model and harness in the PR description. |
A file link with ../ to a file outside the workspace failed to open with "Failed to read workspace file '../other/notes.md'".
resolvePathLinkTarget joined the relative target to the workspace root without collapsing .. segments, so /home/me/project/../other/notes.md kept the root as a string prefix and the workspace check misclassified it as a workspace file.
Collapse . and .. segments in resolvePathLinkTarget (POSIX, Windows drive, UNC, and ~/ forms) before the workspace check, so outside-workspace targets fall through to host-file handling while ../ links staying inside still resolve as workspace files. Adds focused unit tests in terminal-links.test.ts and markdown-links.test.ts.
Verified: vp test run apps/web/src/terminal-links.test.ts apps/web/src/markdown-links.test.ts — 2 files, 92 tests passed.
Built with Muse Spark 1.3 Free via the OpenCode harness.