From 2feac5663dad9afbea75aaa77c4a9faa6f86ca64 Mon Sep 17 00:00:00 2001 From: waleed Date: Thu, 1 Oct 2026 13:28:36 +0300 Subject: [PATCH 1/3] Keep the commit id in archive names for Windows drive paths On Windows the file path of a commit-range document is absolute by drive letter (C:\repo\file), not by a leading separator, so os.path.join(commit_id, path) threw the commit id away. The archive entry became C:/repo/file, the server reported "C:" as the commit id and a drive-less file path, the same file changed in several commits collided in the archive (zipfile "Duplicate name" warning), and the text printer could not match the finding to a document. Strip the drive and the leading separators with ntpath.splitdrive on every OS before joining, and apply the same normalisation when matching the server's file names back to local documents. Reported in support ticket #8417. Co-Authored-By: Claude Fable 5.1 --- cycode/cli/utils/path_utils.py | 29 ++++++++++++++------ tests/cli/commands/scan/test_scan_result.py | 11 ++++++++ tests/cli/utils/test_path_utils_normalize.py | 19 +++++++++++++ tests/test_zip_file.py | 16 +++++++++++ 4 files changed, 67 insertions(+), 8 deletions(-) create mode 100644 tests/cli/utils/test_path_utils_normalize.py diff --git a/cycode/cli/utils/path_utils.py b/cycode/cli/utils/path_utils.py index ea6d2f38..48b41fb2 100644 --- a/cycode/cli/utils/path_utils.py +++ b/cycode/cli/utils/path_utils.py @@ -1,4 +1,5 @@ import json +import ntpath import os import tempfile from functools import cache @@ -125,12 +126,25 @@ def change_filename_extension(filename: str, extension: str) -> str: return f'{base_name}.{extension}' +def _strip_path_anchor(path: str) -> str: + """Drop the drive (``C:``, ``\\\\server\\share``) and the leading separators so the path becomes relative. + + ``os.path.join(prefix, path)`` throws the prefix away when ``path`` is absolute. On POSIX only a leading + slash makes a path absolute, but on Windows a drive letter does too, and ``C:\\repo\\file`` doesn't start + with a separator. ``ntpath.splitdrive`` is used on every OS so the behavior is the same everywhere + (on POSIX it is a no-op for anything that doesn't look like ``X:``). + """ + _, path = ntpath.splitdrive(path) + return path.lstrip('/\\') + + def concat_unique_id(filename: str, unique_id: str) -> str: - if filename.startswith(os.sep): - # remove leading slash to join the path correctly - filename = filename[len(os.sep) :] + """Prefix the file name with the unique id (e.g. a commit SHA) to build the name of the archive entry. - return os.path.join(unique_id, filename) + The server reads the unique id back from the first path component, so the file name must become relative + before joining; otherwise the prefix is lost and a Windows drive letter is reported as the commit id. + """ + return os.path.join(unique_id, _strip_path_anchor(filename)) def get_path_from_context(ctx: typer.Context) -> Optional[str]: @@ -141,8 +155,7 @@ def get_path_from_context(ctx: typer.Context) -> Optional[str]: def normalize_file_path(path: str) -> str: - if path.startswith('/'): - return path[1:] - if path.startswith('./'): + """Make a path comparable with the file name the server reports back: no drive, no root, no leading ``./``.""" + if path.startswith(('./', '.\\')): return path[2:] - return path + return _strip_path_anchor(path) diff --git a/tests/cli/commands/scan/test_scan_result.py b/tests/cli/commands/scan/test_scan_result.py index 6b992f95..c10168df 100644 --- a/tests/cli/commands/scan/test_scan_result.py +++ b/tests/cli/commands/scan/test_scan_result.py @@ -76,3 +76,14 @@ def test_get_document_detections_keeps_detection_when_document_is_not_found() -> assert document_detections[0].document.path == 'unknown.txt' assert len(document_detections[0].detections) == 1 + + +def test_get_document_detections_matches_commit_document_with_windows_drive_path() -> None: + # the archived name never carries the drive letter, so the server reports the path without it + commit_id = 'a' * 40 + document = Document('C:\\repo\\creds.txt', 'content', unique_id=commit_id) + scan_result = _scan_result_for(concat_unique_id(document.path, commit_id), commit_id) + + document_detections = _get_document_detections(scan_result, [document]) + + assert document_detections[0].document is document diff --git a/tests/cli/utils/test_path_utils_normalize.py b/tests/cli/utils/test_path_utils_normalize.py new file mode 100644 index 00000000..a04007ed --- /dev/null +++ b/tests/cli/utils/test_path_utils_normalize.py @@ -0,0 +1,19 @@ +import pytest + +from cycode.cli.utils.path_utils import normalize_file_path + + +@pytest.mark.parametrize( + ('path', 'expected'), + [ + ('/repo/creds.txt', 'repo/creds.txt'), + ('./repo/creds.txt', 'repo/creds.txt'), + ('repo/creds.txt', 'repo/creds.txt'), + ('C:\\repo\\creds.txt', 'repo\\creds.txt'), + ('C:/repo/creds.txt', 'repo/creds.txt'), + ('.\\repo\\creds.txt', 'repo\\creds.txt'), + ('\\\\server\\share\\creds.txt', 'creds.txt'), + ], +) +def test_normalize_file_path(path: str, expected: str) -> None: + assert normalize_file_path(path) == expected diff --git a/tests/test_zip_file.py b/tests/test_zip_file.py index 15c53c17..b96f981d 100644 --- a/tests/test_zip_file.py +++ b/tests/test_zip_file.py @@ -20,3 +20,19 @@ def test_concat_unique_id_to_file_without_leading_slash() -> None: expected_path = os.path.join(unique_id, *filename.split('/')) assert concat_unique_id(filename, unique_id) == expected_path + + +def test_concat_unique_id_keeps_unique_id_for_windows_drive_path() -> None: + # os.path.join drops the prefix when the file name is absolute; on Windows a drive letter makes it absolute, + # which used to archive the file as 'C:/repo/file' and the server then reported 'C:' as the commit id + unique_id = 'a' * 40 + + # separators inside the file name are kept as-is; only the drive and the leading separator go away + assert concat_unique_id('C:\\repo\\creds.txt', unique_id) == os.path.join(unique_id, 'repo\\creds.txt') + assert concat_unique_id('C:/repo/creds.txt', unique_id) == os.path.join(unique_id, 'repo/creds.txt') + + +def test_concat_unique_id_keeps_unique_id_for_unc_path() -> None: + unique_id = 'a' * 40 + + assert concat_unique_id('\\\\server\\share\\creds.txt', unique_id) == os.path.join(unique_id, 'creds.txt') From c65958f20332fe0d4354b779e484abf9a28fbef3 Mon Sep 17 00:00:00 2001 From: waleed Date: Thu, 1 Oct 2026 14:53:30 +0300 Subject: [PATCH 2/3] Build archive entry names as POSIX paths from the running OS's flavor Address review: pick the pathlib pure flavor from the running OS instead of running ntpath.splitdrive everywhere, and emit forward-slash components. This keeps the UNC server and share in the entry name so two shares don't collide, stops a colon in a POSIX file name from being read as a drive, and makes the archive naming and the result matching agree without relying on zipfile's separator rewrite or get_path_by_os. Introduce a shared OPERATING_SYSTEM constant in consts (the other platform checks in the codebase are left for a follow-up), and move all path_utils tests into tests/cli/utils/test_path_utils.py, with the Windows cases exercised on POSIX CI by passing the flavor explicitly. Co-Authored-By: Claude Fable 5.1 --- cycode/cli/apps/scan/scan_result.py | 4 +- cycode/cli/consts.py | 20 +++++++ cycode/cli/utils/path_utils.py | 32 +++++----- tests/cli/commands/scan/test_scan_result.py | 11 ---- tests/cli/utils/test_path_utils.py | 62 ++++++++++++++++++++ tests/cli/utils/test_path_utils_normalize.py | 19 ------ tests/test_zip_file.py | 38 ------------ 7 files changed, 102 insertions(+), 84 deletions(-) create mode 100644 tests/cli/utils/test_path_utils.py delete mode 100644 tests/cli/utils/test_path_utils_normalize.py delete mode 100644 tests/test_zip_file.py diff --git a/cycode/cli/apps/scan/scan_result.py b/cycode/cli/apps/scan/scan_result.py index 16c97a90..ea598a12 100644 --- a/cycode/cli/apps/scan/scan_result.py +++ b/cycode/cli/apps/scan/scan_result.py @@ -7,7 +7,7 @@ from cycode.cli.apps.scan.aggregation_report import try_get_aggregation_report_url_if_needed from cycode.cli.apps.scan.detection_excluder import exclude_irrelevant_document_detections from cycode.cli.models import Document, DocumentDetections, LocalScanResult -from cycode.cli.utils.path_utils import concat_unique_id, get_path_by_os, normalize_file_path +from cycode.cli.utils.path_utils import concat_unique_id, normalize_file_path from cycode.cyclient.models import ( Detection, DetectionSchema, @@ -49,7 +49,7 @@ def _get_document_detections( document_detections = [] for detections_per_file in scan_result.detections_per_file: - file_name = get_path_by_os(detections_per_file.file_name) + file_name = detections_per_file.file_name commit_id = detections_per_file.commit_id logger.debug( diff --git a/cycode/cli/consts.py b/cycode/cli/consts.py index c2153d07..278c9596 100644 --- a/cycode/cli/consts.py +++ b/cycode/cli/consts.py @@ -1,7 +1,27 @@ +import sys +from enum import Enum + PROGRAM_NAME = 'cycode' APP_NAME = 'CycodeCLI' CLI_CONTEXT_SETTINGS = {'terminal_width': 10**9, 'max_content_width': 10**9, 'help_option_names': ['-h', '--help']} + +class OperatingSystem(str, Enum): + WINDOWS = 'windows' + MACOS = 'macos' + LINUX = 'linux' + + +def _detect_operating_system() -> OperatingSystem: + if sys.platform == 'win32': + return OperatingSystem.WINDOWS + if sys.platform == 'darwin': + return OperatingSystem.MACOS + return OperatingSystem.LINUX + + +OPERATING_SYSTEM = _detect_operating_system() + PRE_COMMIT_COMMAND_SCAN_TYPE = 'pre-commit' PRE_COMMIT_COMMAND_SCAN_TYPE_OLD = 'pre_commit' PRE_RECEIVE_COMMAND_SCAN_TYPE = 'pre-receive' diff --git a/cycode/cli/utils/path_utils.py b/cycode/cli/utils/path_utils.py index 48b41fb2..c4ae78fe 100644 --- a/cycode/cli/utils/path_utils.py +++ b/cycode/cli/utils/path_utils.py @@ -1,12 +1,13 @@ import json -import ntpath import os import tempfile from functools import cache +from pathlib import PurePath, PurePosixPath, PureWindowsPath from typing import TYPE_CHECKING, AnyStr, Optional, Union import typer +from cycode.cli.consts import OPERATING_SYSTEM, OperatingSystem from cycode.cli.logger import logger from cycode.cli.utils.binary_utils import is_binary_string @@ -126,16 +127,20 @@ def change_filename_extension(filename: str, extension: str) -> str: return f'{base_name}.{extension}' -def _strip_path_anchor(path: str) -> str: - """Drop the drive (``C:``, ``\\\\server\\share``) and the leading separators so the path becomes relative. +def _to_relative_posix_parts(path: str, operating_system: OperatingSystem = OPERATING_SYSTEM) -> tuple[str, ...]: + """Split a path into OS-agnostic components: drop the root, keep the UNC server and share. - ``os.path.join(prefix, path)`` throws the prefix away when ``path`` is absolute. On POSIX only a leading - slash makes a path absolute, but on Windows a drive letter does too, and ``C:\\repo\\file`` doesn't start - with a separator. ``ntpath.splitdrive`` is used on every OS so the behavior is the same everywhere - (on POSIX it is a no-op for anything that doesn't look like ``X:``). + The archive entry name is read back by the server as ``/``, so it must use forward slashes + and must never start with a root. The flavor comes from the running OS rather than from the path itself, so + a colon in a POSIX file name is not mistaken for a drive letter. """ - _, path = ntpath.splitdrive(path) - return path.lstrip('/\\') + pure: PurePath = PureWindowsPath(path) if operating_system is OperatingSystem.WINDOWS else PurePosixPath(path) + if not pure.anchor: + return pure.parts + + # the anchor is 'C:\\', '/' or '\\\\server\\share\\': keep the names it carries, drop the drive and separators + anchor_parts = tuple(p for p in pure.anchor.replace('\\', '/').split('/') if p and not p.endswith(':')) + return anchor_parts + pure.parts[1:] def concat_unique_id(filename: str, unique_id: str) -> str: @@ -144,7 +149,7 @@ def concat_unique_id(filename: str, unique_id: str) -> str: The server reads the unique id back from the first path component, so the file name must become relative before joining; otherwise the prefix is lost and a Windows drive letter is reported as the commit id. """ - return os.path.join(unique_id, _strip_path_anchor(filename)) + return str(PurePosixPath(unique_id, *_to_relative_posix_parts(filename))) def get_path_from_context(ctx: typer.Context) -> Optional[str]: @@ -155,7 +160,6 @@ def get_path_from_context(ctx: typer.Context) -> Optional[str]: def normalize_file_path(path: str) -> str: - """Make a path comparable with the file name the server reports back: no drive, no root, no leading ``./``.""" - if path.startswith(('./', '.\\')): - return path[2:] - return _strip_path_anchor(path) + """Make a path comparable with the file name the server reports back: no drive, no root, forward slashes.""" + parts = _to_relative_posix_parts(path) + return str(PurePosixPath(*parts)) if parts else '' diff --git a/tests/cli/commands/scan/test_scan_result.py b/tests/cli/commands/scan/test_scan_result.py index c10168df..6b992f95 100644 --- a/tests/cli/commands/scan/test_scan_result.py +++ b/tests/cli/commands/scan/test_scan_result.py @@ -76,14 +76,3 @@ def test_get_document_detections_keeps_detection_when_document_is_not_found() -> assert document_detections[0].document.path == 'unknown.txt' assert len(document_detections[0].detections) == 1 - - -def test_get_document_detections_matches_commit_document_with_windows_drive_path() -> None: - # the archived name never carries the drive letter, so the server reports the path without it - commit_id = 'a' * 40 - document = Document('C:\\repo\\creds.txt', 'content', unique_id=commit_id) - scan_result = _scan_result_for(concat_unique_id(document.path, commit_id), commit_id) - - document_detections = _get_document_detections(scan_result, [document]) - - assert document_detections[0].document is document diff --git a/tests/cli/utils/test_path_utils.py b/tests/cli/utils/test_path_utils.py new file mode 100644 index 00000000..d49c878c --- /dev/null +++ b/tests/cli/utils/test_path_utils.py @@ -0,0 +1,62 @@ +from pathlib import PurePosixPath + +import pytest + +from cycode.cli.consts import OperatingSystem +from cycode.cli.utils.path_utils import _to_relative_posix_parts, concat_unique_id, normalize_file_path + + +def test_concat_unique_id_to_file_with_leading_slash() -> None: + assert concat_unique_id('/path/to/file', 'unique_id') == 'unique_id/path/to/file' + + +def test_concat_unique_id_to_file_without_leading_slash() -> None: + assert concat_unique_id('path/to/file', 'unique_id') == 'unique_id/path/to/file' + + +def test_concat_unique_id_keeps_unique_id_for_windows_drive_path() -> None: + unique_id = 'a' * 40 + + # the entry name is always posix: the drive and the root go away, backslashes become forward slashes + for path in ('C:\\repo\\creds.txt', 'C:/repo/creds.txt'): + parts = _to_relative_posix_parts(path, OperatingSystem.WINDOWS) + assert str(PurePosixPath(unique_id, *parts)) == f'{unique_id}/repo/creds.txt' + + +def test_concat_unique_id_keeps_server_and_share_for_unc_path() -> None: + unique_id = 'a' * 40 + + # the server and the share are part of the path: two shares must not collapse onto the same entry + parts = _to_relative_posix_parts('\\\\server\\share\\creds.txt', OperatingSystem.WINDOWS) + assert str(PurePosixPath(unique_id, *parts)) == f'{unique_id}/server/share/creds.txt' + + +@pytest.mark.parametrize( + ('path', 'expected'), + [ + ('/repo/creds.txt', 'repo/creds.txt'), + ('./repo/creds.txt', 'repo/creds.txt'), + ('repo/creds.txt', 'repo/creds.txt'), + ('', ''), + # a colon is a legal character in a posix file name and must not be read as a drive + ('x:y/creds.txt', 'x:y/creds.txt'), + ], +) +def test_normalize_file_path_on_posix(path: str, expected: str) -> None: + assert normalize_file_path(path) == expected + + +@pytest.mark.parametrize( + ('path', 'expected'), + [ + ('C:\\repo\\creds.txt', 'repo/creds.txt'), + ('C:/repo/creds.txt', 'repo/creds.txt'), + ('.\\repo\\creds.txt', 'repo/creds.txt'), + ('/repo/creds.txt', 'repo/creds.txt'), + # the server and the share are kept, so two shares do not normalize onto the same path + ('\\\\server\\share\\creds.txt', 'server/share/creds.txt'), + ('\\\\server\\other\\creds.txt', 'server/other/creds.txt'), + ], +) +def test_normalize_file_path_on_windows(path: str, expected: str) -> None: + assert str(PurePosixPath(*_to_relative_posix_parts(path, OperatingSystem.WINDOWS))) == expected diff --git a/tests/cli/utils/test_path_utils_normalize.py b/tests/cli/utils/test_path_utils_normalize.py deleted file mode 100644 index a04007ed..00000000 --- a/tests/cli/utils/test_path_utils_normalize.py +++ /dev/null @@ -1,19 +0,0 @@ -import pytest - -from cycode.cli.utils.path_utils import normalize_file_path - - -@pytest.mark.parametrize( - ('path', 'expected'), - [ - ('/repo/creds.txt', 'repo/creds.txt'), - ('./repo/creds.txt', 'repo/creds.txt'), - ('repo/creds.txt', 'repo/creds.txt'), - ('C:\\repo\\creds.txt', 'repo\\creds.txt'), - ('C:/repo/creds.txt', 'repo/creds.txt'), - ('.\\repo\\creds.txt', 'repo\\creds.txt'), - ('\\\\server\\share\\creds.txt', 'creds.txt'), - ], -) -def test_normalize_file_path(path: str, expected: str) -> None: - assert normalize_file_path(path) == expected diff --git a/tests/test_zip_file.py b/tests/test_zip_file.py deleted file mode 100644 index b96f981d..00000000 --- a/tests/test_zip_file.py +++ /dev/null @@ -1,38 +0,0 @@ -import os - -from cycode.cli.utils.path_utils import concat_unique_id - - -def test_concat_unique_id_to_file_with_leading_slash() -> None: - filename = os.path.join('path', 'to', 'file') # we should care about slash characters in tests - unique_id = 'unique_id' - - expected_path = os.path.join(unique_id, filename) - - filename = os.sep + filename - assert concat_unique_id(filename, unique_id) == expected_path - - -def test_concat_unique_id_to_file_without_leading_slash() -> None: - filename = os.path.join('path', 'to', 'file') # we should care about slash characters in tests - unique_id = 'unique_id' - - expected_path = os.path.join(unique_id, *filename.split('/')) - - assert concat_unique_id(filename, unique_id) == expected_path - - -def test_concat_unique_id_keeps_unique_id_for_windows_drive_path() -> None: - # os.path.join drops the prefix when the file name is absolute; on Windows a drive letter makes it absolute, - # which used to archive the file as 'C:/repo/file' and the server then reported 'C:' as the commit id - unique_id = 'a' * 40 - - # separators inside the file name are kept as-is; only the drive and the leading separator go away - assert concat_unique_id('C:\\repo\\creds.txt', unique_id) == os.path.join(unique_id, 'repo\\creds.txt') - assert concat_unique_id('C:/repo/creds.txt', unique_id) == os.path.join(unique_id, 'repo/creds.txt') - - -def test_concat_unique_id_keeps_unique_id_for_unc_path() -> None: - unique_id = 'a' * 40 - - assert concat_unique_id('\\\\server\\share\\creds.txt', unique_id) == os.path.join(unique_id, 'creds.txt') From 65b0ea103453b50e6dcda831d7083d1949d15dee Mon Sep 17 00:00:00 2001 From: waleed Date: Thu, 1 Oct 2026 16:09:03 +0300 Subject: [PATCH 3/3] Run the POSIX colon-name test with an explicit flavor On Windows, normalize_file_path uses the Windows flavor, where 'x:y/creds.txt' is a drive-relative path and the drive is correctly stripped. Pass the Linux flavor explicitly so the case asserts the same thing on every CI runner. Co-Authored-By: Claude Opus 5.5 --- tests/cli/utils/test_path_utils.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/cli/utils/test_path_utils.py b/tests/cli/utils/test_path_utils.py index d49c878c..52a50684 100644 --- a/tests/cli/utils/test_path_utils.py +++ b/tests/cli/utils/test_path_utils.py @@ -38,14 +38,17 @@ def test_concat_unique_id_keeps_server_and_share_for_unc_path() -> None: ('./repo/creds.txt', 'repo/creds.txt'), ('repo/creds.txt', 'repo/creds.txt'), ('', ''), - # a colon is a legal character in a posix file name and must not be read as a drive - ('x:y/creds.txt', 'x:y/creds.txt'), ], ) -def test_normalize_file_path_on_posix(path: str, expected: str) -> None: +def test_normalize_file_path(path: str, expected: str) -> None: assert normalize_file_path(path) == expected +def test_normalize_file_path_keeps_colon_in_posix_name() -> None: + # a colon is a legal character in a posix file name and must not be read as a drive + assert str(PurePosixPath(*_to_relative_posix_parts('x:y/creds.txt', OperatingSystem.LINUX))) == 'x:y/creds.txt' + + @pytest.mark.parametrize( ('path', 'expected'), [