fix(deps): use ASCII-safe non-TTY output to prevent Windows cp1252 decode warnings in deps commands#432
Conversation
…code warnings in deps commands
There was a problem hiding this comment.
Pull request overview
This PR updates the apm deps command output to avoid Rich-rendered Unicode when output is captured/piped (primarily to prevent Windows cp1252 encoding issues), and improves local-dependency detection/labeling so local packages are recognized consistently in dependency listings.
Changes:
- Gate Rich output in
depscommands on TTY detection and fall back to plain text output when not interactive. - Add local dependency source mapping in
deps listso_local/<name>packages are not flagged as orphaned. - Extend dependency-string parsing to treat Windows absolute paths as local paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/apm_cli/models/dependency/reference.py |
Expands local path detection to include Windows absolute paths. |
src/apm_cli/core/token_manager.py |
Changes git credential subprocess environment (GIT_ASKPASS) handling. |
src/apm_cli/commands/deps/cli.py |
Switches deps output to avoid Rich when stdout is not a TTY; adds local-source mapping in deps listing. |
Note on the question in the PR description: the repo is currently configured to use Black (plus isort + mypy) and does not currently include Ruff configuration/dependencies in pyproject.toml; adopting Ruff would likely be best handled as a separate PR/discussion since it can involve large-scale reformat/lint churn.
| # 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 |
There was a problem hiding this comment.
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).
| # 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 |
There was a problem hiding this comment.
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.
| 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(('./','../', '/', '~/', '~\\', '.\\', '..\\')) |
There was a problem hiding this comment.
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).
| 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(('./', '../', '/', '~/', '~\\', '.\\', '..\\')) |
| # Check for Windows absolute paths (e.g., C:\, D:/) | ||
| if len(s) >= 2 and s[1] == ':': | ||
| return True | ||
| return s.startswith(('./','../', '/', '~/', '~\\', '.\\', '..\\')) |
There was a problem hiding this comment.
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.
| # 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(('./', '../', '/', '~/', '~\\', '.\\', '..\\')) |
| env={**os.environ, 'GIT_TERMINAL_PROMPT': '0', | ||
| 'GIT_ASKPASS': '' if sys.platform != 'win32' else 'echo'}, | ||
| 'GIT_ASKPASS': ''}, |
There was a problem hiding this comment.
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.
danielmeppiel
left a comment
danielmeppiel
left a comment
There was a problem hiding this comment.
Thanks for tackling this @abdimk — good instinct on all three areas (Rich Unicode in piped output, Windows path detection, and credential handling). A few things need adjusting before we can merge:
1. is_local_path() — false positive on prefixed strings (critical)
The current check s[1] == ":" will misclassify strings like gh:owner/repo or any x:... prefix as a local Windows path. Please tighten to require a drive letter + colon + slash:
if len(s) >= 3 and s[0].isalpha() and s[1] == ":" and s[2] in ("\\", "/"):
return TrueAlso worth adding UNC path support (\\\\server\\share) while we are here.
Tests needed: please add positive cases (C:\\repos\\pkg, D:/repos/pkg) and a negative case (gh:owner/repo) to the existing is_local_path test suite so this does not regress.
2. GIT_ASKPASS — keep the Windows special-casing (critical)
auth.py:407 explicitly uses "echo" on Windows because an empty string causes issues with git credential fill. Removing that guard from token_manager.py creates a divergence and may break credential resolution on Windows. Please revert this change and keep the sys.platform == "win32" handling consistent with auth.py.
3. Rich output — still emits Unicode on TTY
The TTY gate is a good idea, but Rich Table / Tree / Panel still emit Unicode box-drawing and branch characters when stdout is a TTY. Our repo enforces ASCII-only output (see .github/instructions/encoding.instructions.md). Two options:
- Option A: Disable Rich rendering entirely in these commands and use the plain-text fallback always (simplest, matches the encoding rule).
- Option B: Configure Rich with
Console(safe_box=True)plus manually settingASCII_ONLYtable box styles — but this is more work and still may leak Unicode fromTree.
I would recommend Option A for now — it is the safest path and we can revisit Rich ASCII rendering as a separate improvement.
Also: the no_color=True + force_terminal=False combination on the TTY path removes color from interactive sessions, which degrades UX vs. other commands. If you go with Option B, drop no_color=True so colors still work.
Happy to help if you have questions on any of these. The fixes are fairly contained — the is_local_path regex and test additions are the main effort.
|
Alright! Thanks for the clarrfication @danielmeppiel , I’ll try to fix this one soon. In the meantime, what are your thoughts on using Ruff for linting and type-safety checks insted of Black ? |
Hey, I’m new to this repo and started by fixing some small but crucial issues that were causing test failures.
Summary of what i have changed
Note
Also, is there any reason you’re using Black instead of Ruff? Ruff provides linting and type checks as well, and running ruff check shows the project has around 511 issues, with about 385 of them being easily fixable.