Skip to content

fix(deps): use ASCII-safe non-TTY output to prevent Windows cp1252 decode warnings in deps commands #432

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

Closed
abdimk wants to merge 1 commit into from
Closed

fix(deps): use ASCII-safe non-TTY output to prevent Windows cp1252 decode warnings in deps commands #432

Show file tree
Hide file tree
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
41 changes: 31 additions & 10 deletions src/apm_cli/commands/deps/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,16 @@ def list_packages():
# Import Rich components with fallback
from rich.table import Table
from rich.console import Console
import shutil
term_width = shutil.get_terminal_size((120, 24)).columns
console = Console(width=max(120, term_width))
has_rich = True
# Rich tables include unicode box drawing by default. Avoid rich output
# for non-interactive streams so subprocess capture stays ASCII-safe.
is_tty = bool(getattr(sys.stdout, "isatty", lambda: False)())
has_rich = is_tty
console = Console(
width=max(120, term_width),
force_terminal=False,
no_color=True,
) if has_rich else None
Comment on lines +47 to +55
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Even when isatty() is true, Rich Table defaults to Unicode box-drawing characters, which conflicts with the repo's ASCII-only CLI output rule (see .github/instructions/encoding.instructions.md). If the intent is to keep deps output ASCII-safe, consider always using the plain-text fallback, or explicitly configure Rich to render ASCII borders (and avoid forcing no_color=True so interactive output stays consistent with other commands).

Copilot uses AI. Check for mistakes.
except ImportError:
has_rich = False
console = None
Expand All @@ -64,12 +70,20 @@ def list_packages():
# Load project dependencies to check for orphaned packages
# GitHub: owner/repo or owner/virtual-pkg-name (2 levels)
# Azure DevOps: org/project/repo or org/project/virtual-pkg-name (3 levels)
declared_sources = {} # dep_path -> 'github' | 'azure-devops'
# Local: _local/package-name
declared_sources = {} # dep_path -> 'github' | 'azure-devops' | 'local'
try:
apm_yml_path = project_root / APM_YML_FILENAME
if apm_yml_path.exists():
project_package = APMPackage.from_apm_yml(apm_yml_path)
for dep in project_package.get_apm_dependencies():
# Handle local dependencies
if dep.is_local and dep.local_path:
# Local packages are installed as _local/<dirname>
pkg_name = Path(dep.local_path).name
declared_sources[f"_local/{pkg_name}"] = 'local'
continue

# Build the expected installed package name
repo_parts = dep.repo_url.split('/')
source = 'azure-devops' if dep.is_azure_devops() else 'github'
Expand Down Expand Up @@ -234,7 +248,7 @@ def list_packages():
sys.exit(1)


@deps.command(help="Show dependency tree structure")
@deps.command(help="Show dependency tree structure")
def tree():
"""Display dependencies in hierarchical tree format using lockfile."""
logger = CommandLogger("deps-tree")
Expand All @@ -243,8 +257,11 @@ def tree():
# Import Rich components with fallback
from rich.tree import Tree
from rich.console import Console
console = Console()
has_rich = True
# Tree rendering uses unicode branches. Use fallback text when output is
# captured/piped to keep bytes ASCII-safe for Windows cp1252 readers.
is_tty = bool(getattr(sys.stdout, "isatty", lambda: False)())
has_rich = is_tty
console = Console(force_terminal=False, no_color=True) if has_rich else None
Comment on lines +260 to +264
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

deps tree enables Rich output when stdout is a TTY, but Rich Tree renders Unicode branch characters (and this code forces no_color=True, reducing interactive UX). If we need to guarantee ASCII-safe output per the encoding guidelines, the Rich path should be disabled here too, or replaced with an ASCII-only renderer regardless of TTY.

Copilot uses AI. Check for mistakes.
except ImportError:
has_rich = False
console = None
Expand Down Expand Up @@ -530,13 +547,17 @@ def info(package: str):
try:
# Load package information
package_info = _get_detailed_package_info(package_path)

# Display with Rich panel if available
try:
from rich.panel import Panel
from rich.console import Console
from rich.text import Text
console = Console()
# Rich panels use unicode borders. Keep captured output ASCII by
# using plain-text fallback when not writing to a terminal.
is_tty = bool(getattr(sys.stdout, "isatty", lambda: False)())
if not is_tty:
raise ImportError
console = Console(force_terminal=False, no_color=True)

content_lines = []
content_lines.append(f"[bold]Name:[/bold] {package_info['name']}")
Expand Down
2 changes: 1 addition & 1 deletion src/apm_cli/core/token_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def resolve_credential_from_git(host: str) -> Optional[str]:
text=True,
timeout=GitHubTokenManager._get_credential_timeout(),
env={**os.environ, 'GIT_TERMINAL_PROMPT': '0',
'GIT_ASKPASS': '' if sys.platform != 'win32' else 'echo'},
'GIT_ASKPASS': ''},
Comment on lines 115 to +116
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

On Windows, setting GIT_ASKPASS to an empty string is known to cause issues (the repo already documents this in AuthResolver._build_git_env). This change removes the win32 special-casing and may break git credential fill on Windows; keep the sys.platform == "win32" handling (e.g., use echo) or centralize the logic so token_manager and auth agree.

Copilot uses AI. Check for mistakes.
)
if result.returncode != 0:
return None
Expand Down
7 changes: 5 additions & 2 deletions src/apm_cli/models/dependency/reference.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,17 @@ def get_virtual_package_name(self) -> str:
@staticmethod
def is_local_path(dep_str: str) -> bool:
"""Check if a dependency string looks like a local filesystem path.
Local paths start with './', '../', '/', or '~'.

Local paths start with './', '../', '/', or '~', or are Windows absolute paths.
Protocol-relative URLs ('//...') are explicitly excluded.
"""
s = dep_str.strip()
# Reject protocol-relative URLs ('//...')
if s.startswith('//'):
return False
# Check for Windows absolute paths (e.g., C:\, D:/)
if len(s) >= 2 and s[1] == ':':
return True
return s.startswith(('./','../', '/', '~/', '~\\', '.\\', '..\\'))
Comment on lines +132 to 142
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

is_local_path() treats any string with a colon in position 2 as a Windows absolute path. This will incorrectly classify non-path dependency strings like gh:owner/repo (or any x:y prefix) as local and then fail later with a misleading "local path does not exist" error. Tighten the check to a real drive-letter absolute path (e.g., ^[A-Za-z]:[\\/]) and consider also supporting UNC paths (\\server\\share).

Suggested change
Local paths start with './', '../', '/', or '~', or are Windows absolute paths.
Protocol-relative URLs ('//...') are explicitly excluded.
"""
s = dep_str.strip()
# Reject protocol-relative URLs ('//...')
if s.startswith('//'):
return False
# Check for Windows absolute paths (e.g., C:\, D:/)
if len(s) >= 2 and s[1] == ':':
return True
return s.startswith(('./','../', '/', '~/', '~\\', '.\\', '..\\'))
Local paths start with './', '../', '/', or '~', or are Windows absolute
paths (drive-letter or UNC). Protocol-relative URLs ('//...') are
explicitly excluded.
"""
s = dep_str.strip()
# Reject protocol-relative URLs ('//...')
if s.startswith('//'):
return False
# Check for Windows drive-letter absolute paths (e.g., C:\, D:/).
# Require an ASCII letter, a colon, and then a slash or backslash to
# avoid misclassifying strings like 'gh:owner/repo' as paths.
if len(s) >= 3 and s[0].isalpha() and s[1] == ':' and s[2] in ('\\', '/'):
return True
# Check for Windows UNC paths (e.g., \\server\share).
if s.startswith('\\\\') and len(s) > 2:
return True
return s.startswith(('./', '../', '/', '~/', '~\\', '.\\', '..\\'))

Copilot uses AI. Check for mistakes.
Comment on lines +139 to 142
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

This change adds Windows absolute path detection to is_local_path(), but there are unit tests for local-path detection and they currently don't cover drive-letter paths or UNC paths. Add tests for inputs like C:\\repos\\pkg / D:/repos/pkg and a negative case such as gh:owner/repo to prevent regressions in the new branch.

Suggested change
# Check for Windows absolute paths (e.g., C:\, D:/)
if len(s) >= 2 and s[1] == ':':
return True
return s.startswith(('./','../', '/', '~/', '~\\', '.\\', '..\\'))
# UNC paths on Windows (e.g., \\server\share)
if s.startswith('\\\\'):
return True
# Check for Windows absolute paths with drive letter (e.g., C:\, D:/)
if re.match(r'^[A-Za-z]:[\\/]', s):
return True
# Relative and Unix-style absolute paths
return s.startswith(('./', '../', '/', '~/', '~\\', '.\\', '..\\'))

Copilot uses AI. Check for mistakes.

def get_unique_key(self) -> str:
Expand Down
Loading