From 81b84eff24371a2964288d3516a057734e280df9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Mar 2026 12:23:36 +0000 Subject: [PATCH 1/5] Initial plan From b0e22b930d1a2345c9b520eead09ab52a669573b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Mar 2026 12:35:36 +0000 Subject: [PATCH 2/5] feat: add portable_relpath() utility and migrate all call sites Create src/apm_cli/utils/paths.py with portable_relpath() that centralizes the resolve-then-relativise-then-posixify pattern for Windows-safe paths. Migrate ~23 call sites across integrators, compilation modules, security gate, and uninstall engine to use portable_relpath() or .resolve() on both sides of relative_to(). Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> Agent-Logs-Url: https://github.com/microsoft/apm/sessions/8e610a23-cce1-4f59-ab9f-0f14fd108721 --- src/apm_cli/commands/uninstall/engine.py | 5 +-- src/apm_cli/compilation/agents_compiler.py | 33 ++++++------------- src/apm_cli/compilation/claude_formatter.py | 3 +- src/apm_cli/compilation/context_optimizer.py | 23 ++++++------- .../compilation/distributed_compiler.py | 15 +++++---- src/apm_cli/compilation/template_builder.py | 3 +- src/apm_cli/integration/agent_integrator.py | 13 ++++---- src/apm_cli/integration/command_integrator.py | 5 +-- src/apm_cli/integration/hook_integrator.py | 3 +- .../integration/instruction_integrator.py | 5 +-- src/apm_cli/integration/prompt_integrator.py | 3 +- src/apm_cli/security/gate.py | 3 +- src/apm_cli/utils/__init__.py | 3 ++ src/apm_cli/utils/paths.py | 23 +++++++++++++ 14 files changed, 82 insertions(+), 58 deletions(-) create mode 100644 src/apm_cli/utils/paths.py diff --git a/src/apm_cli/commands/uninstall/engine.py b/src/apm_cli/commands/uninstall/engine.py index a12aed3c..8f36994c 100644 --- a/src/apm_cli/commands/uninstall/engine.py +++ b/src/apm_cli/commands/uninstall/engine.py @@ -6,6 +6,7 @@ from ...constants import APM_MODULES_DIR, APM_YML_FILENAME from ...core.command_logger import CommandLogger from ...utils.path_security import PathTraversalError, safe_rmtree +from ...utils.paths import portable_relpath from ...deps.lockfile import LockFile from ...models.apm_package import APMPackage, DependencyReference @@ -132,7 +133,7 @@ def _remove_packages_from_disk(packages_to_remove, apm_modules_dir, logger): try: safe_rmtree(package_path, apm_modules_dir) logger.progress(f"Removed {package} from apm_modules/") - logger.verbose_detail(f" Path: {package_path.relative_to(apm_modules_dir)}") + logger.verbose_detail(f" Path: {portable_relpath(package_path, apm_modules_dir)}") removed += 1 deleted_pkg_paths.append(package_path) except Exception as e: @@ -213,7 +214,7 @@ def _cleanup_transitive_orphans(lockfile, packages_to_remove, apm_modules_dir, a try: safe_rmtree(orphan_path, apm_modules_dir) logger.progress(f"Removed transitive dependency {orphan_key} from apm_modules/") - logger.verbose_detail(f" Path: {orphan_path.relative_to(apm_modules_dir)}") + logger.verbose_detail(f" Path: {portable_relpath(orphan_path, apm_modules_dir)}") removed += 1 deleted_orphan_paths.append(orphan_path) except Exception as e: diff --git a/src/apm_cli/compilation/agents_compiler.py b/src/apm_cli/compilation/agents_compiler.py index 0df0ec41..772ad2f0 100644 --- a/src/apm_cli/compilation/agents_compiler.py +++ b/src/apm_cli/compilation/agents_compiler.py @@ -19,6 +19,7 @@ find_chatmode_by_name ) from .link_resolver import resolve_markdown_links, validate_link_targets +from ..utils.paths import portable_relpath @dataclass @@ -439,10 +440,7 @@ def _compile_claude_md(self, config: CompilationConfig, primitives: PrimitiveCol f"CLAUDE.md Preview: Would generate {len(claude_result.placements)} files" ] for claude_path in claude_result.content_map.keys(): - try: - rel_path = claude_path.relative_to(self.base_dir) - except ValueError: - rel_path = claude_path + rel_path = portable_relpath(claude_path, self.base_dir) preview_lines.append(f" {rel_path}") return CompilationResult( @@ -535,10 +533,7 @@ def _compile_claude_md(self, config: CompilationConfig, primitives: PrimitiveCol f"Generated {files_written} CLAUDE.md files:" ] for placement in claude_result.placements: - try: - rel_path = placement.claude_path.relative_to(self.base_dir) - except ValueError: - rel_path = placement.claude_path + rel_path = portable_relpath(placement.claude_path, self.base_dir) summary_lines.append(f"- {rel_path} ({len(placement.instructions)} instructions)") return CompilationResult( @@ -629,12 +624,7 @@ def validate_primitives(self, primitives: PrimitiveCollection) -> List[str]: for primitive in primitives.all_primitives(): primitive_errors = primitive.validate() if primitive_errors: - try: - # Try to get relative path, but fall back to absolute if it fails - file_path = str(primitive.file_path.relative_to(self.base_dir)) - except ValueError: - # File is outside base_dir, use absolute path - file_path = str(primitive.file_path) + file_path = portable_relpath(primitive.file_path, self.base_dir) for error in primitive_errors: # Treat validation errors as warnings instead of hard errors @@ -646,10 +636,7 @@ def validate_primitives(self, primitives: PrimitiveCollection) -> List[str]: primitive_dir = primitive.file_path.parent link_errors = validate_link_targets(primitive.content, primitive_dir) if link_errors: - try: - file_path = str(primitive.file_path.relative_to(self.base_dir)) - except ValueError: - file_path = str(primitive.file_path) + file_path = portable_relpath(primitive.file_path, self.base_dir) for link_error in link_errors: self.warnings.append(f"{file_path}: {link_error}") @@ -786,7 +773,7 @@ def _display_placement_preview(self, distributed_result) -> None: for placement in distributed_result.placements: try: - rel_path = placement.agents_path.relative_to(self.base_dir.resolve()) + rel_path = portable_relpath(placement.agents_path, self.base_dir) except ValueError: # Fallback for path resolution issues rel_path = placement.agents_path @@ -810,7 +797,7 @@ def _display_trace_info(self, distributed_result, primitives: PrimitiveCollectio for placement in distributed_result.placements: try: - rel_path = placement.agents_path.relative_to(self.base_dir.resolve()) + rel_path = portable_relpath(placement.agents_path, self.base_dir) except ValueError: rel_path = placement.agents_path self._log("verbose_detail", f"{rel_path}") @@ -818,7 +805,7 @@ def _display_trace_info(self, distributed_result, primitives: PrimitiveCollectio for instruction in placement.instructions: source = getattr(instruction, 'source', 'local') try: - inst_path = instruction.file_path.relative_to(self.base_dir.resolve()) + inst_path = portable_relpath(instruction.file_path, self.base_dir) except ValueError: inst_path = instruction.file_path @@ -838,7 +825,7 @@ def _generate_placement_summary(self, distributed_result) -> str: for placement in distributed_result.placements: try: - rel_path = placement.agents_path.resolve().relative_to(self.base_dir.resolve()).as_posix() + rel_path = portable_relpath(placement.agents_path, self.base_dir) except ValueError: rel_path = str(placement.agents_path) lines.append(f"{rel_path}") @@ -868,7 +855,7 @@ def _generate_distributed_summary(self, distributed_result, config: CompilationC for placement in distributed_result.placements: try: - rel_path = placement.agents_path.resolve().relative_to(self.base_dir.resolve()).as_posix() + rel_path = portable_relpath(placement.agents_path, self.base_dir) except ValueError: rel_path = str(placement.agents_path) lines.append(f"- {rel_path} ({len(placement.instructions)} instructions)") diff --git a/src/apm_cli/compilation/claude_formatter.py b/src/apm_cli/compilation/claude_formatter.py index 4c2f7079..52a739c2 100644 --- a/src/apm_cli/compilation/claude_formatter.py +++ b/src/apm_cli/compilation/claude_formatter.py @@ -18,6 +18,7 @@ from ..primitives.models import Instruction, PrimitiveCollection, Chatmode from ..version import get_version +from ..utils.paths import portable_relpath from .constants import BUILD_ID_PLACEHOLDER from .constitution import read_constitution @@ -299,7 +300,7 @@ def _generate_claude_content( str(instruction.file_path), 'local' ) try: - rel_path = instruction.file_path.relative_to(self.base_dir) + rel_path = portable_relpath(instruction.file_path, self.base_dir) except ValueError: rel_path = instruction.file_path diff --git a/src/apm_cli/compilation/context_optimizer.py b/src/apm_cli/compilation/context_optimizer.py index 2e827634..53acf043 100644 --- a/src/apm_cli/compilation/context_optimizer.py +++ b/src/apm_cli/compilation/context_optimizer.py @@ -21,6 +21,7 @@ CompilationResults, ProjectAnalysis, OptimizationDecision, OptimizationStats, PlacementStrategy, PlacementSummary ) +from ..utils.paths import portable_relpath # CRITICAL: Shadow Click commands to prevent namespace collision # When this module is imported during 'apm compile', Click's active context @@ -349,7 +350,7 @@ def get_compilation_results( instruction_patterns_detected=len(self._optimization_decisions), max_depth=max((a.depth for a in self._directory_cache.values()), default=0), constitution_detected=constitution_detected, - constitution_path=str(constitution_path.relative_to(self.base_dir)) if constitution_detected else None + constitution_path=portable_relpath(constitution_path, self.base_dir) if constitution_detected else None ) # Create placement summaries @@ -421,7 +422,7 @@ def _analyze_project_structure(self) -> None: # Calculate depth for analysis try: - relative_path = current_path.relative_to(self.base_dir) + relative_path = current_path.resolve().relative_to(self.base_dir.resolve()) depth = len(relative_path.parts) except ValueError: depth = 0 @@ -512,7 +513,7 @@ def _should_exclude_path(self, path: Path) -> bool: except (OSError, FileNotFoundError): resolved = path.absolute() try: - rel_path = resolved.relative_to(self.base_dir) + rel_path = resolved.relative_to(self.base_dir.resolve()) except ValueError: # Path is not relative to base_dir, don't exclude return False @@ -655,8 +656,8 @@ def _solve_placement_optimization( if intended_dir: # Place in the intended directory (e.g., docs/ for docs/**/*.md) placement = intended_dir - reasoning = f"No matching files found, placed in intended directory '{intended_dir.relative_to(self.base_dir)}'" - self._warnings.append(f"Pattern '{pattern}' matches no files - placing in intended directory '{intended_dir.relative_to(self.base_dir)}'") + reasoning = f"No matching files found, placed in intended directory '{portable_relpath(intended_dir, self.base_dir)}'" + self._warnings.append(f"Pattern '{pattern}' matches no files - placing in intended directory '{portable_relpath(intended_dir, self.base_dir)}'") else: # Fallback to root for global patterns placement = self.base_dir @@ -796,7 +797,7 @@ def _file_matches_pattern(self, file_path: Path, pattern: str) -> bool: try: # Resolve both paths to handle symlinks and path inconsistencies resolved_file = file_path.resolve() - rel_path = resolved_file.relative_to(self.base_dir) + rel_path = resolved_file.relative_to(self.base_dir.resolve()) # Use cached glob results instead of repeated glob calls matches = self._cached_glob(expanded_pattern) @@ -812,7 +813,7 @@ def _file_matches_pattern(self, file_path: Path, pattern: str) -> bool: try: # Resolve both paths to handle symlinks and path inconsistencies resolved_file = file_path.resolve() - rel_path = resolved_file.relative_to(self.base_dir) + rel_path = resolved_file.relative_to(self.base_dir.resolve()) if fnmatch.fnmatch(str(rel_path), expanded_pattern): return True except ValueError: @@ -1112,7 +1113,7 @@ def _find_minimal_coverage_placement(self, matching_directories: Set[Path]) -> O return None # Convert to relative paths for easier analysis - relative_dirs = [d.relative_to(self.base_dir) for d in matching_directories] + relative_dirs = [d.resolve().relative_to(self.base_dir.resolve()) for d in matching_directories] # Find the lowest common ancestor that covers all directories if len(relative_dirs) == 1: @@ -1166,7 +1167,7 @@ def _is_hierarchically_covered(self, target_dir: Path, placement_dir: Path) -> b """ try: # Check if target is the same as placement or is a subdirectory of placement - target_dir.relative_to(placement_dir) + target_dir.resolve().relative_to(placement_dir.resolve()) return True except ValueError: # target_dir is not under placement_dir @@ -1286,8 +1287,8 @@ def _is_child_directory(self, child: Path, parent: Path) -> bool: bool: True if child is subdirectory of parent. """ try: - child.relative_to(parent) - return child != parent + child.resolve().relative_to(parent.resolve()) + return child.resolve() != parent.resolve() except ValueError: return False diff --git a/src/apm_cli/compilation/distributed_compiler.py b/src/apm_cli/compilation/distributed_compiler.py index 686e92e6..30991a95 100644 --- a/src/apm_cli/compilation/distributed_compiler.py +++ b/src/apm_cli/compilation/distributed_compiler.py @@ -20,6 +20,7 @@ from .link_resolver import UnifiedLinkResolver from ..output.formatters import CompilationFormatter from ..output.models import CompilationResults +from ..utils.paths import portable_relpath # CRITICAL: Shadow Click commands to prevent namespace collision set = builtins.set @@ -241,7 +242,7 @@ def analyze_directory_structure(self, instructions: List[Instruction]) -> Direct directories[abs_dir].add(pattern) # Calculate depth and parent relationships - depth = len(abs_dir.relative_to(self.base_dir).parts) + depth = len(abs_dir.resolve().relative_to(self.base_dir.resolve()).parts) depth_map[abs_dir] = depth if depth > 0: @@ -542,7 +543,7 @@ def _generate_agents_content( if placement.source_attribution: source = placement.source_attribution.get(str(instruction.file_path), 'local') try: - rel_path = instruction.file_path.relative_to(self.base_dir) + rel_path = portable_relpath(instruction.file_path, self.base_dir) except ValueError: rel_path = instruction.file_path @@ -612,7 +613,7 @@ def _find_orphaned_agents_files(self, generated_paths: List[Path]) -> List[Path] for agents_file in self.base_dir.rglob("AGENTS.md"): # Skip files that are outside our project or in special directories try: - relative_path = agents_file.relative_to(self.base_dir) + relative_path = agents_file.resolve().relative_to(self.base_dir.resolve()) # Skip files in certain directories that shouldn't be cleaned skip_dirs = {".git", ".apm", "node_modules", "__pycache__", ".pytest_cache", "apm_modules"} @@ -645,13 +646,13 @@ def _generate_orphan_warnings(self, orphaned_files: List[Path]) -> List[str]: # Professional warning format with readable list for multiple files if len(orphaned_files) == 1: - rel_path = orphaned_files[0].relative_to(self.base_dir) + rel_path = portable_relpath(orphaned_files[0], self.base_dir) warning_messages.append(f"Orphaned AGENTS.md found: {rel_path} - run 'apm compile --clean' to remove") else: # For multiple files, create a single multi-line warning message file_list = [] for file_path in orphaned_files[:5]: # Show first 5 - rel_path = file_path.relative_to(self.base_dir) + rel_path = portable_relpath(file_path, self.base_dir) file_list.append(f" * {rel_path}") if len(orphaned_files) > 5: file_list.append(f" * ...and {len(orphaned_files) - 5} more") @@ -681,14 +682,14 @@ def _cleanup_orphaned_files(self, orphaned_files: List[Path], dry_run: bool = Fa # In dry-run mode, just report what would be cleaned cleanup_messages.append(f"Would clean up {len(orphaned_files)} orphaned AGENTS.md files") for file_path in orphaned_files: - rel_path = file_path.relative_to(self.base_dir) + rel_path = portable_relpath(file_path, self.base_dir) cleanup_messages.append(f" * {rel_path}") else: # Actually perform the cleanup cleanup_messages.append(f"Cleaning up {len(orphaned_files)} orphaned AGENTS.md files") for file_path in orphaned_files: try: - rel_path = file_path.relative_to(self.base_dir) + rel_path = portable_relpath(file_path, self.base_dir) file_path.unlink() cleanup_messages.append(f" + Removed {rel_path}") except Exception as e: diff --git a/src/apm_cli/compilation/template_builder.py b/src/apm_cli/compilation/template_builder.py index 8aa20267..b9049140 100644 --- a/src/apm_cli/compilation/template_builder.py +++ b/src/apm_cli/compilation/template_builder.py @@ -5,6 +5,7 @@ from pathlib import Path from typing import List, Dict, Optional, Tuple from ..primitives.models import Instruction, Chatmode +from ..utils.paths import portable_relpath @dataclass @@ -45,7 +46,7 @@ def build_conditional_sections(instructions: List[Instruction]) -> str: try: # Try to get relative path for cleaner display if instruction.file_path.is_absolute(): - relative_path = instruction.file_path.relative_to(Path.cwd()) + relative_path = portable_relpath(instruction.file_path, Path.cwd()) else: relative_path = instruction.file_path except (ValueError, OSError): diff --git a/src/apm_cli/integration/agent_integrator.py b/src/apm_cli/integration/agent_integrator.py index a98eafc1..fa6c86da 100644 --- a/src/apm_cli/integration/agent_integrator.py +++ b/src/apm_cli/integration/agent_integrator.py @@ -9,6 +9,7 @@ from typing import List, Dict from apm_cli.integration.base_integrator import BaseIntegrator, IntegrationResult +from apm_cli.utils.paths import portable_relpath class AgentIntegrator(BaseIntegrator): @@ -161,7 +162,7 @@ def integrate_package_agents(self, package_info, project_root: Path, for source_file in agent_files: target_filename = self.get_target_filename(source_file, package_info.package.name) target_path = agents_dir / target_filename - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 @@ -176,7 +177,7 @@ def integrate_package_agents(self, package_info, project_root: Path, if claude_agents_dir: claude_filename = self.get_target_filename_claude(source_file, package_info.package.name) claude_target = claude_agents_dir / claude_filename - claude_rel = str(claude_target.relative_to(project_root)) + claude_rel = portable_relpath(claude_target, project_root) if not self.check_collision(claude_target, claude_rel, managed_files, force, diagnostics=diagnostics): self.copy_agent(source_file, claude_target) target_paths.append(claude_target) @@ -185,7 +186,7 @@ def integrate_package_agents(self, package_info, project_root: Path, if cursor_agents_dir: cursor_filename = self.get_target_filename_cursor(source_file, package_info.package.name) cursor_target = cursor_agents_dir / cursor_filename - cursor_rel = str(cursor_target.relative_to(project_root)) + cursor_rel = portable_relpath(cursor_target, project_root) if not self.check_collision(cursor_target, cursor_rel, managed_files, force, diagnostics=diagnostics): self.copy_agent(source_file, cursor_target) target_paths.append(cursor_target) @@ -263,7 +264,7 @@ def integrate_package_agents_claude(self, package_info, project_root: Path, for source_file in agent_files: target_filename = self.get_target_filename_claude(source_file, package_info.package.name) target_path = agents_dir / target_filename - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 @@ -382,7 +383,7 @@ def integrate_package_agents_cursor(self, package_info, project_root: Path, for source_file in agent_files: target_filename = self.get_target_filename_cursor(source_file, package_info.package.name) target_path = agents_dir / target_filename - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 @@ -452,7 +453,7 @@ def integrate_package_agents_opencode(self, package_info, project_root: Path, source_file, package_info.package.name ) target_path = agents_dir / target_filename - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 diff --git a/src/apm_cli/integration/command_integrator.py b/src/apm_cli/integration/command_integrator.py index 6cac15c6..883f7ef5 100644 --- a/src/apm_cli/integration/command_integrator.py +++ b/src/apm_cli/integration/command_integrator.py @@ -9,6 +9,7 @@ import frontmatter from apm_cli.integration.base_integrator import BaseIntegrator, IntegrationResult +from apm_cli.utils.paths import portable_relpath # Re-export for backward compat (tests import CommandIntegrationResult) CommandIntegrationResult = IntegrationResult @@ -136,7 +137,7 @@ def integrate_package_commands(self, package_info, project_root: Path, base_name = prompt_file.stem target_path = commands_dir / f"{base_name}.md" - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 @@ -224,7 +225,7 @@ def integrate_package_commands_opencode(self, package_info, project_root: Path, base_name = prompt_file.stem target_path = commands_dir / f"{base_name}.md" - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 0ace6ade..2d122c27 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -50,6 +50,7 @@ from dataclasses import dataclass, field from apm_cli.integration.base_integrator import BaseIntegrator +from apm_cli.utils.paths import portable_relpath @dataclass @@ -313,7 +314,7 @@ def integrate_package_hooks(self, package_info, project_root: Path, stem = hook_file.stem target_filename = f"{package_name}-{stem}.json" target_path = hooks_dir / target_filename - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): continue diff --git a/src/apm_cli/integration/instruction_integrator.py b/src/apm_cli/integration/instruction_integrator.py index fb1166f4..2d6f36c1 100644 --- a/src/apm_cli/integration/instruction_integrator.py +++ b/src/apm_cli/integration/instruction_integrator.py @@ -12,6 +12,7 @@ from typing import Dict, List, Optional, Set from apm_cli.integration.base_integrator import BaseIntegrator, IntegrationResult +from apm_cli.utils.paths import portable_relpath class InstructionIntegrator(BaseIntegrator): @@ -78,7 +79,7 @@ def integrate_package_instructions( for source_file in instruction_files: target_path = instructions_dir / source_file.name - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 @@ -227,7 +228,7 @@ def integrate_package_instructions_cursor( mdc_name = f"{stem}.mdc" target_path = rules_dir / mdc_name - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 diff --git a/src/apm_cli/integration/prompt_integrator.py b/src/apm_cli/integration/prompt_integrator.py index 0d33ff58..444831df 100644 --- a/src/apm_cli/integration/prompt_integrator.py +++ b/src/apm_cli/integration/prompt_integrator.py @@ -4,6 +4,7 @@ from typing import List, Dict from apm_cli.integration.base_integrator import BaseIntegrator, IntegrationResult +from apm_cli.utils.paths import portable_relpath class PromptIntegrator(BaseIntegrator): @@ -111,7 +112,7 @@ def integrate_package_prompts(self, package_info, project_root: Path, for source_file in prompt_files: target_filename = self.get_target_filename(source_file, package_info.package.name) target_path = prompts_dir / target_filename - rel_path = str(target_path.relative_to(project_root)) + rel_path = portable_relpath(target_path, project_root) if self.check_collision(target_path, rel_path, managed_files, force, diagnostics=diagnostics): files_skipped += 1 diff --git a/src/apm_cli/security/gate.py b/src/apm_cli/security/gate.py index ee71663d..e8ac9f96 100644 --- a/src/apm_cli/security/gate.py +++ b/src/apm_cli/security/gate.py @@ -13,6 +13,7 @@ from typing import Dict, List, Literal, Optional from .content_scanner import ContentScanner, ScanFinding +from ..utils.paths import portable_relpath # --------------------------------------------------------------------------- @@ -102,7 +103,7 @@ def scan_files( except OSError: continue if file_findings: - rel = str(fpath.relative_to(root)) + rel = portable_relpath(fpath, root) findings_by_file[rel] = file_findings return SecurityGate._build_verdict( diff --git a/src/apm_cli/utils/__init__.py b/src/apm_cli/utils/__init__.py index dd6df7e8..d397b8d1 100644 --- a/src/apm_cli/utils/__init__.py +++ b/src/apm_cli/utils/__init__.py @@ -21,6 +21,8 @@ CATEGORY_ERROR, ) +from .paths import portable_relpath + __all__ = [ '_rich_success', '_rich_error', @@ -37,4 +39,5 @@ 'CATEGORY_OVERWRITE', 'CATEGORY_WARNING', 'CATEGORY_ERROR', + 'portable_relpath', ] \ No newline at end of file diff --git a/src/apm_cli/utils/paths.py b/src/apm_cli/utils/paths.py new file mode 100644 index 00000000..19313b68 --- /dev/null +++ b/src/apm_cli/utils/paths.py @@ -0,0 +1,23 @@ +"""Cross-platform path utilities for APM CLI. + +Centralises the resolve-then-relativise-then-posixify pattern so every +call site gets Windows-safe, forward-slash relative paths by default. +""" + +from __future__ import annotations + +from pathlib import Path + + +def portable_relpath(path: Path, base: Path) -> str: + """Return a forward-slash relative path, resolving both sides first. + + Handles Windows 8.3 short names (e.g. ``RUNNER~1`` vs ``runneradmin``) + and ensures consistent POSIX output on every platform. + + Falls back to ``str(path)`` when *path* is not under *base*. + """ + try: + return path.resolve().relative_to(base.resolve()).as_posix() + except ValueError: + return str(path) From 6dca0fcadc6fb70e7df21a1c49d55f15a0350004 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Mar 2026 12:37:39 +0000 Subject: [PATCH 3/5] ci: add lint guard for raw str(relative_to) patterns; update CHANGELOG Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> Agent-Logs-Url: https://github.com/microsoft/apm/sessions/8e610a23-cce1-4f59-ab9f-0f14fd108721 --- .github/workflows/ci.yml | 8 +++ CHANGELOG.md | 3 + tests/unit/test_portable_relpath.py | 98 +++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+) create mode 100644 tests/unit/test_portable_relpath.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 05b2e203..f71e142e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,6 +42,14 @@ jobs: - name: Run tests run: uv run pytest tests/unit tests/test_console.py -n auto --dist worksteal + - name: Lint - no raw str(relative_to) patterns + run: | + # Fail if any code uses str(x.relative_to(y)) instead of portable_relpath() + if grep -rn 'str(.*\.relative_to(' src/apm_cli/ --include="*.py" | grep -v portable_relpath | grep -v '\.pyc'; then + echo "::error::Found raw str(path.relative_to()) calls. Use portable_relpath() from apm_cli.utils.paths instead." + exit 1 + fi + - name: Install UPX run: | sudo apt-get update diff --git a/CHANGELOG.md b/CHANGELOG.md index bf44c322..ad1a2a6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Systematic Windows path compatibility hardening: added `portable_relpath()` utility and migrated ~23 `relative_to()` call sites to use `.resolve()` on both sides + `.as_posix()` output, preventing `ValueError` on Windows 8.3 short names and backslash-contaminated path strings (#419) - Virtual package types (files, collections, subdirectories) now respect `ARTIFACTORY_ONLY=1`, matching the primary zip-archive proxy-only behavior (#418) ### Added +- `portable_relpath()` helper in `apm_cli.utils.paths` centralises the resolve-relativise-posixify pattern for cross-platform path safety (#419) +- CI lint guard (`grep`-based) blocks raw `str(path.relative_to())` regressions (#419) - `ci-runtime.yml` workflow for nightly + manual runtime inference tests, decoupled from release pipeline (#371) - `APM_RUN_INFERENCE_TESTS` env var to gate live inference (`apm run`) in test scripts (#371) - PR traceability `::notice` annotation in `ci-integration.yml` smoke-test job (#371) diff --git a/tests/unit/test_portable_relpath.py b/tests/unit/test_portable_relpath.py new file mode 100644 index 00000000..e1ae1fbb --- /dev/null +++ b/tests/unit/test_portable_relpath.py @@ -0,0 +1,98 @@ +"""Unit tests for apm_cli.utils.paths.portable_relpath(). + +Covers: +- Basic relative path computation with forward slashes +- .resolve() on both sides (handles Windows 8.3 short-name mismatches) +- Fallback to str(path) when path is not under base +- Edge cases: same directory, deeply nested, trailing slashes +""" + +from pathlib import Path + +import pytest + +from apm_cli.utils.paths import portable_relpath + + +class TestPortableRelpath: + """Tests for portable_relpath().""" + + def test_simple_relative(self, tmp_path): + """Basic child path returns forward-slash relative string.""" + child = tmp_path / "sub" / "file.md" + child.parent.mkdir(parents=True, exist_ok=True) + child.touch() + result = portable_relpath(child, tmp_path) + assert result == "sub/file.md" + + def test_deeply_nested(self, tmp_path): + """Deeply nested paths use forward slashes throughout.""" + child = tmp_path / "a" / "b" / "c" / "d.txt" + child.parent.mkdir(parents=True, exist_ok=True) + child.touch() + result = portable_relpath(child, tmp_path) + assert result == "a/b/c/d.txt" + + def test_same_directory(self, tmp_path): + """Path equal to base returns '.'.""" + result = portable_relpath(tmp_path, tmp_path) + assert result == "." + + def test_file_in_base(self, tmp_path): + """File directly in base returns just the filename.""" + child = tmp_path / "README.md" + child.touch() + result = portable_relpath(child, tmp_path) + assert result == "README.md" + + def test_fallback_when_not_under_base(self, tmp_path): + """Returns str(path) when path is not under base.""" + other = tmp_path / "other" + other.mkdir() + base = tmp_path / "base" + base.mkdir() + result = portable_relpath(other, base) + # Should fall back to str(path) — which may vary by platform, + # but must not raise an exception + assert isinstance(result, str) + + def test_result_never_contains_backslash(self, tmp_path): + """Returned string never contains backslashes (Windows safety).""" + child = tmp_path / "src" / "apm_cli" / "utils" / "paths.py" + child.parent.mkdir(parents=True, exist_ok=True) + child.touch() + result = portable_relpath(child, tmp_path) + assert "\\" not in result + assert result == "src/apm_cli/utils/paths.py" + + def test_resolve_handles_symlinks(self, tmp_path): + """Symlinked paths resolve to the same result.""" + real_dir = tmp_path / "real" + real_dir.mkdir() + child = real_dir / "file.txt" + child.touch() + + link_dir = tmp_path / "link" + try: + link_dir.symlink_to(real_dir) + except OSError: + pytest.skip("symlinks not supported on this platform") + + linked_child = link_dir / "file.txt" + # Both should resolve to the same thing + result_real = portable_relpath(child, tmp_path) + result_link = portable_relpath(linked_child, tmp_path) + assert result_real == result_link + + def test_nonexistent_path_still_works(self, tmp_path): + """Works even if the path doesn't exist on disk (resolve still works).""" + fake = tmp_path / "does" / "not" / "exist.md" + result = portable_relpath(fake, tmp_path) + assert result == "does/not/exist.md" + + def test_return_type_is_str(self, tmp_path): + """Always returns a str, never a Path.""" + child = tmp_path / "file.txt" + child.touch() + result = portable_relpath(child, tmp_path) + assert isinstance(result, str) From 55ad6fb5e5f7a471b9ba80fcbf076d03c0ad7502 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 23 Mar 2026 12:41:03 +0000 Subject: [PATCH 4/5] refactor: address code review - remove redundant try/except, fix type consistency, improve CI lint pattern Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> Agent-Logs-Url: https://github.com/microsoft/apm/sessions/8e610a23-cce1-4f59-ab9f-0f14fd108721 --- .github/workflows/ci.yml | 2 +- src/apm_cli/compilation/agents_compiler.py | 26 ++++--------------- src/apm_cli/compilation/claude_formatter.py | 5 +--- src/apm_cli/compilation/context_optimizer.py | 6 ++--- .../compilation/distributed_compiler.py | 5 +--- src/apm_cli/compilation/template_builder.py | 2 +- tests/unit/test_portable_relpath.py | 5 ++-- 7 files changed, 13 insertions(+), 38 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f71e142e..01a21c56 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,7 +45,7 @@ jobs: - name: Lint - no raw str(relative_to) patterns run: | # Fail if any code uses str(x.relative_to(y)) instead of portable_relpath() - if grep -rn 'str(.*\.relative_to(' src/apm_cli/ --include="*.py" | grep -v portable_relpath | grep -v '\.pyc'; then + if grep -rn --include="*.py" -P 'str\([^)]*\.relative_to\(' src/apm_cli/ | grep -v portable_relpath | grep -v '\.pyc'; then echo "::error::Found raw str(path.relative_to()) calls. Use portable_relpath() from apm_cli.utils.paths instead." exit 1 fi diff --git a/src/apm_cli/compilation/agents_compiler.py b/src/apm_cli/compilation/agents_compiler.py index 772ad2f0..604a766f 100644 --- a/src/apm_cli/compilation/agents_compiler.py +++ b/src/apm_cli/compilation/agents_compiler.py @@ -772,11 +772,7 @@ def _display_placement_preview(self, distributed_result) -> None: self._log("progress", "") for placement in distributed_result.placements: - try: - rel_path = portable_relpath(placement.agents_path, self.base_dir) - except ValueError: - # Fallback for path resolution issues - rel_path = placement.agents_path + rel_path = portable_relpath(placement.agents_path, self.base_dir) self._log("verbose_detail", f"{rel_path}") self._log("verbose_detail", f" Instructions: {len(placement.instructions)}") self._log("verbose_detail", f" Patterns: {', '.join(sorted(placement.coverage_patterns))}") @@ -796,18 +792,12 @@ def _display_trace_info(self, distributed_result, primitives: PrimitiveCollectio self._log("progress", "") for placement in distributed_result.placements: - try: - rel_path = portable_relpath(placement.agents_path, self.base_dir) - except ValueError: - rel_path = placement.agents_path + rel_path = portable_relpath(placement.agents_path, self.base_dir) self._log("verbose_detail", f"{rel_path}") for instruction in placement.instructions: source = getattr(instruction, 'source', 'local') - try: - inst_path = portable_relpath(instruction.file_path, self.base_dir) - except ValueError: - inst_path = instruction.file_path + inst_path = portable_relpath(instruction.file_path, self.base_dir) self._log("verbose_detail", f" * {instruction.apply_to or 'no pattern'} <- {source} {inst_path}") self._log("verbose_detail", "") @@ -824,10 +814,7 @@ def _generate_placement_summary(self, distributed_result) -> str: lines = ["Distributed AGENTS.md Placement Summary:", ""] for placement in distributed_result.placements: - try: - rel_path = portable_relpath(placement.agents_path, self.base_dir) - except ValueError: - rel_path = str(placement.agents_path) + rel_path = portable_relpath(placement.agents_path, self.base_dir) lines.append(f"{rel_path}") lines.append(f" Instructions: {len(placement.instructions)}") lines.append(f" Patterns: {', '.join(sorted(placement.coverage_patterns))}") @@ -854,10 +841,7 @@ def _generate_distributed_summary(self, distributed_result, config: CompilationC ] for placement in distributed_result.placements: - try: - rel_path = portable_relpath(placement.agents_path, self.base_dir) - except ValueError: - rel_path = str(placement.agents_path) + rel_path = portable_relpath(placement.agents_path, self.base_dir) lines.append(f"- {rel_path} ({len(placement.instructions)} instructions)") lines.extend([ diff --git a/src/apm_cli/compilation/claude_formatter.py b/src/apm_cli/compilation/claude_formatter.py index 52a739c2..f06e5ef3 100644 --- a/src/apm_cli/compilation/claude_formatter.py +++ b/src/apm_cli/compilation/claude_formatter.py @@ -299,10 +299,7 @@ def _generate_claude_content( source = placement.source_attribution.get( str(instruction.file_path), 'local' ) - try: - rel_path = portable_relpath(instruction.file_path, self.base_dir) - except ValueError: - rel_path = instruction.file_path + rel_path = portable_relpath(instruction.file_path, self.base_dir) sections.append(f"") diff --git a/src/apm_cli/compilation/context_optimizer.py b/src/apm_cli/compilation/context_optimizer.py index 53acf043..4fc4181d 100644 --- a/src/apm_cli/compilation/context_optimizer.py +++ b/src/apm_cli/compilation/context_optimizer.py @@ -811,10 +811,8 @@ def _file_matches_pattern(self, file_path: Path, pattern: str) -> bool: else: # For non-recursive patterns, use fnmatch as before try: - # Resolve both paths to handle symlinks and path inconsistencies - resolved_file = file_path.resolve() - rel_path = resolved_file.relative_to(self.base_dir.resolve()) - if fnmatch.fnmatch(str(rel_path), expanded_pattern): + rel_str = portable_relpath(file_path, self.base_dir) + if fnmatch.fnmatch(rel_str, expanded_pattern): return True except ValueError: pass diff --git a/src/apm_cli/compilation/distributed_compiler.py b/src/apm_cli/compilation/distributed_compiler.py index 30991a95..48c3214b 100644 --- a/src/apm_cli/compilation/distributed_compiler.py +++ b/src/apm_cli/compilation/distributed_compiler.py @@ -542,10 +542,7 @@ def _generate_agents_content( # Add source attribution for individual instructions if placement.source_attribution: source = placement.source_attribution.get(str(instruction.file_path), 'local') - try: - rel_path = portable_relpath(instruction.file_path, self.base_dir) - except ValueError: - rel_path = instruction.file_path + rel_path = portable_relpath(instruction.file_path, self.base_dir) sections.append(f"") diff --git a/src/apm_cli/compilation/template_builder.py b/src/apm_cli/compilation/template_builder.py index b9049140..90962442 100644 --- a/src/apm_cli/compilation/template_builder.py +++ b/src/apm_cli/compilation/template_builder.py @@ -48,7 +48,7 @@ def build_conditional_sections(instructions: List[Instruction]) -> str: if instruction.file_path.is_absolute(): relative_path = portable_relpath(instruction.file_path, Path.cwd()) else: - relative_path = instruction.file_path + relative_path = str(instruction.file_path) except (ValueError, OSError): # Fall back to absolute or given path if relative fails relative_path = instruction.file_path diff --git a/tests/unit/test_portable_relpath.py b/tests/unit/test_portable_relpath.py index e1ae1fbb..82c2ef1b 100644 --- a/tests/unit/test_portable_relpath.py +++ b/tests/unit/test_portable_relpath.py @@ -52,9 +52,8 @@ def test_fallback_when_not_under_base(self, tmp_path): base = tmp_path / "base" base.mkdir() result = portable_relpath(other, base) - # Should fall back to str(path) — which may vary by platform, - # but must not raise an exception - assert isinstance(result, str) + # Should fall back to str(path) — resolved absolute path + assert result == str(other.resolve()) def test_result_never_contains_backslash(self, tmp_path): """Returned string never contains backslashes (Windows safety).""" From 0e35fd429cb35eb545aa9fc764ec683bf2cb7413 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 24 Mar 2026 05:03:15 +0100 Subject: [PATCH 5/5] fix: address PR review comments -- fallback POSIX paths and cleanup - portable_relpath() fallback now returns path.resolve().as_posix() instead of str(path), preventing backslash leaks on Windows - Catch OSError/RuntimeError from resolve() for robustness - template_builder.py except branch uses .as_posix() consistently - Remove unused Path import from test file - Consolidate CHANGELOG entries per one-line-per-PR convention Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 +--- src/apm_cli/compilation/template_builder.py | 2 +- src/apm_cli/utils/paths.py | 10 +++++++--- tests/unit/test_portable_relpath.py | 11 +++++------ 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52ecc646..c7b77ea7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,14 +10,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Systematic Windows path compatibility hardening: added `portable_relpath()` utility and migrated ~23 `relative_to()` call sites to use `.resolve()` on both sides + `.as_posix()` output, preventing `ValueError` on Windows 8.3 short names and backslash-contaminated path strings (#419) +- Systematic Windows path compatibility hardening: added `portable_relpath()` utility, migrated ~23 `relative_to()` call sites to use `.resolve()` on both sides with `.as_posix()` output, and added CI lint guard to block raw `str(path.relative_to())` regressions (#419, #422) - Virtual package types (files, collections, subdirectories) now respect `ARTIFACTORY_ONLY=1`, matching the primary zip-archive proxy-only behavior (#418) - `apm pack --target claude` no longer produces an empty bundle when skills/agents are installed under `.github/` -- cross-target path mapping remaps `skills/` and `agents/` to the pack target prefix (#420) ### Added -- `portable_relpath()` helper in `apm_cli.utils.paths` centralises the resolve-relativise-posixify pattern for cross-platform path safety (#419) -- CI lint guard (`grep`-based) blocks raw `str(path.relative_to())` regressions (#419) - `ci-runtime.yml` workflow for nightly + manual runtime inference tests, decoupled from release pipeline (#371) - `APM_RUN_INFERENCE_TESTS` env var to gate live inference (`apm run`) in test scripts (#371) - PR traceability `::notice` annotation in `ci-integration.yml` smoke-test job (#371) diff --git a/src/apm_cli/compilation/template_builder.py b/src/apm_cli/compilation/template_builder.py index 90962442..e9b945d2 100644 --- a/src/apm_cli/compilation/template_builder.py +++ b/src/apm_cli/compilation/template_builder.py @@ -51,7 +51,7 @@ def build_conditional_sections(instructions: List[Instruction]) -> str: relative_path = str(instruction.file_path) except (ValueError, OSError): # Fall back to absolute or given path if relative fails - relative_path = instruction.file_path + relative_path = instruction.file_path.as_posix() sections.append(f"") sections.append(content) diff --git a/src/apm_cli/utils/paths.py b/src/apm_cli/utils/paths.py index 19313b68..f084bf6c 100644 --- a/src/apm_cli/utils/paths.py +++ b/src/apm_cli/utils/paths.py @@ -15,9 +15,13 @@ def portable_relpath(path: Path, base: Path) -> str: Handles Windows 8.3 short names (e.g. ``RUNNER~1`` vs ``runneradmin``) and ensures consistent POSIX output on every platform. - Falls back to ``str(path)`` when *path* is not under *base*. + When *path* is not under *base* (or resolution fails), falls back to + a resolved absolute POSIX path. """ try: return path.resolve().relative_to(base.resolve()).as_posix() - except ValueError: - return str(path) + except (ValueError, OSError, RuntimeError): + try: + return path.resolve().as_posix() + except (OSError, RuntimeError): + return path.as_posix() diff --git a/tests/unit/test_portable_relpath.py b/tests/unit/test_portable_relpath.py index 82c2ef1b..946a17f5 100644 --- a/tests/unit/test_portable_relpath.py +++ b/tests/unit/test_portable_relpath.py @@ -3,12 +3,10 @@ Covers: - Basic relative path computation with forward slashes - .resolve() on both sides (handles Windows 8.3 short-name mismatches) -- Fallback to str(path) when path is not under base +- Fallback to POSIX path when path is not under base - Edge cases: same directory, deeply nested, trailing slashes """ -from pathlib import Path - import pytest from apm_cli.utils.paths import portable_relpath @@ -46,14 +44,15 @@ def test_file_in_base(self, tmp_path): assert result == "README.md" def test_fallback_when_not_under_base(self, tmp_path): - """Returns str(path) when path is not under base.""" + """Returns POSIX-style resolved path when path is not under base.""" other = tmp_path / "other" other.mkdir() base = tmp_path / "base" base.mkdir() result = portable_relpath(other, base) - # Should fall back to str(path) — resolved absolute path - assert result == str(other.resolve()) + # Should fall back to resolved absolute POSIX path + assert result == other.resolve().as_posix() + assert "\\" not in result def test_result_never_contains_backslash(self, tmp_path): """Returned string never contains backslashes (Windows safety)."""