From b2143ef3d9e3b845b5479ef134d467a2f6e276b4 Mon Sep 17 00:00:00 2001 From: mitnick Date: Fri, 26 Jun 2026 01:46:52 +0000 Subject: [PATCH 1/2] fix(interface): correct workspace-subdir collision and copy-diff classification (#0) --- strix/interface/utils.py | 19 ++++++++++--- tests/test_diff_workspace_utils.py | 43 ++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 tests/test_diff_workspace_utils.py diff --git a/strix/interface/utils.py b/strix/interface/utils.py index bffc0d47..97ec59c3 100644 --- a/strix/interface/utils.py +++ b/strix/interface/utils.py @@ -746,6 +746,7 @@ def _classify_diff_entries(entries: list[DiffEntry]) -> dict[str, Any]: analyzable_files: list[str] = [] analyzable_seen: set[str] = set() modified_seen: set[str] = set() + added_seen: set[str] = set() for entry in entries: path = entry.path @@ -757,7 +758,7 @@ def _classify_diff_entries(entries: list[DiffEntry]) -> dict[str, Any]: continue if entry.status == "A": - added_files.append(path) + _append_unique(added_files, added_seen, path) _append_unique(analyzable_files, analyzable_seen, path) continue @@ -780,7 +781,7 @@ def _classify_diff_entries(entries: list[DiffEntry]) -> dict[str, Any]: continue if entry.status == "C": - _append_unique(modified_files, modified_seen, path) + _append_unique(added_files, added_seen, path) _append_unique(analyzable_files, analyzable_seen, path) continue @@ -1177,10 +1178,20 @@ def assign_workspace_subdirs(targets_info: list[dict[str, Any]]) -> None: continue count = name_counts.get(base_name, 0) + 1 - name_counts[base_name] = count - workspace_subdir = base_name if count == 1 else f"{base_name}-{count}" + # Avoid colliding with a subdir already allocated to another target + # (e.g. base names ["api-2", "api", "api"] would otherwise both map to "api-2"). + max_attempts = len(targets_info) + 1 + attempts = 0 + while workspace_subdir in name_counts and attempts < max_attempts: + count += 1 + workspace_subdir = f"{base_name}-{count}" + attempts += 1 + + name_counts[base_name] = count + name_counts[workspace_subdir] = count + details["workspace_subdir"] = workspace_subdir diff --git a/tests/test_diff_workspace_utils.py b/tests/test_diff_workspace_utils.py new file mode 100644 index 00000000..b089e5c9 --- /dev/null +++ b/tests/test_diff_workspace_utils.py @@ -0,0 +1,43 @@ +"""Tests for diff classification and workspace-subdir assignment in interface.utils.""" + +from __future__ import annotations + +from typing import Any + +from strix.interface.utils import ( + DiffEntry, + _classify_diff_entries, + assign_workspace_subdirs, +) + + +def _local_target(path: str) -> dict[str, Any]: + return {"type": "local_code", "details": {"target_path": path}} + + +def test_assign_workspace_subdirs_avoids_suffix_collision() -> None: + # Base names derive to ["api-2", "api", "api"]; the third target's naive + # suffix "api-2" would collide with the first, silently dropping a repo. + targets = [ + _local_target("/srv/api-2"), + _local_target("/srv/api"), + _local_target("/other/api"), + ] + + assign_workspace_subdirs(targets) + + subdirs = [t["details"]["workspace_subdir"] for t in targets] + assert len(set(subdirs)) == 3 + assert subdirs[0] == "api-2" + assert subdirs[1] == "api" + assert subdirs[2] not in {"api-2", "api"} + + +def test_classify_diff_entries_copy_is_added_not_modified() -> None: + result = _classify_diff_entries( + [DiffEntry(status="C", path="new.py", old_path="orig.py", similarity=100)] + ) + + assert "new.py" in result["added_files"] + assert "new.py" not in result["modified_files"] + assert "new.py" in result["analyzable_files"] From dfecf0343d739b1bd1c6192f8d0fa76094223cdf Mon Sep 17 00:00:00 2001 From: mitnick Date: Sat, 27 Jun 2026 00:13:08 +0000 Subject: [PATCH 2/2] fix(interface): track allocated subdirs separately from base-name counts Storing the derived suffix back into name_counts poisoned the counter of any later target whose base name equalled that suffix, producing surprising (though still unique) workspace names. Use a dedicated allocated_subdirs set for the collision guard. Adds a regression test. Addresses Greptile review. --- strix/interface/utils.py | 7 +++++-- tests/test_diff_workspace_utils.py | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/strix/interface/utils.py b/strix/interface/utils.py index 97ec59c3..0dfb7daa 100644 --- a/strix/interface/utils.py +++ b/strix/interface/utils.py @@ -1163,6 +1163,7 @@ def derive_local_base_name(path_str: str) -> str: def assign_workspace_subdirs(targets_info: list[dict[str, Any]]) -> None: name_counts: dict[str, int] = {} + allocated_subdirs: set[str] = set() for target in targets_info: target_type = target["type"] @@ -1182,15 +1183,17 @@ def assign_workspace_subdirs(targets_info: list[dict[str, Any]]) -> None: # Avoid colliding with a subdir already allocated to another target # (e.g. base names ["api-2", "api", "api"] would otherwise both map to "api-2"). + # Track allocated subdirs separately from base-name counts so a derived + # suffix never poisons the count of a later target sharing that name. max_attempts = len(targets_info) + 1 attempts = 0 - while workspace_subdir in name_counts and attempts < max_attempts: + while workspace_subdir in allocated_subdirs and attempts < max_attempts: count += 1 workspace_subdir = f"{base_name}-{count}" attempts += 1 name_counts[base_name] = count - name_counts[workspace_subdir] = count + allocated_subdirs.add(workspace_subdir) details["workspace_subdir"] = workspace_subdir diff --git a/tests/test_diff_workspace_utils.py b/tests/test_diff_workspace_utils.py index b089e5c9..2074df14 100644 --- a/tests/test_diff_workspace_utils.py +++ b/tests/test_diff_workspace_utils.py @@ -33,6 +33,25 @@ def test_assign_workspace_subdirs_avoids_suffix_collision() -> None: assert subdirs[2] not in {"api-2", "api"} +def test_assign_workspace_subdirs_suffix_does_not_poison_later_base() -> None: + # Base names ["api", "api", "api-2"]: the second "api" target allocates the + # subdir "api-2"; the later real "api-2" target must still get a unique, + # non-colliding subdir without the earlier allocation poisoning its counter. + targets = [ + _local_target("/srv/api"), + _local_target("/other/api"), + _local_target("/srv/api-2"), + ] + + assign_workspace_subdirs(targets) + + subdirs = [t["details"]["workspace_subdir"] for t in targets] + assert len(set(subdirs)) == 3 + assert subdirs[0] == "api" + assert subdirs[1] == "api-2" + assert subdirs[2].startswith("api-2") + + def test_classify_diff_entries_copy_is_added_not_modified() -> None: result = _classify_diff_entries( [DiffEntry(status="C", path="new.py", old_path="orig.py", similarity=100)]