Skip to content

fix: resolve both sides of relative_to() for Windows 8.3 path compat #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danielmeppiel merged 1 commit into from
Mar 22, 2026
Merged

fix: resolve both sides of relative_to() for Windows 8.3 path compat #411

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/apm_cli/compilation/agents_compiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ def _generate_placement_summary(self, distributed_result) -> str:

for placement in distributed_result.placements:
try:
rel_path = placement.agents_path.relative_to(self.base_dir.resolve()).as_posix()
rel_path = placement.agents_path.resolve().relative_to(self.base_dir.resolve()).as_posix()
except ValueError:
Comment on lines 839 to 842
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.base_dir.resolve() is recomputed for every placement. Since Path.resolve() can hit the filesystem and is relatively expensive on Windows, compute the resolved base dir once before the loop (e.g., base_dir_resolved = self.base_dir.resolve()) and reuse it for each relative_to() call.

This issue also appears on line 869 of the same file.

Copilot uses AI. Check for mistakes.
Comment on lines 840 to 842
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR normalizes placement.agents_path via .resolve() in the summary generators, but _display_placement_preview() / _display_trace_info() still call placement.agents_path.relative_to(self.base_dir.resolve()) without resolving the placement path. That means the same Windows 8.3 short-name/case mismatch can still trigger ValueError and force those outputs to fall back to absolute paths. Consider applying the same "resolve both sides" normalization there as well for consistent behavior.

Copilot uses AI. Check for mistakes.
Comment on lines 839 to 842
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are unit tests covering the separator normalization and the "outside base dir" fallback, but nothing that exercises the Windows-specific mismatch this PR targets (where relative_to() fails unless both sides are canonicalized). Please add a regression test that simulates an 8.3/case-mismatch scenario (e.g., by patching Path.resolve() for one side) and asserts the summary contains the expected relative sub/AGENTS.md path instead of falling back to an absolute path.

Copilot generated this review using guidance from repository custom instructions.
rel_path = str(placement.agents_path)
lines.append(f"{rel_path}")
Expand Down Expand Up @@ -868,7 +868,7 @@ def _generate_distributed_summary(self, distributed_result, config: CompilationC

for placement in distributed_result.placements:
try:
rel_path = placement.agents_path.relative_to(self.base_dir.resolve()).as_posix()
rel_path = placement.agents_path.resolve().relative_to(self.base_dir.resolve()).as_posix()
except ValueError:
rel_path = str(placement.agents_path)
lines.append(f"- {rel_path} ({len(placement.instructions)} instructions)")
Expand Down
Loading