Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion build/apm.spec
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,12 @@ a = Analysis(
win_private_assemblies=False,
cipher=None,
noarchive=False,
optimize=1, # -O: strip asserts; keep __doc__ so Click reads command help from docstrings (#1298)
optimize=2, # -OO: strip asserts AND __doc__. Reduces PYZ string surface to
# avoid Defender ML false positives (Trojan:Script/Wacatac.H!ml on v0.14.0; see #1407).
# Every @click.command() MUST set help= explicitly; the docstring fallback is gone.
# tests/unit/test_cli_consistency.py::test_every_registered_command_has_explicit_help
# is the silent-drift guard. See #1298 for the original (now-superseded) motivation
# to keep docstrings.
)

# Exclude bundled OpenSSL shared libraries on Linux.
Expand Down
37 changes: 21 additions & 16 deletions src/apm_cli/commands/view.py
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,26 @@ def display_versions(package: str, logger: CommandLogger) -> None:
# ------------------------------------------------------------------


@click.command(name="view")
_VIEW_HELP = (
"View package metadata or list remote versions.\n\n"
"Without FIELD, displays local metadata for an installed package. "
"With FIELD, queries specific data (may contact the remote).\n\n"
"\b\n"
"Fields:\n"
" versions List available remote tags and branches\n\n"
"\b\n"
Comment on lines +422 to +425

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Keeping \b as-is. Quick rationale:

  1. PyZ surface cost is negligible. Two \x08 bytes inside a ~700-byte string, against the ~1.4 MB of marshalled docstrings this PR strips. Lost in the noise -- not a meaningful contribution to the ML feature surface this PR targets.

  2. Printable-ASCII rule intent is not violated. The rule (encoding.instructions.md) targets characters that fail cp1252 encoding on Windows terminals (UnicodeEncodeError: charmap codec cant encode...). \x08 encodes fine in cp1252, and Click's HelpFormatter strips it before any output is written -- it's a no-rewrap sentinel, not user-visible output. See https://click.palletsprojects.com/en/stable/documentation/#preventing-rewrapping. The byte never reaches the terminal.

  3. Established idiom in this repo. Same \b sentinel is already used in 4 other commands' docstrings: outdated.py:295, deps/cli.py:751, audit.py:922, 931. Mine is the first non-docstring case (those get stripped by optimize=2), but the user-rendered output is identical.

  4. Alternatives produce worse UX. Tested under PYTHONOPTIMIZE=2:

    • Drop \b, use \n\n paragraphs -> Click rewraps the Examples block into an unreadable run-on: apm view org/repo # Local metadata apm view org/repo versions # Remote tags/branches ...
    • Move Examples to epilog= -> same rewrap problem.

\b is the documented Click answer for preserving Fields/Examples formatting, and it's what every Click maintainer/reader expects to see.

"Examples:\n"
" apm view org/repo # Local metadata\n"
" apm view org/repo versions # Remote tags/branches\n"
" apm view org/repo -g # From user scope"
)


@click.command(
name="view",
help=_VIEW_HELP,
short_help="View package metadata or list remote versions",
)
@click.argument("package", required=True)
@click.argument("field", required=False, default=None)
@click.option(
Expand All @@ -427,21 +446,7 @@ def display_versions(package: str, logger: CommandLogger) -> None:
help="Inspect package from user scope (~/.apm/)",
)
def view(package: str, field: str | None, global_: bool):
"""View package metadata or list remote versions.

Without FIELD, displays local metadata for an installed package.
With FIELD, queries specific data (may contact the remote).

\b
Fields:
versions List available remote tags and branches

\b
Examples:
apm view org/repo # Local metadata
apm view org/repo versions # Remote tags/branches
apm view org/repo -g # From user scope
"""
"""View package metadata or list remote versions."""
from ..core.scope import InstallScope, get_apm_dir

logger = CommandLogger("view")
Expand Down
32 changes: 32 additions & 0 deletions tests/unit/test_build_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,38 @@ def test_spec_file_helper_functions_are_extractable(self):
assert "def _read_version_from_pyproject" in snippet
assert "def is_upx_available" in snippet

def test_pyinstaller_optimize_is_2(self):
"""Regression guard: Analysis(optimize=...) must be 2 (python -OO).

``optimize=2`` strips both ``assert``s and ``__doc__`` from compiled
bytecode. Stripping ``__doc__`` keeps the PYZ string surface small,
which is critical for avoiding the Defender ML false positive
(Trojan:Script/Wacatac.H!ml) that affected v0.14.0 -- see #1407.

If this test fails, do NOT just lower the value to fix #1298-style
docstring-fallback bugs. Instead, set ``help=`` (or ``short_help=``)
explicitly on the offending Click command; the
``test_every_registered_command_has_explicit_help`` test in
``tests/unit/test_cli_consistency.py`` enforces this contract.
"""
source = _SPEC_FILE.read_text(encoding="utf-8")
tree = ast.parse(source, filename=str(_SPEC_FILE))
analysis_optimize_values: list[int] = []
for node in ast.walk(tree):
if (
isinstance(node, ast.Call)
and isinstance(node.func, ast.Name)
and node.func.id == "Analysis"
):
for kw in node.keywords:
if kw.arg == "optimize" and isinstance(kw.value, ast.Constant):
analysis_optimize_values.append(kw.value.value)
assert analysis_optimize_values, "No Analysis(optimize=...) keyword found in build/apm.spec"
assert all(v == 2 for v in analysis_optimize_values), (
f"Analysis(optimize=...) must be 2 (got {analysis_optimize_values}). "
"See test docstring for context."
)


# ---------------------------------------------------------------------------
# 2. should_use_upx()
Expand Down
38 changes: 38 additions & 0 deletions tests/unit/test_cli_consistency.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,50 @@

from unittest.mock import patch

import click
from click.testing import CliRunner

from apm_cli.cli import cli
from apm_cli.output.script_formatters import ScriptExecutionFormatter


def _walk_commands(group: click.Group, prefix: tuple[str, ...] = ()):
"""Yield (path_tuple, command) for every command reachable under group."""
for name, cmd in group.commands.items():
path = (*prefix, name)
yield path, cmd
if isinstance(cmd, click.Group):
yield from _walk_commands(cmd, path)


def test_every_registered_command_has_explicit_help():
"""Silent-drift guard: no command may rely on the docstring fallback.

The release binary is built with PyInstaller ``optimize=2`` (``python -OO``)
to keep the PYZ string surface small (Defender ML false-positive mitigation;
see #1407 and build/apm.spec). ``-OO`` strips ``__doc__``, so any Click
command without an explicit ``help=`` renders with an empty summary and
empty ``--help`` body in the binary -- exactly the regression that #1298
reported for ``apm view``.

Every command and sub-command registered under the top-level ``cli`` group
must set ``help=`` (or ``short_help=``) explicitly.
"""
missing: list[str] = []
for path, cmd in _walk_commands(cli):
if cmd.hidden:
# Hidden aliases (e.g. ``apm info``) inherit help from their source
# command; checking the visible command is sufficient.
continue
help_text = (cmd.help or "").strip() or (cmd.short_help or "").strip()
if not help_text:
missing.append(" ".join(path))
assert not missing, (
"Commands missing explicit help= (would render blank under "
"PyInstaller optimize=2): " + ", ".join(sorted(missing))
)


def test_experimental_subcommand_help_is_specific():
runner = CliRunner()

Expand Down
Loading