From 48bbea6df3dbf199b3797851a832b6743888977d Mon Sep 17 00:00:00 2001 From: Shxiao Date: Tue, 8 Sep 2026 15:51:57 +0900 Subject: [PATCH] fix(workspace-changes): record symlink targets without the verbatim prefix (#5250) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workspace-changes): record symlink targets without the verbatim prefix os.readlink on Windows reports absolute targets in extended-length form (\?\C:\... or \?\UNC\server\share). The scanner stored that raw spelling, so workspace-change events showed \?\-prefixed targets that do not match ordinary Windows paths. Strip the prefix when recording; POSIX readlink output is unchanged. Skills projection/review readlink sites are untouched — they have no user-facing contract pinned on the spelling. * fix(workspace-changes): gate symlink target normalization to Windows Review follow-up on #5250: - Gate _normalize_symlink_target on os.name == "nt". readlink(2) on POSIX returns the literal string the link was created with, and backslash is a valid filename byte on Linux, so a target that starts with the extended-length prefix there must be recorded verbatim. The docstring's POSIX claim is now provably true. - Commit the unit checks the PR body previously described as ad-hoc: drive and UNC prefix stripping, relative and plain POSIX targets, mid-string prefix left verbatim, and an off-Windows identity case, so the new branch has real coverage on every platform instead of relying on a Windows host with symlink privilege. * test(workspace-changes): force Windows platform in prefix-strip unit tests The os.name gate added in the previous commit makes _normalize_symlink_target a verbatim identity off-Windows, so the two prefix-strip assertions failed on the ubuntu-only unit CI. Force os.name to "nt" via monkeypatch in both, mirroring the off-Windows identity test, so every case pins exactly one platform's contract and the suite is green on every host. * fix(workspace-changes): strip only extended drive-letter prefixes Review follow-up on #5250: the catch-all branch also stripped the extended-length prefix from volume-GUID targets (\?\Volume{...}\...), leaving a relative-looking path that loses the target's namespace. Restrict the branch to extended drive-letter paths (letter, colon, separator) and keep every other \?\ namespace form verbatim; add the volume-GUID regression plus degenerate-prefix cases. --- .../deerflow/workspace_changes/scanner.py | 29 +++++++++++ backend/tests/test_workspace_changes.py | 50 ++++++++++++++++++- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/backend/packages/harness/deerflow/workspace_changes/scanner.py b/backend/packages/harness/deerflow/workspace_changes/scanner.py index 50cb956f8..f4c28931a 100644 --- a/backend/packages/harness/deerflow/workspace_changes/scanner.py +++ b/backend/packages/harness/deerflow/workspace_changes/scanner.py @@ -259,6 +259,33 @@ def _snapshot_file( ) +def _normalize_symlink_target(target: str) -> str: + """Strip the Windows extended-length prefix from a symlink target. + + ``os.readlink`` on Windows reports absolute targets in extended-length + form (``\\\\?\\C:\\...`` or ``\\\\?\\UNC\\server\\share``). Recorded targets + are surfaced in workspace-change events and compared against ordinary + paths, so keep the plain spelling. + + On POSIX this is a provable identity: the strip only applies on Windows + hosts. ``readlink(2)`` returns the literal string the link was created + with, and backslash is a valid filename byte on Linux — a target string + that merely starts with ``\\\\?\\`` there must be recorded verbatim. + + On Windows, only extended *drive-letter* paths are stripped. Other + ``\\\\?\\`` namespace forms (volume-GUID paths, device paths) are kept + verbatim: stripping them would leave a relative-looking remainder that + no longer names the target's namespace. + """ + if os.name != "nt": + return target + if target.startswith("\\\\?\\UNC\\"): + return "\\\\" + target[len("\\\\?\\UNC\\") :] + if target.startswith("\\\\?\\") and len(target) >= 7 and target[4].isascii() and target[4].isalpha() and target[5] == ":" and target[6] in "\\/": + return target[4:] + return target + + def _snapshot_symlink(root: WorkspaceRoot, host_file: Path) -> FileSnapshot | None: # Deliberately never follows the link (no read_bytes()/open() on the target): # the target may point anywhere on the host, including outside the scanned @@ -278,6 +305,8 @@ def _snapshot_symlink(root: WorkspaceRoot, host_file: Path) -> FileSnapshot | No target = os.readlink(host_file) except OSError: target = None + else: + target = _normalize_symlink_target(target) return FileSnapshot( path=virtual_path, diff --git a/backend/tests/test_workspace_changes.py b/backend/tests/test_workspace_changes.py index c46f20f87..a2932c45e 100644 --- a/backend/tests/test_workspace_changes.py +++ b/backend/tests/test_workspace_changes.py @@ -1,5 +1,6 @@ from __future__ import annotations +import os from pathlib import Path import pytest @@ -19,7 +20,11 @@ from deerflow.workspace_changes import ( scan_workspace_roots, ) from deerflow.workspace_changes.api import get_workspace_changes_response -from deerflow.workspace_changes.scanner import SAMPLE_BYTES, is_sensitive_workspace_path +from deerflow.workspace_changes.scanner import ( + SAMPLE_BYTES, + _normalize_symlink_target, + is_sensitive_workspace_path, +) def _roots(tmp_path): @@ -741,3 +746,46 @@ async def test_workspace_changes_route_forwards_include_files_flag(): assert response["available"] is True assert response["files"] == [] assert calls["event_types"] == ["workspace_changes"] + + +def test_normalize_symlink_target_strips_extended_length_drive_prefix(monkeypatch): + # The strip only applies on Windows hosts, so force the platform here; + # this test runs on the ubuntu-only CI too. + monkeypatch.setattr(os, "name", "nt") + assert _normalize_symlink_target(r"\\?\C:\Users\u1\target.txt") == r"C:\Users\u1\target.txt" + + +def test_normalize_symlink_target_strips_extended_length_unc_prefix(monkeypatch): + monkeypatch.setattr(os, "name", "nt") + assert _normalize_symlink_target(r"\\?\UNC\server\share\a.txt") == r"\\server\share\a.txt" + + +def test_normalize_symlink_target_preserves_volume_guid_and_degenerate_prefixes(monkeypatch): + monkeypatch.setattr(os, "name", "nt") + # Volume-GUID paths are absolute Windows targets in their own namespace; + # stripping the prefix would leave a relative-looking path. + target = r"\\?\Volume{01234567-89ab-cdef-0123-456789abcdef}\folder\target.txt" + assert _normalize_symlink_target(target) == target + # Only a drive letter followed by a colon and a separator is a drive path. + for degenerate in (r"\\?\C:", r"\\?\1:\x", r"\\?\:"): + assert _normalize_symlink_target(degenerate) == degenerate + + +def test_normalize_symlink_target_leaves_relative_and_plain_posix_targets_verbatim(): + assert _normalize_symlink_target("relative/target.txt") == "relative/target.txt" + assert _normalize_symlink_target("/tmp/target.txt") == "/tmp/target.txt" + + +def test_normalize_symlink_target_leaves_mid_string_prefix_verbatim(): + # Backslash is a legal filename byte on POSIX, so only a *leading* + # extended-length prefix may ever be stripped. + for target in (r"/data/\\?\weird-target.txt", r"C:\data\\?\nested.txt"): + assert _normalize_symlink_target(target) == target + + +def test_normalize_symlink_target_is_identity_off_windows(monkeypatch): + # The strip is gated on Windows hosts: readlink(2) on POSIX returns the + # literal string the link was created with, so a target that starts with + # "\\?\" there must be recorded verbatim. + monkeypatch.setattr(os, "name", "posix") + assert _normalize_symlink_target(r"\\?\C:\Users\u1\target.txt") == r"\\?\C:\Users\u1\target.txt"