From ae771e2b62a6f525d9ae69cf9c0a62ab532a5487 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Tue, 7 Jul 2026 01:44:25 +0000 Subject: [PATCH 01/13] Validate CLAUDE_CLI_VERSION and avoid shell interpolation in download_cli.py Accept only "latest" or a version starting with an alphanumeric character. On Unix, download install.sh to a temp file and execute it directly instead of piping curl into bash through a shell string. On Windows, pass the version to PowerShell in the environment instead of interpolating it into -Command. --- scripts/download_cli.py | 173 +++++++++++++---- tests/test_download_cli.py | 369 +++++++++++++++++++++++++++++++++++++ 2 files changed, 510 insertions(+), 32 deletions(-) create mode 100644 tests/test_download_cli.py diff --git a/scripts/download_cli.py b/scripts/download_cli.py index 7167159b5..d2c082aaf 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -8,16 +8,50 @@ import os import platform import random +import re import shutil import subprocess import sys +import tempfile import time +from collections.abc import Callable from pathlib import Path +# A version must start with an alphanumeric character (so flag-shaped values +# like "--help" are rejected) and may then contain only characters that appear +# in real CLI versions, including dev builds such as +# "2.1.146-dev.20260519.t105443.shaece3dab". +# +# This allowlist is defense in depth rather than the only thing standing between +# a hostile CLAUDE_CLI_VERSION and a shell: neither install path interpolates the +# version into a command string. Widening it requires re-reading +# tests/test_download_cli.py::TestGetCliVersion. +# +# Deliberately unanchored, and matched with fullmatch() rather than match(): +# with "^...$" a swap to match() would silently accept a trailing newline +# ("1.0.0\n"); unanchored, the same swap accepts obvious prefixes like +# "1.0.0; id" and fails immediately in tests. +VERSION_PATTERN = re.compile(r"[0-9A-Za-z][0-9A-Za-z.+-]*") + +# The Windows installer reads the version out of the environment under this +# name instead of having it spliced into the PowerShell command text. +INSTALL_VERSION_ENV_VAR = "CLAUDE_CLI_INSTALL_VERSION" + def get_cli_version() -> str: - """Get the CLI version to download from environment or default.""" - return os.environ.get("CLAUDE_CLI_VERSION", "latest") + """Get the CLI version to download from environment or default. + + Raises: + ValueError: If CLAUDE_CLI_VERSION is set to something other than + "latest" or a value matching VERSION_PATTERN. + """ + version = os.environ.get("CLAUDE_CLI_VERSION", "latest") + if version != "latest" and not VERSION_PATTERN.fullmatch(version): + raise ValueError( + f"Invalid CLAUDE_CLI_VERSION: {version!r}. " + f"Expected 'latest' or a version matching {VERSION_PATTERN.pattern}" + ) + return version def find_installed_cli() -> Path | None: @@ -50,6 +84,69 @@ def find_installed_cli() -> Path | None: return None +def run_command(command: list[str], env: dict[str, str] | None = None) -> None: + """Run one install command with no shell and no inherited stdin. + + install.sh runs `claude install`, which branches on `[ -t 0 ]`. Under the + old `curl | bash` its stdin was the pipe, so it never saw a TTY; keep it + that way rather than handing it the caller's terminal. + + ``env`` replaces the child's environment when given; None inherits ours. + """ + subprocess.run( + command, + check=True, + capture_output=True, + stdin=subprocess.DEVNULL, + env=env, + ) + + +def check_install_script(script_path: str) -> None: + """Reject a downloaded install script that is not a shell script. + + claude.ai answers unknown paths with HTTP 200 and an HTML body, which + `curl -f` cannot detect, so check the shebang before executing the file. + A wrong body is deterministic, so this fails fast instead of retrying. + """ + with Path(script_path).open("rb") as f: + magic = f.read(2) + if magic != b"#!": + print( + f"Error: downloaded install script does not start with a shebang " + f"(first bytes: {magic!r}). Refusing to execute it.", + file=sys.stderr, + ) + sys.exit(1) + + +def retry_install(attempt: Callable[[], None]) -> None: + """Run an install attempt, retrying the whole attempt on command failure.""" + # Small jitter to stagger parallel matrix builds hitting the same endpoint + time.sleep(random.uniform(0, 5)) + + last_err: subprocess.CalledProcessError | None = None + for attempt_num in range(1, 4): + try: + attempt() + return + except subprocess.CalledProcessError as e: + last_err = e + if attempt_num < 3: + delay = 2**attempt_num + print( + f"Install attempt {attempt_num} failed (exit {e.returncode}), " + f"retrying in {delay}s...", + file=sys.stderr, + ) + time.sleep(delay) + + print(f"Error downloading CLI after 3 attempts: {last_err}", file=sys.stderr) + print(f"stdout: {last_err.stdout.decode()}", file=sys.stderr) + print(f"stderr: {last_err.stderr.decode()}", file=sys.stderr) + sys.exit(1) + + def download_cli() -> None: """Download Claude Code CLI using the official install script.""" version = get_cli_version() @@ -59,7 +156,13 @@ def download_cli() -> None: # Build install command based on platform if system == "Windows": - # Use PowerShell installer on Windows + # Use PowerShell installer on Windows. The version is handed to + # PowerShell in the environment and referenced by name, so it is never + # part of the command text that PowerShell parses. `$env:NAME` in + # argument position expands to exactly one argument -- PowerShell does + # not re-split or re-parse it -- which is the argv separation the Unix + # path gets from `["bash", script, version]`. + install_env: dict[str, str] | None = None if version == "latest": install_cmd = [ "powershell", @@ -74,39 +177,45 @@ def download_cli() -> None: "-ExecutionPolicy", "Bypass", "-Command", - f"& ([scriptblock]::Create((irm https://claude.ai/install.ps1))) {version}", + "& ([scriptblock]::Create((irm https://claude.ai/install.ps1))) " + f"$env:{INSTALL_VERSION_ENV_VAR}", ] - else: + install_env = {**os.environ, INSTALL_VERSION_ENV_VAR: version} + retry_install(lambda: run_command(install_cmd, env=install_env)) + return + + # Download install.sh to a file and run it directly rather than piping + # curl into bash through a shell string: nothing is interpolated into a + # command line, and check=True sees curl's and bash's exit codes + # separately instead of only the last status of a pipeline. + with tempfile.TemporaryDirectory() as tmpdir: + script_path = str(Path(tmpdir) / "install.sh") + + # -L follows the cross-host redirect to the bootstrap script. # --retry-all-errors covers 429 from claude.ai when multiple matrix - # jobs fetch install.sh simultaneously. pipefail propagates curl's - # exit code through the pipe so subprocess.run's check=True catches it. - curl = "curl -fsSL --retry 5 --retry-delay 2 --retry-all-errors https://claude.ai/install.sh" - target = "" if version == "latest" else f" -s {version}" - install_cmd = ["bash", "-c", f"set -o pipefail; {curl} | bash{target}"] - - # Small jitter to stagger parallel matrix builds hitting the same endpoint - time.sleep(random.uniform(0, 5)) + # jobs fetch install.sh simultaneously. + curl_cmd = [ + "curl", + "-fsSL", + "--retry", + "5", + "--retry-delay", + "2", + "--retry-all-errors", + "-o", + script_path, + "https://claude.ai/install.sh", + ] + bash_cmd = ["bash", script_path] + if version != "latest": + bash_cmd.append(version) - last_err: subprocess.CalledProcessError | None = None - for attempt in range(1, 4): - try: - subprocess.run(install_cmd, check=True, capture_output=True) - return - except subprocess.CalledProcessError as e: - last_err = e - if attempt < 3: - delay = 2**attempt - print( - f"Install attempt {attempt} failed (exit {e.returncode}), " - f"retrying in {delay}s...", - file=sys.stderr, - ) - time.sleep(delay) + def attempt() -> None: + run_command(curl_cmd) + check_install_script(script_path) + run_command(bash_cmd) - print(f"Error downloading CLI after 3 attempts: {last_err}", file=sys.stderr) - print(f"stdout: {last_err.stdout.decode()}", file=sys.stderr) - print(f"stderr: {last_err.stderr.decode()}", file=sys.stderr) - sys.exit(1) + retry_install(attempt) def copy_cli_to_bundle() -> None: diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py new file mode 100644 index 000000000..e943baefd --- /dev/null +++ b/tests/test_download_cli.py @@ -0,0 +1,369 @@ +"""Tests for scripts/download_cli.py version validation and install invocation.""" + +import importlib.util +import subprocess +import sys +from collections.abc import Iterator +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +import pytest + +# scripts/ is not a package, so load download_cli.py by path +_spec = importlib.util.spec_from_file_location( + "download_cli", + Path(__file__).parent.parent / "scripts" / "download_cli.py", +) +assert _spec is not None and _spec.loader is not None +download_cli = importlib.util.module_from_spec(_spec) +sys.modules["download_cli"] = download_cli +_spec.loader.exec_module(download_cli) + +DEV_VERSION = "2.1.146-dev.20260519.t105443.shaece3dab" +ENV_VAR = download_cli.INSTALL_VERSION_ENV_VAR + + +class TestGetCliVersion: + """CLAUDE_CLI_VERSION must be 'latest' or a plain version token.""" + + def test_default_is_latest(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("CLAUDE_CLI_VERSION", raising=False) + assert download_cli.get_cli_version() == "latest" + + @pytest.mark.parametrize( + "version", + [ + "latest", + "1.2.3", + "2.1.195", + DEV_VERSION, + "1.2.3+build.4", + "0", + ], + ) + def test_accepted(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: + monkeypatch.setenv("CLAUDE_CLI_VERSION", version) + assert download_cli.get_cli_version() == version + + @pytest.mark.parametrize( + "version", + [ + "1.0.0; touch /tmp/pwned", + "--help", + "-s", + "$(id)", + "`id`", + "1.0.0 && id", + "1.0.0 | id", + "1.0.0\nid", + "1.0.0\n", + "1.0.0 2.0.0", + "$VERSION", + "../../etc/passwd", + "", + ".1.2.3", + ], + ) + def test_rejected(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: + monkeypatch.setenv("CLAUDE_CLI_VERSION", version) + with pytest.raises(ValueError, match="Invalid CLAUDE_CLI_VERSION"): + download_cli.get_cli_version() + + def test_error_names_the_offending_value( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("CLAUDE_CLI_VERSION", "1.0.0; id") + with pytest.raises(ValueError) as excinfo: + download_cli.get_cli_version() + assert "1.0.0; id" in str(excinfo.value) + + @pytest.mark.parametrize( + "char", + [" ", ";", "$", "`", '"', "'", "(", ")", "&", "|", "\n", "\r"], + ) + def test_version_pattern_admits_no_shell_metacharacters(self, char: str) -> None: + """VERSION_PATTERN must never admit a shell metacharacter. + + No install path depends on this any more: the Unix path passes the + version as its own argv element, and the Windows path hands it to + PowerShell in the environment rather than in the command text. The + allowlist is still the invariant we want -- a version that can express + `;` or `$` is not a version -- so keep it as defense in depth. Widening + the pattern (to permit `_` or `~`, say) must not let any of these + through. + """ + assert not download_cli.VERSION_PATTERN.fullmatch(char) + assert not download_cli.VERSION_PATTERN.fullmatch(f"1.2.3{char}") + assert not download_cli.VERSION_PATTERN.fullmatch(f"1.2.3{char}whoami") + + def test_version_pattern_is_unanchored(self) -> None: + """The pattern must stay unanchored so a future swap of fullmatch() for + match() fails loudly on a prefix, rather than silently reintroducing + the trailing-newline bypass that `^...$` + match() allows.""" + assert "^" not in download_cli.VERSION_PATTERN.pattern + assert "$" not in download_cli.VERSION_PATTERN.pattern + assert not download_cli.VERSION_PATTERN.fullmatch("1.0.0\n") + + +@pytest.fixture +def no_sleep() -> Iterator[None]: + """Skip the jitter and retry sleeps.""" + with patch.object(download_cli.time, "sleep"): + yield + + +def _fake_curl(body: bytes = b"#!/bin/bash\necho install\n") -> object: + """A subprocess.run side effect that makes `curl -o PATH` write ``body``.""" + + def side_effect(command: list[str], **kwargs: object) -> MagicMock: + if command[0] == "curl" and "-o" in command: + Path(command[command.index("-o") + 1]).write_bytes(body) + return MagicMock() + + return side_effect + + +def _run_unix_download( + version: str, body: bytes = b"#!/bin/bash\necho install\n" +) -> list[list[str]]: + """Run download_cli() on the Unix path, returning the argv of each subprocess.""" + with ( + patch.object(download_cli.platform, "system", return_value="Linux"), + patch.object( + download_cli.subprocess, "run", side_effect=_fake_curl(body) + ) as mock_run, + patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": version}), + ): + download_cli.download_cli() + return [call.args[0] for call in mock_run.call_args_list] + + +@pytest.mark.usefixtures("no_sleep") +class TestUnixInstall: + """The Unix path downloads install.sh, then executes it without a shell.""" + + def test_downloads_then_executes_script(self) -> None: + curl_cmd, bash_cmd = _run_unix_download("1.2.3") + + assert curl_cmd[0] == "curl" + assert curl_cmd[-1] == "https://claude.ai/install.sh" + assert bash_cmd[0] == "bash" + + # curl writes install.sh into a temp dir; bash executes that same file. + script_path = curl_cmd[curl_cmd.index("-o") + 1] + assert Path(script_path).name == "install.sh" + assert bash_cmd[1] == script_path + + def test_retry_flags_preserved(self) -> None: + curl_cmd, _ = _run_unix_download("1.2.3") + assert "--retry-all-errors" in curl_cmd + assert curl_cmd[curl_cmd.index("--retry") + 1] == "5" + assert curl_cmd[curl_cmd.index("--retry-delay") + 1] == "2" + + @pytest.mark.parametrize("flag", ["f", "s", "S", "L"]) + def test_curl_short_flags_present(self, flag: str) -> None: + """`install.sh` is a cross-host 302, so -L is required; -f/-s/-S keep the + original error and quiet behavior. Checked per letter rather than as the + literal "-fsSL" so splitting the cluster can't silently drop one.""" + curl_cmd, _ = _run_unix_download("1.2.3") + clusters = [ + arg[1:] + for arg in curl_cmd + if arg.startswith("-") and not arg.startswith("--") + ] + assert any(flag in cluster for cluster in clusters), ( + f"curl -{flag} missing from {curl_cmd!r}" + ) + + def test_stdin_is_devnull(self) -> None: + """install.sh runs `claude install`, which branches on `[ -t 0 ]`. The + old `curl | bash` gave it a pipe; it must never inherit a real TTY.""" + with ( + patch.object(download_cli.platform, "system", return_value="Linux"), + patch.object( + download_cli.subprocess, "run", side_effect=_fake_curl() + ) as mock_run, + patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), + ): + download_cli.download_cli() + + assert mock_run.call_args_list + for call in mock_run.call_args_list: + assert call.kwargs["stdin"] is subprocess.DEVNULL, ( + f"{call.args[0][0]} may inherit the caller's TTY" + ) + + def test_latest_passes_no_version_argument(self) -> None: + _, bash_cmd = _run_unix_download("latest") + assert bash_cmd[0] == "bash" + assert len(bash_cmd) == 2 + + @pytest.mark.parametrize("version", ["1.2.3", DEV_VERSION]) + def test_version_is_its_own_argv_element(self, version: str) -> None: + """Regression guard: the version must never be interpolated into a + command string, and the Unix path must never invoke a shell.""" + commands = _run_unix_download(version) + + for cmd in commands: + assert isinstance(cmd, list), f"argv must be a list, got {cmd!r}" + # No `bash -c ` / `sh -c ` anywhere. + assert "-c" not in cmd, f"shell string reintroduced: {cmd!r}" + # The version may only appear as a standalone argv element. + for arg in cmd: + assert arg == version or version not in arg, ( + f"version interpolated into {arg!r}" + ) + + bash_cmd = commands[-1] + assert bash_cmd == ["bash", bash_cmd[1], version] + + def test_never_uses_shell_true(self) -> None: + with ( + patch.object(download_cli.platform, "system", return_value="Linux"), + patch.object( + download_cli.subprocess, "run", side_effect=_fake_curl() + ) as mock_run, + patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), + ): + download_cli.download_cli() + + for call in mock_run.call_args_list: + assert call.kwargs.get("shell") is not True + assert call.kwargs["check"] is True + + @pytest.mark.parametrize( + "body", + [ + b"\nNot found", + b"", + b"#", + b"\x7fELF", + ], + ) + def test_non_shebang_body_is_rejected_before_bash( + self, body: bytes, capsys: pytest.CaptureFixture[str] + ) -> None: + """claude.ai serves HTTP 200 + HTML for unknown paths, which `curl -f` + cannot detect. Such a body must never reach bash.""" + with ( + patch.object(download_cli.platform, "system", return_value="Linux"), + patch.object( + download_cli.subprocess, "run", side_effect=_fake_curl(body) + ) as mock_run, + patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), + pytest.raises(SystemExit) as excinfo, + ): + download_cli.download_cli() + + assert excinfo.value.code == 1 + assert "does not start with a shebang" in capsys.readouterr().err + # curl ran once, bash never; the bad body is not retried. + assert [call.args[0][0] for call in mock_run.call_args_list] == ["curl"] + + def test_shebang_body_is_executed(self) -> None: + commands = _run_unix_download("1.2.3", body=b"#!/bin/sh\nexit 0\n") + assert [cmd[0] for cmd in commands] == ["curl", "bash"] + + def test_curl_failure_is_not_masked(self) -> None: + """A failing download must fail the build, not fall through to bash.""" + error = subprocess.CalledProcessError(1, ["curl"], output=b"", stderr=b"boom") + + def fake_run(command: list[str], **kwargs: object) -> MagicMock: + if command[0] == "curl": + raise error + return MagicMock() + + with ( + patch.object(download_cli.platform, "system", return_value="Linux"), + patch.object(download_cli.subprocess, "run", side_effect=fake_run) as run, + patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), + pytest.raises(SystemExit) as excinfo, + ): + download_cli.download_cli() + + assert excinfo.value.code == 1 + # curl attempted 3 times, bash never reached. + assert [call.args[0][0] for call in run.call_args_list] == ["curl"] * 3 + + +def _run_windows_download(version: str) -> Any: + """Run download_cli() on the Windows path, returning the single run() call.""" + with ( + patch.object(download_cli.platform, "system", return_value="Windows"), + patch.object(download_cli.subprocess, "run") as mock_run, + patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": version}), + ): + download_cli.download_cli() + + (call,) = mock_run.call_args_list + return call + + +@pytest.mark.usefixtures("no_sleep") +class TestWindowsInstall: + """The PowerShell branch routes through the same validation.""" + + def test_rejects_injected_version_before_running_anything(self) -> None: + with ( + patch.object(download_cli.platform, "system", return_value="Windows"), + patch.object(download_cli.subprocess, "run") as mock_run, + patch.dict( + download_cli.os.environ, + {"CLAUDE_CLI_VERSION": "1.0.0; Write-Host pwned"}, + ), + pytest.raises(ValueError, match="Invalid CLAUDE_CLI_VERSION"), + ): + download_cli.download_cli() + + mock_run.assert_not_called() + + def test_valid_version_reaches_powershell_command(self) -> None: + call = _run_windows_download(DEV_VERSION) + cmd = call.args[0] + assert cmd[0] == "powershell" + assert cmd[-1].endswith(f"$env:{ENV_VAR}") + assert call.kwargs["stdin"] is subprocess.DEVNULL + + @pytest.mark.parametrize("version", ["1.2.3", DEV_VERSION]) + def test_version_is_never_in_the_powershell_command_text( + self, version: str + ) -> None: + """Regression guard: the version must reach PowerShell through the + environment, never spliced into the `-Command` string it parses.""" + call = _run_windows_download(version) + + for arg in call.args[0]: + assert version not in arg, f"version interpolated into {arg!r}" + + @pytest.mark.parametrize("version", ["1.2.3", DEV_VERSION]) + def test_version_is_carried_in_the_environment(self, version: str) -> None: + call = _run_windows_download(version) + + env = call.kwargs["env"] + assert env is not None, "PowerShell must be given an explicit environment" + assert env[ENV_VAR] == version + # The child still needs PATH, SystemRoot, etc. to run at all. + assert "CLAUDE_CLI_VERSION" in env + + def test_command_references_the_env_var_it_sets(self) -> None: + """The name in the command text and the name in the environment are the + same constant, so a rename cannot desynchronize them silently.""" + call = _run_windows_download("1.2.3") + assert f"$env:{ENV_VAR}" in call.args[0][-1] + assert ENV_VAR in call.kwargs["env"] + + def test_latest_uses_plain_installer(self) -> None: + call = _run_windows_download("latest") + assert call.args[0][-1] == "irm https://claude.ai/install.ps1 | iex" + + def test_latest_passes_no_version_argument(self) -> None: + """`latest` invokes the installer with no argument at all -- not with an + empty string -- and sets no version in the child's environment.""" + call = _run_windows_download("latest") + + command = call.args[0][-1] + assert "scriptblock" not in command + assert "$env:" not in command + # env=None inherits ours, so no version is injected there either. + assert call.kwargs["env"] is None From 118bf7c9e3c7ed86d6aa98fea5465e46c2e988b0 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Tue, 7 Jul 2026 02:36:03 +0000 Subject: [PATCH 02/13] Validate the version argument to update_cli_version.py Reject any version that is not a concrete CLI version before the file is read or written, and build the replacement with a callable so re.sub does not escape-process it. Move the version pattern and its validation helper into scripts/_cli_version_validation.py, shared with download_cli.py. --- scripts/_cli_version_validation.py | 69 +++++++ scripts/download_cli.py | 37 ++-- scripts/update_cli_version.py | 74 ++++++-- tests/test_download_cli.py | 22 +++ tests/test_update_cli_version.py | 279 +++++++++++++++++++++++++++++ 5 files changed, 446 insertions(+), 35 deletions(-) create mode 100644 scripts/_cli_version_validation.py create mode 100644 tests/test_update_cli_version.py diff --git a/scripts/_cli_version_validation.py b/scripts/_cli_version_validation.py new file mode 100644 index 000000000..dbe9b75ac --- /dev/null +++ b/scripts/_cli_version_validation.py @@ -0,0 +1,69 @@ +r"""Shared validation for Claude Code CLI version strings. + +Two scripts constrain the same value: update_cli_version.py writes it into +src/claude_agent_sdk/_cli_version.py, and download_cli.py (reached from +build_wheel.py) reads it back out and hands it to an installer. A second copy +of the rule would let the writer emit a value the reader rejects, so the +pattern and its validation helper live here once. + +A version must start with an alphanumeric character (so flag-shaped values +like "--help" are rejected) and may then contain only characters that appear +in real CLI versions, including dev builds such as +"2.1.146-dev.20260519.t105443.shaece3dab". + +This allowlist is a security boundary, not just input hygiene: + + * update_cli_version.update_cli_version() writes the version into a Python + string literal in a real source file, so it must never admit a double + quote, a backslash, or a newline. + * download_cli.download_cli() hands the version to an installer. Neither of + its paths interpolates it into a command string -- Unix passes it as its + own argv element, Windows passes it in the environment -- so for that + caller the allowlist is defense in depth rather than the only barrier. + +Widening it requires re-reading tests/test_download_cli.py::TestGetCliVersion +and tests/test_update_cli_version.py. + +Deliberately unanchored, and matched with fullmatch() rather than match(): +with "^...$" a swap to match() would silently accept a trailing newline +("1.0.0\n"); unanchored, the same swap accepts obvious prefixes like +"1.0.0; id" and fails immediately in tests. +""" + +import re + +VERSION_PATTERN = re.compile(r"[0-9A-Za-z][0-9A-Za-z.+-]*") + + +def validate_version(version: str, *, source: str, allow_latest: bool) -> str: + """Return ``version`` unchanged if it is a usable CLI version. + + Args: + version: The candidate version string. + source: Name of where the value came from, used in the error message + (e.g. "CLAUDE_CLI_VERSION"). + allow_latest: Whether the "latest" sentinel is acceptable. It is for a + download, which resolves it at install time; it is not for a value + pinned into _cli_version.py, which must name one concrete build. + + Raises: + ValueError: If ``version`` is neither an allowed "latest" nor a + fullmatch of VERSION_PATTERN. + """ + # "latest" fullmatches VERSION_PATTERN, so it has to be ruled out by name + # rather than by the pattern when it is not allowed. + if version == "latest": + if allow_latest: + return version + raise ValueError( + f"Invalid {source}: 'latest' is not a concrete version. " + f"Expected a version matching {VERSION_PATTERN.pattern}" + ) + + if not VERSION_PATTERN.fullmatch(version): + expected = "'latest' or a version" if allow_latest else "a concrete version" + raise ValueError( + f"Invalid {source}: {version!r}. " + f"Expected {expected} matching {VERSION_PATTERN.pattern}" + ) + return version diff --git a/scripts/download_cli.py b/scripts/download_cli.py index d2c082aaf..35d185d38 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -8,7 +8,6 @@ import os import platform import random -import re import shutil import subprocess import sys @@ -17,21 +16,18 @@ from collections.abc import Callable from pathlib import Path -# A version must start with an alphanumeric character (so flag-shaped values -# like "--help" are rejected) and may then contain only characters that appear -# in real CLI versions, including dev builds such as -# "2.1.146-dev.20260519.t105443.shaece3dab". -# -# This allowlist is defense in depth rather than the only thing standing between -# a hostile CLAUDE_CLI_VERSION and a shell: neither install path interpolates the -# version into a command string. Widening it requires re-reading -# tests/test_download_cli.py::TestGetCliVersion. -# -# Deliberately unanchored, and matched with fullmatch() rather than match(): -# with "^...$" a swap to match() would silently accept a trailing newline -# ("1.0.0\n"); unanchored, the same swap accepts obvious prefixes like -# "1.0.0; id" and fails immediately in tests. -VERSION_PATTERN = re.compile(r"[0-9A-Za-z][0-9A-Za-z.+-]*") +# scripts/ is not a package. Running this file directly -- as build_wheel.py +# does, via `python scripts/download_cli.py` -- already puts scripts/ on +# sys.path, but loading it by path (importlib.spec_from_file_location, as the +# tests do) does not. Add it either way so the shared module resolves. +_SCRIPTS_DIR = str(Path(__file__).parent) +if _SCRIPTS_DIR not in sys.path: + sys.path.insert(0, _SCRIPTS_DIR) + +import _cli_version_validation as version_validation # noqa: E402 + +# Re-exported: this module's callers and tests refer to download_cli.VERSION_PATTERN. +VERSION_PATTERN = version_validation.VERSION_PATTERN # The Windows installer reads the version out of the environment under this # name instead of having it spliced into the PowerShell command text. @@ -46,12 +42,9 @@ def get_cli_version() -> str: "latest" or a value matching VERSION_PATTERN. """ version = os.environ.get("CLAUDE_CLI_VERSION", "latest") - if version != "latest" and not VERSION_PATTERN.fullmatch(version): - raise ValueError( - f"Invalid CLAUDE_CLI_VERSION: {version!r}. " - f"Expected 'latest' or a version matching {VERSION_PATTERN.pattern}" - ) - return version + return version_validation.validate_version( + version, source="CLAUDE_CLI_VERSION", allow_latest=True + ) def find_installed_cli() -> Path | None: diff --git a/scripts/update_cli_version.py b/scripts/update_cli_version.py index 1ef17c7ee..123251831 100755 --- a/scripts/update_cli_version.py +++ b/scripts/update_cli_version.py @@ -1,32 +1,80 @@ #!/usr/bin/env python3 """Update Claude Code CLI version in _cli_version.py.""" +import json import re import sys from pathlib import Path +# scripts/ is not a package. Running this file directly -- `python +# scripts/update_cli_version.py 1.2.3` -- already puts scripts/ on sys.path, +# but loading it by path (importlib.spec_from_file_location, as the tests do) +# does not. Add it either way so the shared module resolves. +_SCRIPTS_DIR = str(Path(__file__).parent) +if _SCRIPTS_DIR not in sys.path: + sys.path.insert(0, _SCRIPTS_DIR) -def update_cli_version(new_version: str) -> None: - """Update CLI version in _cli_version.py.""" - # Update _cli_version.py - version_path = Path("src/claude_agent_sdk/_cli_version.py") +import _cli_version_validation as version_validation # noqa: E402 + +DEFAULT_VERSION_PATH = Path("src/claude_agent_sdk/_cli_version.py") + +# Matches the assignment build_wheel.get_bundled_cli_version() reads back. +ASSIGNMENT_PATTERN = re.compile(r'__cli_version__ = "[^"]+"') + + +def update_cli_version(new_version: str, version_path: Path | None = None) -> None: + """Update CLI version in _cli_version.py. + + Raises: + ValueError: If ``new_version`` is not a concrete version, or if the + target file has no ``__cli_version__`` assignment to replace. The + file is left untouched in both cases. + """ + # _cli_version.py is a real source file that gets imported, and the value + # written here is later read back by build_wheel.py and passed to + # download_cli.py. Validate before touching the file: an unvalidated value + # closes the string literal and injects arbitrary Python. "latest" is + # rejected -- unlike the download, which resolves it at install time, the + # pinned file has to name the one build that went into the wheels. + version_validation.validate_version( + new_version, source="CLI version", allow_latest=False + ) + + if version_path is None: + version_path = DEFAULT_VERSION_PATH content = version_path.read_text() - content = re.sub( - r'__cli_version__ = "[^"]+"', - f'__cli_version__ = "{new_version}"', - content, - count=1, - flags=re.MULTILINE, + # json.dumps() rather than an f-string: it always emits a closed, + # double-quoted, fully escaped literal, so a widened VERSION_PATTERN could + # never make this file unparseable or inject code. It is byte-identical to + # `"{new_version}"` for every version the pattern admits today. Note it is + # only a containment barrier: a version holding a quote would emit `"a\"b"`, + # which build_wheel.py's `"([^"]+)"` reader truncates to `a\` rather than + # round-tripping. repr() is not an option -- it emits single quotes, which + # that same reader would not match at all. + literal = f"__cli_version__ = {json.dumps(new_version)}" + + # A callable replacement, because re.sub() applies backslash-escape + # processing to a *string* replacement -- \1 and \g<0> expand, \n becomes a + # newline, and a bare trailing \ raises. A callable's return value is used + # literally. + new_content, count = ASSIGNMENT_PATTERN.subn( + lambda _match: literal, content, count=1 ) + if count != 1: + raise ValueError(f"No __cli_version__ assignment found in {version_path}") - version_path.write_text(content) + version_path.write_text(new_content) print(f"Updated {version_path} to {new_version}") if __name__ == "__main__": if len(sys.argv) != 2: - print("Usage: python scripts/update_cli_version.py ") + print("Usage: python scripts/update_cli_version.py ", file=sys.stderr) sys.exit(1) - update_cli_version(sys.argv[1]) + try: + update_cli_version(sys.argv[1]) + except ValueError as exc: + print(f"Error: {exc}", file=sys.stderr) + sys.exit(1) diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index e943baefd..34b83a931 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -1,6 +1,7 @@ """Tests for scripts/download_cli.py version validation and install invocation.""" import importlib.util +import os import subprocess import sys from collections.abc import Iterator @@ -105,6 +106,27 @@ def test_version_pattern_is_unanchored(self) -> None: assert "$" not in download_cli.VERSION_PATTERN.pattern assert not download_cli.VERSION_PATTERN.fullmatch("1.0.0\n") + def test_script_validates_when_run_directly(self, tmp_path: Path) -> None: + """build_wheel.py runs this file as a subprocess, so the shared + validation module must import without scripts/ being a package. + + PATH is emptied so that if validation ever regresses this reaches a + missing `curl` instead of really installing the CLI; sys.executable is + absolute, so python itself still starts. The timeout bounds the retry + sleeps on that path. + """ + script = Path(__file__).parent.parent / "scripts" / "download_cli.py" + result = subprocess.run( + [sys.executable, str(script)], + env=os.environ | {"CLAUDE_CLI_VERSION": "1.0.0; id", "PATH": str(tmp_path)}, + capture_output=True, + text=True, + timeout=60, + ) + assert result.returncode != 0 + assert "Invalid CLAUDE_CLI_VERSION" in result.stderr + assert "ModuleNotFoundError" not in result.stderr + @pytest.fixture def no_sleep() -> Iterator[None]: diff --git a/tests/test_update_cli_version.py b/tests/test_update_cli_version.py new file mode 100644 index 000000000..65519b908 --- /dev/null +++ b/tests/test_update_cli_version.py @@ -0,0 +1,279 @@ +"""Tests for scripts/update_cli_version.py version validation and file writing.""" + +import importlib.util +import re +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).parent.parent +SCRIPT_PATH = REPO_ROOT / "scripts" / "update_cli_version.py" + +# scripts/ is not a package, so load update_cli_version.py by path +_spec = importlib.util.spec_from_file_location("update_cli_version", SCRIPT_PATH) +assert _spec is not None and _spec.loader is not None +update_cli_version = importlib.util.module_from_spec(_spec) +sys.modules["update_cli_version"] = update_cli_version +_spec.loader.exec_module(update_cli_version) + +DEV_VERSION = "2.1.146-dev.20260519.t105443.shaece3dab" + +ORIGINAL = '"""Bundled Claude Code CLI version."""\n\n__cli_version__ = "2.1.195"\n' + + +@pytest.fixture +def version_file(tmp_path: Path) -> Path: + """A stand-in for src/claude_agent_sdk/_cli_version.py.""" + path = tmp_path / "_cli_version.py" + path.write_text(ORIGINAL) + return path + + +def import_version(path: Path) -> str: + """Import the written file as Python and return its __cli_version__. + + Fails on a file that is not valid Python, which is the whole point: the + value goes into a real source file that later gets imported. + """ + spec = importlib.util.spec_from_file_location(f"_written_{path.stem}", path) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + version: str = module.__cli_version__ + return version + + +class TestAcceptedVersions: + """Concrete versions round-trip through the file unchanged.""" + + @pytest.mark.parametrize( + "version", + [ + "1.2.3", + "2.1.195", + "2.1.201", + DEV_VERSION, + "1.2.3+build.4", + "0", + ], + ) + def test_round_trip(self, version_file: Path, version: str) -> None: + update_cli_version.update_cli_version(version, version_file) + assert import_version(version_file) == version + + def test_docstring_and_shape_preserved(self, version_file: Path) -> None: + update_cli_version.update_cli_version(DEV_VERSION, version_file) + assert version_file.read_text() == ORIGINAL.replace("2.1.195", DEV_VERSION) + + def test_only_first_assignment_replaced(self, tmp_path: Path) -> None: + path = tmp_path / "_cli_version.py" + path.write_text('__cli_version__ = "1.0.0"\n_other = "1.0.0"\n') + update_cli_version.update_cli_version("2.0.0", path) + assert path.read_text() == '__cli_version__ = "2.0.0"\n_other = "1.0.0"\n' + + +class TestRejectedVersions: + """Invalid input raises before the file is touched.""" + + @pytest.mark.parametrize( + "version", + [ + # Quote breakout: closes the string literal in a real source file. + '1.0.0"', + '1.0.0" + __import__("os").system("id") + "', + '"', + # Backslashes: re.sub() replacement-escape processing, and invalid + # Python escapes in the emitted literal. + "1.0.0\\", + "\\", + "1.0.0\\1", + "1.0.0\\g<0>", + "\\g<0>", + "1.0.0\\n", + # Newlines: an extra source line in the generated module. + "1.0.0\n", + "1.0.0\nimport os", + "\n", + # Empty and flag-shaped. + "", + "-1.2.3", + "--help", + "-s", + # Leftovers from the shell-injection allowlist. + "1.0.0; id", + "$(id)", + "`id`", + "1.0.0 2.0.0", + " 1.0.0", + "../../etc/passwd", + ".1.2.3", + ], + ) + def test_rejected_and_file_untouched( + self, version_file: Path, version: str + ) -> None: + with pytest.raises(ValueError, match="Invalid CLI version"): + update_cli_version.update_cli_version(version, version_file) + assert version_file.read_text() == ORIGINAL + + def test_latest_is_rejected(self, version_file: Path) -> None: + """_cli_version.py must name one concrete build, never "latest". + + build-and-publish.yml builds five wheels on five runners, each of which + independently runs build_wheel.py -> download_cli.py with this value. + "latest" would let the runners resolve different CLI builds into wheels + published under a single SDK version, and would leave _cli_version.py -- + the only record of what shipped -- naming nothing. download_cli.py + accepts "latest" because CLAUDE_CLI_VERSION defaults to it for + unpinned local builds; that is a different question from what may be + pinned into the file. + """ + with pytest.raises(ValueError, match="Invalid CLI version"): + update_cli_version.update_cli_version("latest", version_file) + assert version_file.read_text() == ORIGINAL + + def test_error_names_the_offending_value(self, version_file: Path) -> None: + with pytest.raises(ValueError) as excinfo: + update_cli_version.update_cli_version('1.0.0"; id', version_file) + assert '1.0.0"; id' in str(excinfo.value) + + def test_missing_assignment_leaves_file_untouched(self, tmp_path: Path) -> None: + path = tmp_path / "_cli_version.py" + path.write_text("# no assignment here\n") + with pytest.raises(ValueError, match="No __cli_version__ assignment"): + update_cli_version.update_cli_version("1.2.3", path) + assert path.read_text() == "# no assignment here\n" + + +class TestReplacementIsLiteral: + """The version reaches the file verbatim, with no escape interpretation. + + Validation already excludes every character these tests use, so they stub + it out to exercise the write path directly. Without that, a plain-string + re.sub() replacement -- which expands \\1 and \\g<0>, turns \\n into a + newline, and raises on a bare trailing backslash -- would look identical to + the callable replacement for all reachable input, and the guard would be + unfalsifiable. This is the test that fails if someone swaps the callable + back for an f-string. + """ + + @pytest.fixture + def no_validation(self, monkeypatch: pytest.MonkeyPatch) -> None: + def accept(version: str, *, source: str, allow_latest: bool) -> str: + return version + + monkeypatch.setattr( + update_cli_version.version_validation, "validate_version", accept + ) + + @pytest.mark.usefixtures("no_validation") + @pytest.mark.parametrize( + "version", + [ + "2.0.0\\1", + "2.0.0\\g<0>", + "2.0.0\\", + "\\g<0>\\1", + '2.0.0"', + "2.0.0\\n", + "2.0.0\nid", + ], + ) + def test_written_file_imports_back_to_the_exact_string( + self, version_file: Path, version: str + ) -> None: + update_cli_version.update_cli_version(version, version_file) + assert import_version(version_file) == version + + @pytest.mark.usefixtures("no_validation") + def test_backreference_is_not_expanded_into_the_file( + self, version_file: Path + ) -> None: + """A string replacement would splice the matched assignment into itself.""" + update_cli_version.update_cli_version("2.0.0\\g<0>", version_file) + text = version_file.read_text() + assert text.count("__cli_version__") == 1 + + def test_replacement_is_a_callable( + self, monkeypatch: pytest.MonkeyPatch, version_file: Path + ) -> None: + """Guard the mechanism itself, not only its observable output.""" + spy = _SubnSpy(update_cli_version.ASSIGNMENT_PATTERN) + monkeypatch.setattr(update_cli_version, "ASSIGNMENT_PATTERN", spy) + + update_cli_version.update_cli_version("1.2.3", version_file) + + (repl,) = spy.replacements + assert callable(repl), ( + f"re.sub replacement must be a callable to avoid escape processing, " + f"got {repl!r}" + ) + + +class _SubnSpy: + """Records the replacement handed to Pattern.subn(), then delegates.""" + + def __init__(self, pattern: re.Pattern[str]) -> None: + self._pattern = pattern + self.replacements: list[object] = [] + + def subn(self, repl: object, string: str, count: int = 0) -> tuple[str, int]: + self.replacements.append(repl) + return self._pattern.subn(repl, string, count=count) # type: ignore[arg-type] + + +class TestCommandLine: + """The script still works when run directly, with scripts/ not a package. + + Every case runs in a throwaway cwd holding a copy of the real + src/claude_agent_sdk/_cli_version.py layout, so a regression in the guard + corrupts the fixture rather than the repository. + """ + + @pytest.fixture + def cwd(self, tmp_path: Path) -> Path: + target = tmp_path / "src" / "claude_agent_sdk" / "_cli_version.py" + target.parent.mkdir(parents=True) + target.write_text(ORIGINAL) + return tmp_path + + def _run(self, cwd: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(SCRIPT_PATH), *args], + cwd=cwd, + capture_output=True, + text=True, + ) + + def _target(self, cwd: Path) -> Path: + return cwd / "src" / "claude_agent_sdk" / "_cli_version.py" + + def test_invalid_version_exits_nonzero_without_writing(self, cwd: Path) -> None: + result = self._run(cwd, '9.9.9"; import os') + + assert result.returncode == 1 + assert "Invalid CLI version" in result.stderr + assert result.stdout == "" + assert self._target(cwd).read_text() == ORIGINAL + + def test_latest_exits_nonzero_without_writing(self, cwd: Path) -> None: + result = self._run(cwd, "latest") + + assert result.returncode == 1 + assert "not a concrete version" in result.stderr + assert self._target(cwd).read_text() == ORIGINAL + + def test_missing_argument_prints_usage_to_stderr(self, cwd: Path) -> None: + result = self._run(cwd) + assert result.returncode == 1 + assert "Usage:" in result.stderr + + def test_writes_when_run_directly(self, cwd: Path) -> None: + """Direct execution must resolve the shared validation module.""" + result = self._run(cwd, DEV_VERSION) + + assert result.returncode == 0, result.stderr + assert "ModuleNotFoundError" not in result.stderr + assert import_version(self._target(cwd)) == DEV_VERSION From cd91b2526fd888bbfe2d89d297104310a0100ea6 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Tue, 7 Jul 2026 07:02:30 +0000 Subject: [PATCH 03/13] Fix two union-attr errors in download_cli.py retry loop retry_install() stashed the last CalledProcessError in an optional and read its stdout and stderr after the loop, where nothing proves range(1, 4) bound it. Report the failure from inside the handler for the final attempt instead, so the error is in scope and non-None by construction. mypy only runs over src/, so this never surfaced in CI. Adding scripts/ to that invocation needs the pre-existing errors in build_wheel.py and check_pypi_quota.py resolved first. --- scripts/download_cli.py | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/scripts/download_cli.py b/scripts/download_cli.py index 35d185d38..cfba1b872 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -33,6 +33,9 @@ # name instead of having it spliced into the PowerShell command text. INSTALL_VERSION_ENV_VAR = "CLAUDE_CLI_INSTALL_VERSION" +# How many times retry_install() runs an attempt before giving up. +MAX_INSTALL_ATTEMPTS = 3 + def get_cli_version() -> str: """Get the CLI version to download from environment or default. @@ -118,26 +121,32 @@ def retry_install(attempt: Callable[[], None]) -> None: # Small jitter to stagger parallel matrix builds hitting the same endpoint time.sleep(random.uniform(0, 5)) - last_err: subprocess.CalledProcessError | None = None - for attempt_num in range(1, 4): + # The failure is reported from inside the handler for the last attempt, so + # the error is in scope and non-None by construction. Stashing it in a + # `CalledProcessError | None` and reading it after the loop instead would + # rely on the reader -- and the type checker -- knowing that range(1, 4) + # cannot be empty. + for attempt_num in range(1, MAX_INSTALL_ATTEMPTS + 1): try: attempt() return except subprocess.CalledProcessError as e: - last_err = e - if attempt_num < 3: - delay = 2**attempt_num + if attempt_num == MAX_INSTALL_ATTEMPTS: print( - f"Install attempt {attempt_num} failed (exit {e.returncode}), " - f"retrying in {delay}s...", + f"Error downloading CLI after {MAX_INSTALL_ATTEMPTS} attempts: {e}", file=sys.stderr, ) - time.sleep(delay) - - print(f"Error downloading CLI after 3 attempts: {last_err}", file=sys.stderr) - print(f"stdout: {last_err.stdout.decode()}", file=sys.stderr) - print(f"stderr: {last_err.stderr.decode()}", file=sys.stderr) - sys.exit(1) + print(f"stdout: {e.stdout.decode()}", file=sys.stderr) + print(f"stderr: {e.stderr.decode()}", file=sys.stderr) + sys.exit(1) + + delay = 2**attempt_num + print( + f"Install attempt {attempt_num} failed (exit {e.returncode}), " + f"retrying in {delay}s...", + file=sys.stderr, + ) + time.sleep(delay) def download_cli() -> None: From 6764d44b533d495dcea9274b896e778c029a1c41 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Tue, 7 Jul 2026 07:02:52 +0000 Subject: [PATCH 04/13] Fail CI when the CLI install pipeline fails The two `curl -fsSL https://claude.ai/install.sh | bash` steps ran under the runner's default `bash -e`, which has no pipefail: the step's status came from `bash`, and `bash` reading an empty script exits 0. A failed download left the CLI missing and the step green, and the job only fell over later. Set `shell: bash` on both steps -- GitHub runs that with `--noprofile --norc -eo pipefail` -- rather than adding `set -o pipefail` by hand, so the option cannot be dropped by a later edit to the script body. These are the only two piped downloads in .github/workflows/. --- .github/workflows/test.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 67aa88740..17b391b19 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -60,6 +60,10 @@ jobs: - name: Install Claude Code (Linux/macOS) if: runner.os == 'Linux' || runner.os == 'macOS' + # `shell: bash` runs with -eo pipefail. Without pipefail the step takes + # its status from `bash`, which exits 0 on an empty script, so a failed + # `curl` leaves the CLI uninstalled and the step green. + shell: bash run: | curl -fsSL https://claude.ai/install.sh | bash echo "$HOME/.local/bin" >> $GITHUB_PATH @@ -149,6 +153,8 @@ jobs: - name: Install Claude Code (Linux) if: runner.os == 'Linux' + # `shell: bash` runs with -eo pipefail; see the note in the test job. + shell: bash run: | curl -fsSL https://claude.ai/install.sh | bash echo "$HOME/.local/bin" >> $GITHUB_PATH From 7d23db2f9c0f712bc17aedee6eae825682d4bab2 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Tue, 7 Jul 2026 07:05:22 +0000 Subject: [PATCH 05/13] Lint and typecheck scripts/ in CI The ruff and mypy steps only covered src/ and tests/, so scripts/ was never checked and had accumulated type errors. Add it to all three invocations and fix what mypy found: - twine is imported under try/except ImportError in build_wheel.py and is not a declared dependency, so add an ignore_missing_imports override for it, matching the existing opentelemetry one. - check_pypi_quota.py: give dict its type arguments, cast the json.load result rather than returning Any, and use a separate float local in human() instead of rebinding its int parameter. All three are pure typing changes; output is unchanged. scripts/ already passes ruff check and ruff format --check as-is, so no formatting changes were needed. --- .github/workflows/lint.yml | 6 +++--- pyproject.toml | 6 ++++++ scripts/check_pypi_quota.py | 14 ++++++++------ 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 45fdf805e..d605e1f9c 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -25,9 +25,9 @@ jobs: - name: Run ruff run: | - ruff check src/ tests/ - ruff format --check src/ tests/ + ruff check src/ tests/ scripts/ + ruff format --check src/ tests/ scripts/ - name: Run mypy run: | - mypy src/ \ No newline at end of file + mypy src/ scripts/ \ No newline at end of file diff --git a/pyproject.toml b/pyproject.toml index 8528699f4..5ce6db6ad 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -101,6 +101,12 @@ strict_equality = true module = ["opentelemetry", "opentelemetry.*"] ignore_missing_imports = true +[[tool.mypy.overrides]] +# twine is not a declared dependency (not even in the [dev] extra); the +# import in scripts/build_wheel.py is guarded by try/except ImportError. +module = ["twine"] +ignore_missing_imports = true + [tool.ruff] target-version = "py310" line-length = 88 diff --git a/scripts/check_pypi_quota.py b/scripts/check_pypi_quota.py index 394126af9..70e310a77 100755 --- a/scripts/check_pypi_quota.py +++ b/scripts/check_pypi_quota.py @@ -15,12 +15,13 @@ import sys import urllib.request from pathlib import Path +from typing import Any, cast PYPI_PROJECT_LIMIT_BYTES = 50 * 1024**3 # 50 GiB (increased from PyPI default 10 GiB) PYPI_FILE_LIMIT_BYTES = 100 * 1024**2 # 100 MiB -def fetch_project_files(package: str) -> list[dict]: +def fetch_project_files(package: str) -> list[dict[str, Any]]: req = urllib.request.Request( f"https://pypi.org/simple/{package}/", headers={ @@ -30,15 +31,16 @@ def fetch_project_files(package: str) -> list[dict]: ) with urllib.request.urlopen(req, timeout=30) as resp: data = json.load(resp) - return data.get("files", []) + return cast("list[dict[str, Any]]", data.get("files", [])) def human(n: int) -> str: + size = float(n) for unit in ("B", "KiB", "MiB", "GiB", "TiB"): - if abs(n) < 1024 or unit == "TiB": - return f"{n:.2f} {unit}" - n /= 1024 - return f"{n:.2f} TiB" + if abs(size) < 1024 or unit == "TiB": + return f"{size:.2f} {unit}" + size /= 1024 + return f"{size:.2f} TiB" def main() -> int: From d4a65299a4ee1bc841273d60253ee6cacaa3f24c Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Tue, 7 Jul 2026 07:15:08 +0000 Subject: [PATCH 06/13] Exit cleanly on an invalid CLAUDE_CLI_VERSION get_cli_version() raises ValueError when CLAUDE_CLI_VERSION is neither "latest" nor a value matching VERSION_PATTERN. Nothing caught it, so running the script -- which is how build_wheel.py invokes it -- ended in a traceback rather than the one-line stderr message and exit 1 that every other failure in this file produces (the curl-exhaustion path in retry_install(), the shebang check in check_install_script(), and the missing-binary path in copy_cli_to_bundle()). Catch ValueError at the entry point and report it the same way, mirroring what update_cli_version.py's __main__ already does. The shared validator in _cli_version_validation.py keeps raising: it is library code, and both update_cli_version.py and the tests read the exception. get_cli_version() keeps raising too, so what is accepted and what is rejected is unchanged. Add subprocess tests over the command-line surface: a rejected version exits 1, names the offending value on a line of its own, and prints no traceback. The value check is anchored to the start of a line because an uncaught raise renders the same text under "ValueError: ", which contains "Error: " as a substring and would satisfy a plain containment check. Validation runs before any curl or installer, so these tests reach no network. The existing tests that call get_cli_version() and download_cli() directly still assert ValueError. --- scripts/download_cli.py | 12 ++++++++- tests/test_download_cli.py | 55 +++++++++++++++++++++++++++++++++++--- 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/scripts/download_cli.py b/scripts/download_cli.py index cfba1b872..55f071ff0 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -276,4 +276,14 @@ def main() -> None: if __name__ == "__main__": - main() + # get_cli_version() raises on a bad CLAUDE_CLI_VERSION, and this script + # runs as a build step. Report that the way every other failure here is + # reported -- one line on stderr, exit 1 -- instead of letting a traceback + # out. The shared validator keeps raising, because update_cli_version.py + # and the tests read the exception; only the entry point turns it into an + # exit status. Matches update_cli_version.py's __main__. + try: + main() + except ValueError as exc: + print(f"Error: {exc}", file=sys.stderr) + sys.exit(1) diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index 34b83a931..9ede6c86c 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -11,11 +11,10 @@ import pytest +SCRIPT_PATH = Path(__file__).parent.parent / "scripts" / "download_cli.py" + # scripts/ is not a package, so load download_cli.py by path -_spec = importlib.util.spec_from_file_location( - "download_cli", - Path(__file__).parent.parent / "scripts" / "download_cli.py", -) +_spec = importlib.util.spec_from_file_location("download_cli", SCRIPT_PATH) assert _spec is not None and _spec.loader is not None download_cli = importlib.util.module_from_spec(_spec) sys.modules["download_cli"] = download_cli @@ -389,3 +388,51 @@ def test_latest_passes_no_version_argument(self) -> None: assert "$env:" not in command # env=None inherits ours, so no version is injected there either. assert call.kwargs["env"] is None + + +class TestCommandLine: + """The script itself reports a bad version rather than raising through. + + build_wheel.py runs this file as `python scripts/download_cli.py`, so an + uncaught ValueError would surface as a traceback in a build log. Only the + rejected path is exercised here: it fails in get_cli_version() before any + curl or installer runs, so the test touches no network. + """ + + def _run(self, version: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(SCRIPT_PATH)], + env={**os.environ, "CLAUDE_CLI_VERSION": version}, + capture_output=True, + text=True, + ) + + def test_invalid_version_exits_nonzero_without_a_traceback(self) -> None: + result = self._run("1.0.0; id") + + assert result.returncode == 1 + assert "Invalid CLAUDE_CLI_VERSION" in result.stderr + assert "Traceback" not in result.stderr + + def test_error_names_the_offending_value(self) -> None: + """Named in the one-line message, not merely in a traceback frame. + + Anchored to the start of a line: an uncaught raise renders the same + text under `ValueError: `, which *contains* `Error: ` as a substring, + so a plain `in` check would pass on the very traceback this guards + against. Scanning lines rather than stderr as a whole also keeps it + immune to any warning Python prints first. + """ + stderr = self._run("1.0.0; id").stderr + + (error_line,) = [ + line for line in stderr.splitlines() if line.startswith("Error: ") + ] + assert "1.0.0; id" in error_line + + def test_nothing_is_installed_before_the_version_is_rejected(self) -> None: + """The banner prints, then it bails -- no download is announced.""" + result = self._run("--help") + + assert result.returncode == 1 + assert "Downloading Claude Code CLI version" not in result.stdout From a6a912af7a5b80b7c5675ec936ffad43b8a3cbda Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Mon, 13 Jul 2026 22:52:43 +0000 Subject: [PATCH 07/13] Match the installer's version grammar, and stop retrying its rejections The CLI version allowlist was much looser than the grammar of the installer it feeds. install.sh enforces ^(stable|latest|[0-9]+\.[0-9]+\.[0-9]+(-[^[:space:]]+)?)$ (install.ps1 agrees), while our pattern accepted any alphanumeric-led run of [0-9A-Za-z.+-]. Everything the installer rejects but we accept is an error deferred to install time, where it surfaces ~11s later behind "Error downloading CLI after 3 attempts" -- which reads like a network failure. The dangerous case is "stable". It passed our validator *and* the installer, so `update_cli_version.py stable` wrote `__cli_version__ = "stable"` and the wheel build succeeded, silently pinning a moving dist-tag. That defeats the point of rejecting "latest" for a pin: _cli_version.py is the only record of which build went into the wheels, so it has to name one concrete build. "v2.1.207", "next", "beta" and "LATEST" were accepted as "concrete versions" too. So: * VERSION_PATTERN now mirrors the installer's concrete-version rule. The suffix is deliberately narrower than the installer's `-[^\s]+`, which would admit quotes, semicolons and backslashes -- we accept a strict subset, so the pattern remains the security boundary that keeps a version out of a Python string literal and off a command line. Verified against every version this repo has ever pinned, including dev builds like 2.1.146-dev.20260519.t105443.shaece3dab. * "latest" and "stable" are handled as what they are: moving dist-tags, matched case-insensitively and normalized. Allowed for a download, rejected for a pin. allow_latest becomes allow_dist_tag. * A leading "v" and an unsupported dist-tag each get an error that says what to type instead, rather than printing a regex. * Surrounding whitespace is stripped before validating, and the stripped value is what gets used -- a trailing newline from a file read or a CRLF checkout is unambiguous in intent. * A `Usage:`-style rejection from the installer is no longer retried. Like the shebang check, the verdict is deterministic: retrying only burns the backoff and then misreports it as a download failure. :house: Remote-Dev: homespace --- scripts/_cli_version_validation.py | 125 +++++++++++++++----- scripts/download_cli.py | 65 +++++++++-- scripts/update_cli_version.py | 12 +- tests/test_download_cli.py | 180 ++++++++++++++++++++++++++++- tests/test_update_cli_version.py | 94 ++++++++++++--- 5 files changed, 413 insertions(+), 63 deletions(-) diff --git a/scripts/_cli_version_validation.py b/scripts/_cli_version_validation.py index dbe9b75ac..0109edc77 100644 --- a/scripts/_cli_version_validation.py +++ b/scripts/_cli_version_validation.py @@ -6,12 +6,25 @@ of the rule would let the writer emit a value the reader rejects, so the pattern and its validation helper live here once. -A version must start with an alphanumeric character (so flag-shaped values -like "--help" are rejected) and may then contain only characters that appear -in real CLI versions, including dev builds such as -"2.1.146-dev.20260519.t105443.shaece3dab". +The installer is the authority on what a version may be. install.sh enforces -This allowlist is a security boundary, not just input hygiene: + ^(stable|latest|[0-9]+\.[0-9]+\.[0-9]+(-[^[:space:]]+)?)$ + +and install.ps1 enforces the same rule, so a value this module admits but the +installer does not is not a version -- it is an error we defer to install time, +where it surfaces behind a retry loop and a misleading "Error downloading CLI" +headline. VERSION_PATTERN therefore mirrors that grammar: three dot-separated +numeric components with an optional prerelease/build suffix, which covers both +releases ("2.1.207") and dev builds +("2.1.146-dev.20260519.t105443.shaece3dab"). + +We deliberately accept a strict *subset* of what the installer allows: the +installer's suffix is `-[^\s]+`, which would admit quotes, backslashes, +semicolons and every other non-space character, so the suffix here is narrowed +to the alphanumeric/dot/plus/hyphen set that real versions use. Never widen +this pattern back toward the installer's. + +That narrowing is a security boundary, not just input hygiene: * update_cli_version.update_cli_version() writes the version into a Python string literal in a real source file, so it must never admit a double @@ -21,49 +34,105 @@ own argv element, Windows passes it in the environment -- so for that caller the allowlist is defense in depth rather than the only barrier. -Widening it requires re-reading tests/test_download_cli.py::TestGetCliVersion -and tests/test_update_cli_version.py. +"latest" and "stable" are the installer's dist-tags. Both are *moving*: they +resolve to whatever build is current at install time. That is fine for a +download, and wrong for a pin -- _cli_version.py is the only record of which +build went into the wheels, so it must name one concrete build. Hence +``allow_dist_tag``. -Deliberately unanchored, and matched with fullmatch() rather than match(): -with "^...$" a swap to match() would silently accept a trailing newline -("1.0.0\n"); unanchored, the same swap accepts obvious prefixes like -"1.0.0; id" and fails immediately in tests. +Widening any of this requires re-reading tests/test_download_cli.py and +tests/test_update_cli_version.py. + +VERSION_PATTERN is deliberately unanchored, and matched with fullmatch() +rather than match(): with "^...$" a swap to match() would silently accept a +trailing newline ("1.0.0\n"); unanchored, the same swap accepts obvious +prefixes like "1.0.0; id" and fails immediately in tests. """ import re -VERSION_PATTERN = re.compile(r"[0-9A-Za-z][0-9A-Za-z.+-]*") +# A concrete version: MAJOR.MINOR.PATCH with an optional suffix. The suffix is +# the installer's `-[^\s]+` narrowed to characters that appear in real +# versions -- see the module docstring. +VERSION_PATTERN = re.compile(r"[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z.+-]+)?") + +# The moving tags the installer resolves at install time. Compared lowercased, +# so "LATEST" is the sentinel rather than a mysterious "concrete version". +DIST_TAGS = ("latest", "stable") +# Anything word-shaped that is not a version: "next", "beta", "nightly". Named +# so the error can say *why* it was rejected instead of printing a regex. +_DIST_TAG_SHAPED = re.compile(r"[A-Za-z][0-9A-Za-z-]*") -def validate_version(version: str, *, source: str, allow_latest: bool) -> str: - """Return ``version`` unchanged if it is a usable CLI version. +_SUPPORTED_TAGS = ", ".join(repr(tag) for tag in DIST_TAGS) + + +def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: + """Return the usable form of ``version``, or raise. + + Surrounding whitespace is stripped before anything else: a trailing "\\n" + from a file read, a "\\r" from a CRLF checkout, or a stray space from YAML + is unambiguous in intent, and the stripped value is what the caller gets + back and must use downstream. Args: version: The candidate version string. source: Name of where the value came from, used in the error message (e.g. "CLAUDE_CLI_VERSION"). - allow_latest: Whether the "latest" sentinel is acceptable. It is for a - download, which resolves it at install time; it is not for a value - pinned into _cli_version.py, which must name one concrete build. + allow_dist_tag: Whether a moving dist-tag ("latest", "stable") is + acceptable. It is for a download, which resolves it at install + time; it is not for a value pinned into _cli_version.py, which must + name the one concrete build that went into the wheels. + + Returns: + The stripped version, with a dist-tag normalized to lowercase. Raises: - ValueError: If ``version`` is neither an allowed "latest" nor a + ValueError: If ``version`` is neither an allowed dist-tag nor a fullmatch of VERSION_PATTERN. """ - # "latest" fullmatches VERSION_PATTERN, so it has to be ruled out by name - # rather than by the pattern when it is not allowed. - if version == "latest": - if allow_latest: - return version + candidate = version.strip() + + # A dist-tag fails VERSION_PATTERN, so it is recognized by name -- and + # case-insensitively, so "LATEST" is not mistaken for something else. + if candidate.lower() in DIST_TAGS: + if allow_dist_tag: + return candidate.lower() raise ValueError( - f"Invalid {source}: 'latest' is not a concrete version. " - f"Expected a version matching {VERSION_PATTERN.pattern}" + f"Invalid {source}: {candidate!r} is a moving dist-tag, not a concrete " + f"version. A pinned version must name the one build that goes into the " + f"wheels. Expected a version matching {VERSION_PATTERN.pattern}" ) - if not VERSION_PATTERN.fullmatch(version): - expected = "'latest' or a version" if allow_latest else "a concrete version" + # "v2.1.207" is the single most likely typo, and the installer rejects it. + # Say so, rather than printing the pattern and leaving the reader to spot + # the leading "v". Not normalized away: the caller asked for something we + # do not support, and silently installing a different string is worse. + if candidate[:1] in ("v", "V") and VERSION_PATTERN.fullmatch(candidate[1:]): + raise ValueError( + f"Invalid {source}: {candidate!r}. " + f"Did you mean {candidate[1:]!r}? (no leading 'v')" + ) + + if _DIST_TAG_SHAPED.fullmatch(candidate): + expected = ( + f"{_SUPPORTED_TAGS}, or a concrete version" + if allow_dist_tag + else "a concrete version" + ) + raise ValueError( + f"Invalid {source}: {candidate!r} is not a supported dist-tag; " + f"use {expected}" + ) + + if not VERSION_PATTERN.fullmatch(candidate): + expected = ( + f"{_SUPPORTED_TAGS}, or a version" + if allow_dist_tag + else "a concrete version" + ) raise ValueError( f"Invalid {source}: {version!r}. " f"Expected {expected} matching {VERSION_PATTERN.pattern}" ) - return version + return candidate diff --git a/scripts/download_cli.py b/scripts/download_cli.py index 55f071ff0..1efe67199 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -15,6 +15,7 @@ import time from collections.abc import Callable from pathlib import Path +from typing import NoReturn # scripts/ is not a package. Running this file directly -- as build_wheel.py # does, via `python scripts/download_cli.py` -- already puts scripts/ on @@ -40,13 +41,16 @@ def get_cli_version() -> str: """Get the CLI version to download from environment or default. + Returns the stripped, dist-tag-normalized value -- use it, rather than the + raw environment string, for everything downstream. + Raises: - ValueError: If CLAUDE_CLI_VERSION is set to something other than - "latest" or a value matching VERSION_PATTERN. + ValueError: If CLAUDE_CLI_VERSION is set to something other than a + dist-tag ("latest", "stable") or a value matching VERSION_PATTERN. """ version = os.environ.get("CLAUDE_CLI_VERSION", "latest") return version_validation.validate_version( - version, source="CLAUDE_CLI_VERSION", allow_latest=True + version, source="CLAUDE_CLI_VERSION", allow_dist_tag=True ) @@ -116,6 +120,42 @@ def check_install_script(script_path: str) -> None: sys.exit(1) +def _decode(stream: bytes | str | None) -> str: + """One captured stream as text. run_command() captures bytes; be tolerant.""" + if stream is None: + return "" + if isinstance(stream, bytes): + return stream.decode(errors="replace") + return stream + + +def _output_of(error: subprocess.CalledProcessError) -> str: + """The failed command's stdout and stderr, decoded, as one string.""" + return f"{_decode(error.stdout)}\n{_decode(error.stderr)}" + + +def is_argument_rejection(error: subprocess.CalledProcessError) -> bool: + """True when the installer rejected its arguments rather than failing to run. + + install.sh and install.ps1 validate the version they are handed against + their own grammar -- roughly `stable|latest|N.N.N(-suffix)?` -- and answer a + mismatch by printing a usage line and exiting. Like the shebang check, that + verdict is deterministic: the same arguments fail the same way every time, + so retrying only burns the backoff and then reports the deterministic + rejection as "Error downloading CLI after 3 attempts", which reads like a + network problem. Fail fast instead. + """ + return "usage:" in _output_of(error).lower() + + +def _fail(headline: str, error: subprocess.CalledProcessError) -> NoReturn: + """Report a failed install command and exit.""" + print(headline, file=sys.stderr) + print(f"stdout: {_decode(error.stdout)}", file=sys.stderr) + print(f"stderr: {_decode(error.stderr)}", file=sys.stderr) + sys.exit(1) + + def retry_install(attempt: Callable[[], None]) -> None: """Run an install attempt, retrying the whole attempt on command failure.""" # Small jitter to stagger parallel matrix builds hitting the same endpoint @@ -131,14 +171,19 @@ def retry_install(attempt: Callable[[], None]) -> None: attempt() return except subprocess.CalledProcessError as e: + if is_argument_rejection(e): + _fail( + f"Error: the installer rejected its arguments (exit " + f"{e.returncode}). This is not a network failure and will " + f"not succeed on a retry.", + e, + ) + if attempt_num == MAX_INSTALL_ATTEMPTS: - print( + _fail( f"Error downloading CLI after {MAX_INSTALL_ATTEMPTS} attempts: {e}", - file=sys.stderr, + e, ) - print(f"stdout: {e.stdout.decode()}", file=sys.stderr) - print(f"stderr: {e.stderr.decode()}", file=sys.stderr) - sys.exit(1) delay = 2**attempt_num print( @@ -164,6 +209,10 @@ def download_cli() -> None: # argument position expands to exactly one argument -- PowerShell does # not re-split or re-parse it -- which is the argv separation the Unix # path gets from `["bash", script, version]`. + # "latest" is the installer's own default, so it is expressed by + # passing no argument at all. Every other accepted value -- a concrete + # version, or the "stable" dist-tag -- is passed through the same + # argument path. install_env: dict[str, str] | None = None if version == "latest": install_cmd = [ diff --git a/scripts/update_cli_version.py b/scripts/update_cli_version.py index 123251831..40ca57acb 100755 --- a/scripts/update_cli_version.py +++ b/scripts/update_cli_version.py @@ -33,11 +33,13 @@ def update_cli_version(new_version: str, version_path: Path | None = None) -> No # _cli_version.py is a real source file that gets imported, and the value # written here is later read back by build_wheel.py and passed to # download_cli.py. Validate before touching the file: an unvalidated value - # closes the string literal and injects arbitrary Python. "latest" is - # rejected -- unlike the download, which resolves it at install time, the - # pinned file has to name the one build that went into the wheels. - version_validation.validate_version( - new_version, source="CLI version", allow_latest=False + # closes the string literal and injects arbitrary Python. The moving + # dist-tags ("latest", "stable") are rejected -- unlike the download, which + # resolves them at install time, the pinned file has to name the one build + # that went into the wheels. Write the *validated* value: it is the input + # with surrounding whitespace stripped. + new_version = version_validation.validate_version( + new_version, source="CLI version", allow_dist_tag=False ) if version_path is None: diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index 9ede6c86c..82969baca 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -25,7 +25,13 @@ class TestGetCliVersion: - """CLAUDE_CLI_VERSION must be 'latest' or a plain version token.""" + """CLAUDE_CLI_VERSION must be a dist-tag or a concrete version. + + The grammar mirrors the installer's own + (`stable|latest|N.N.N(-suffix)?`), narrowed: anything this accepts but the + installer rejects is an error deferred to install time, where it surfaces + behind a retry loop as a misleading download failure. + """ def test_default_is_latest(self, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("CLAUDE_CLI_VERSION", raising=False) @@ -35,17 +41,63 @@ def test_default_is_latest(self, monkeypatch: pytest.MonkeyPatch) -> None: "version", [ "latest", + "stable", "1.2.3", "2.1.195", DEV_VERSION, - "1.2.3+build.4", - "0", + "1.2.3-beta.1", + "1.2.3-rc.1+build.4", ], ) def test_accepted(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: monkeypatch.setenv("CLAUDE_CLI_VERSION", version) assert download_cli.get_cli_version() == version + def test_stable_is_a_downloadable_dist_tag( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """`stable` is a moving tag the installer resolves, like `latest`. + + Allowed here (a download resolves it at install time) and rejected by + update_cli_version.py (a pin must name one concrete build). + """ + monkeypatch.setenv("CLAUDE_CLI_VERSION", "stable") + assert download_cli.get_cli_version() == "stable" + + @pytest.mark.parametrize( + ("version", "expected"), + [ + ("LATEST", "latest"), + ("Latest", "latest"), + ("STABLE", "stable"), + ("Stable", "stable"), + ], + ) + def test_dist_tags_are_case_insensitive_and_normalized( + self, monkeypatch: pytest.MonkeyPatch, version: str, expected: str + ) -> None: + """ "LATEST" is the sentinel, not a mysterious "concrete version".""" + monkeypatch.setenv("CLAUDE_CLI_VERSION", version) + assert download_cli.get_cli_version() == expected + + @pytest.mark.parametrize( + ("version", "expected"), + [ + ("1.2.3\n", "1.2.3"), + ("1.2.3\r\n", "1.2.3"), + (" 1.2.3 ", "1.2.3"), + ("\tlatest\n", "latest"), + (f" {DEV_VERSION}\r\n", DEV_VERSION), + ], + ) + def test_surrounding_whitespace_is_stripped( + self, monkeypatch: pytest.MonkeyPatch, version: str, expected: str + ) -> None: + """A trailing newline from a file read or a CRLF checkout is + unambiguous in intent; the stripped value is what is used downstream.""" + monkeypatch.setenv("CLAUDE_CLI_VERSION", version) + assert download_cli.get_cli_version() == expected + @pytest.mark.parametrize( "version", [ @@ -57,12 +109,21 @@ def test_accepted(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: "1.0.0 && id", "1.0.0 | id", "1.0.0\nid", - "1.0.0\n", "1.0.0 2.0.0", "$VERSION", "../../etc/passwd", "", ".1.2.3", + # Not versions at all -- the old allowlist took these for concrete + # versions and let the installer reject them 11 seconds later. + "0", + "1.2", + "1.2.3.4", + "1.2.3+build.4", # the installer's grammar has no bare "+" suffix + "v2.1.207", + "next", + "beta", + "nightly", ], ) def test_rejected(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: @@ -70,6 +131,39 @@ def test_rejected(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: with pytest.raises(ValueError, match="Invalid CLAUDE_CLI_VERSION"): download_cli.get_cli_version() + @pytest.mark.parametrize("version", ["v2.1.207", "V1.2.3"]) + def test_leading_v_is_named_not_normalized( + self, monkeypatch: pytest.MonkeyPatch, version: str + ) -> None: + """The likeliest typo gets told what to type, and is never silently + rewritten into a different version than the caller asked for.""" + monkeypatch.setenv("CLAUDE_CLI_VERSION", version) + with pytest.raises(ValueError) as excinfo: + download_cli.get_cli_version() + + message = str(excinfo.value) + assert f"Did you mean {version[1:]!r}?" in message + assert "no leading 'v'" in message + + @pytest.mark.parametrize("version", ["next", "beta", "nightly"]) + def test_unsupported_dist_tag_is_named( + self, monkeypatch: pytest.MonkeyPatch, version: str + ) -> None: + monkeypatch.setenv("CLAUDE_CLI_VERSION", version) + with pytest.raises(ValueError) as excinfo: + download_cli.get_cli_version() + + message = str(excinfo.value) + assert "not a supported dist-tag" in message + assert "'latest'" in message + assert "'stable'" in message + + @pytest.mark.parametrize("version", ["2.1.207", DEV_VERSION, "1.0.0-alpha"]) + def test_pattern_admits_every_published_version_shape(self, version: str) -> None: + """The tightened pattern is about false *accepts*; it must not start + rejecting anything real, releases or dev builds.""" + assert download_cli.VERSION_PATTERN.fullmatch(version) + def test_error_names_the_offending_value( self, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -220,6 +314,13 @@ def test_latest_passes_no_version_argument(self) -> None: assert bash_cmd[0] == "bash" assert len(bash_cmd) == 2 + def test_stable_is_passed_as_an_argument(self) -> None: + """`latest` is the installer's default and so is expressed by passing + nothing; every other accepted value, `stable` included, is an argv + element the installer resolves.""" + _, bash_cmd = _run_unix_download("stable") + assert bash_cmd == ["bash", bash_cmd[1], "stable"] + @pytest.mark.parametrize("version", ["1.2.3", DEV_VERSION]) def test_version_is_its_own_argv_element(self, version: str) -> None: """Regression guard: the version must never be interpolated into a @@ -308,6 +409,77 @@ def fake_run(command: list[str], **kwargs: object) -> MagicMock: assert [call.args[0][0] for call in run.call_args_list] == ["curl"] * 3 +@pytest.mark.usefixtures("no_sleep") +class TestArgumentRejectionIsNotRetried: + """An installer that rejects its arguments is not a flaky network. + + The installer validates the version it is handed and answers a mismatch + with a usage line. Retrying that burns ~11s of backoff and then reports it + as "Error downloading CLI after 3 attempts", which reads like a download + failure. Same rationale as the shebang check: deterministic, so fail fast. + """ + + USAGE = b"Usage: install.sh [version|stable|latest]\n" + + def _run_with_failing_bash( + self, stderr: bytes = USAGE, stdout: bytes = b"" + ) -> list[list[str]]: + error = subprocess.CalledProcessError(1, ["bash"], output=stdout, stderr=stderr) + + def fake_run(command: list[str], **kwargs: object) -> MagicMock: + if command[0] == "curl": + Path(command[command.index("-o") + 1]).write_bytes(b"#!/bin/bash\n") + return MagicMock() + raise error + + with ( + patch.object(download_cli.platform, "system", return_value="Linux"), + patch.object(download_cli.subprocess, "run", side_effect=fake_run) as run, + patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), + pytest.raises(SystemExit) as excinfo, + ): + download_cli.download_cli() + + assert excinfo.value.code == 1 + return [call.args[0][0] for call in run.call_args_list] + + def test_usage_rejection_runs_bash_once( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + assert self._run_with_failing_bash() == ["curl", "bash"] + + err = capsys.readouterr().err + assert "rejected its arguments" in err + assert "not a network failure" in err + assert "after 3 attempts" not in err + + def test_usage_on_stdout_is_also_a_rejection(self) -> None: + """install.ps1 writes its usage line to stdout, not stderr.""" + assert self._run_with_failing_bash(stderr=b"", stdout=b"Usage: ...\n") == [ + "curl", + "bash", + ] + + def test_a_real_install_failure_still_retries(self) -> None: + """Only the usage verdict short-circuits; a genuine failure keeps its + three attempts.""" + commands = self._run_with_failing_bash(stderr=b"curl: (6) could not resolve\n") + assert commands == ["curl", "bash"] * download_cli.MAX_INSTALL_ATTEMPTS + + @pytest.mark.parametrize( + ("stderr", "expected"), + [ + (b"Usage: install.sh [version]", True), + (b"usage: install.ps1 ", True), + (b"error: unknown option", False), + (b"", False), + ], + ) + def test_is_argument_rejection(self, stderr: bytes, expected: bool) -> None: + error = subprocess.CalledProcessError(1, ["bash"], output=b"", stderr=stderr) + assert download_cli.is_argument_rejection(error) is expected + + def _run_windows_download(version: str) -> Any: """Run download_cli() on the Windows path, returning the single run() call.""" with ( diff --git a/tests/test_update_cli_version.py b/tests/test_update_cli_version.py index 65519b908..8889b03b2 100644 --- a/tests/test_update_cli_version.py +++ b/tests/test_update_cli_version.py @@ -55,14 +55,32 @@ class TestAcceptedVersions: "2.1.195", "2.1.201", DEV_VERSION, - "1.2.3+build.4", - "0", + "1.2.3-beta.1", + "1.2.3-rc.1+build.4", ], ) def test_round_trip(self, version_file: Path, version: str) -> None: update_cli_version.update_cli_version(version, version_file) assert import_version(version_file) == version + @pytest.mark.parametrize( + ("argument", "written"), + [ + ("1.2.3\n", "1.2.3"), + ("1.2.3\r\n", "1.2.3"), + (" 2.1.201 ", "2.1.201"), + (f"{DEV_VERSION}\n", DEV_VERSION), + ], + ) + def test_surrounding_whitespace_is_stripped_before_writing( + self, version_file: Path, argument: str, written: str + ) -> None: + """A version read out of a file arrives with a trailing newline. Strip + it -- and write the stripped value, not the raw one, so the newline + never lands inside the string literal.""" + update_cli_version.update_cli_version(argument, version_file) + assert import_version(version_file) == written + def test_docstring_and_shape_preserved(self, version_file: Path) -> None: update_cli_version.update_cli_version(DEV_VERSION, version_file) assert version_file.read_text() == ORIGINAL.replace("2.1.195", DEV_VERSION) @@ -92,8 +110,9 @@ class TestRejectedVersions: "1.0.0\\g<0>", "\\g<0>", "1.0.0\\n", - # Newlines: an extra source line in the generated module. - "1.0.0\n", + # Newlines *inside* the value: an extra source line in the + # generated module. (A merely trailing newline is stripped -- see + # TestAcceptedVersions.) "1.0.0\nimport os", "\n", # Empty and flag-shaped. @@ -106,9 +125,19 @@ class TestRejectedVersions: "$(id)", "`id`", "1.0.0 2.0.0", - " 1.0.0", + " 1.0.0 2.0.0 ", "../../etc/passwd", ".1.2.3", + # Not versions at all: the old allowlist took these for concrete + # versions and pinned them into the file, deferring the failure to + # the wheel build. + "0", + "1.2", + "1.2.3.4", + "1.2.3+build.4", + "v2.1.207", + "next", + "beta", ], ) def test_rejected_and_file_untouched( @@ -118,20 +147,39 @@ def test_rejected_and_file_untouched( update_cli_version.update_cli_version(version, version_file) assert version_file.read_text() == ORIGINAL - def test_latest_is_rejected(self, version_file: Path) -> None: - """_cli_version.py must name one concrete build, never "latest". + @pytest.mark.parametrize("tag", ["latest", "stable", "LATEST", "Stable"]) + def test_dist_tags_are_rejected(self, version_file: Path, tag: str) -> None: + """_cli_version.py must name one concrete build, never a moving tag. build-and-publish.yml builds five wheels on five runners, each of which independently runs build_wheel.py -> download_cli.py with this value. - "latest" would let the runners resolve different CLI builds into wheels - published under a single SDK version, and would leave _cli_version.py -- - the only record of what shipped -- naming nothing. download_cli.py - accepts "latest" because CLAUDE_CLI_VERSION defaults to it for - unpinned local builds; that is a different question from what may be - pinned into the file. + A dist-tag would let the runners resolve different CLI builds into + wheels published under a single SDK version, and would leave + _cli_version.py -- the only record of what shipped -- naming nothing. + "stable" is the dangerous one: it passes the installer too, so before + this guard it silently pinned a moving tag and the build *succeeded*. + download_cli.py accepts the tags because CLAUDE_CLI_VERSION defaults to + "latest" for unpinned local builds; that is a different question from + what may be pinned into the file. """ - with pytest.raises(ValueError, match="Invalid CLI version"): - update_cli_version.update_cli_version("latest", version_file) + with pytest.raises(ValueError, match="Invalid CLI version") as excinfo: + update_cli_version.update_cli_version(tag, version_file) + + assert "moving dist-tag" in str(excinfo.value) + assert "not a concrete version" in str(excinfo.value) + assert version_file.read_text() == ORIGINAL + + def test_leading_v_is_named_not_normalized(self, version_file: Path) -> None: + """It reached the file before, and failed at wheel-build time.""" + with pytest.raises(ValueError) as excinfo: + update_cli_version.update_cli_version("v2.1.207", version_file) + + assert "Did you mean '2.1.207'?" in str(excinfo.value) + assert version_file.read_text() == ORIGINAL + + def test_unsupported_dist_tag_is_named(self, version_file: Path) -> None: + with pytest.raises(ValueError, match="not a supported dist-tag"): + update_cli_version.update_cli_version("next", version_file) assert version_file.read_text() == ORIGINAL def test_error_names_the_offending_value(self, version_file: Path) -> None: @@ -161,7 +209,7 @@ class TestReplacementIsLiteral: @pytest.fixture def no_validation(self, monkeypatch: pytest.MonkeyPatch) -> None: - def accept(version: str, *, source: str, allow_latest: bool) -> str: + def accept(version: str, *, source: str, allow_dist_tag: bool) -> str: return version monkeypatch.setattr( @@ -258,13 +306,23 @@ def test_invalid_version_exits_nonzero_without_writing(self, cwd: Path) -> None: assert result.stdout == "" assert self._target(cwd).read_text() == ORIGINAL - def test_latest_exits_nonzero_without_writing(self, cwd: Path) -> None: - result = self._run(cwd, "latest") + @pytest.mark.parametrize("tag", ["latest", "stable"]) + def test_dist_tag_exits_nonzero_without_writing(self, cwd: Path, tag: str) -> None: + """`stable` used to pin cleanly and build cleanly -- against whatever + the tag happened to point at that day.""" + result = self._run(cwd, tag) assert result.returncode == 1 assert "not a concrete version" in result.stderr assert self._target(cwd).read_text() == ORIGINAL + def test_leading_v_exits_nonzero_without_writing(self, cwd: Path) -> None: + result = self._run(cwd, "v2.1.207") + + assert result.returncode == 1 + assert "Did you mean '2.1.207'?" in result.stderr + assert self._target(cwd).read_text() == ORIGINAL + def test_missing_argument_prints_usage_to_stderr(self, cwd: Path) -> None: result = self._run(cwd) assert result.returncode == 1 From 0b68217e66b9172f22f181dfad160a01ff1d77e9 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Mon, 13 Jul 2026 23:41:24 +0000 Subject: [PATCH 08/13] Simplify the version validator and its install-path tests Quality pass over the validation code, no behavior change: every input accepted or rejected before is accepted or rejected the same way after. - validate_version() now returns on a pattern match up front, so the three error branches read as "pick the most useful reason" rather than as a fallthrough. Their two near-identical "expected" phrases collapse into one _expected() helper, which also fixes an inconsistency: the generic-malformed message said "or a version" where the dist-tag branch already said "or a concrete version". - Inline _output_of() into is_argument_rejection(), its only caller, and fold the reason both streams are searched into the docstring. - Hoist the Unix install patch stack into a _unix_install() context manager. Six tests were each re-opening the same three patches by hand; they now differ only in the subprocess.run side effect. - Drop test_stable_is_a_downloadable_dist_tag: "stable" is already a case in test_accepted. Its rationale moves to a comment there. :house: Remote-Dev: homespace --- scripts/_cli_version_validation.py | 34 ++++++------- scripts/download_cli.py | 11 ++--- tests/test_download_cli.py | 78 ++++++++++++++---------------- 3 files changed, 57 insertions(+), 66 deletions(-) diff --git a/scripts/_cli_version_validation.py b/scripts/_cli_version_validation.py index 0109edc77..38ecabfcd 100644 --- a/scripts/_cli_version_validation.py +++ b/scripts/_cli_version_validation.py @@ -67,6 +67,13 @@ _SUPPORTED_TAGS = ", ".join(repr(tag) for tag in DIST_TAGS) +def _expected(allow_dist_tag: bool) -> str: + """The phrase naming what the caller should have passed instead.""" + if allow_dist_tag: + return f"{_SUPPORTED_TAGS}, or a concrete version" + return "a concrete version" + + def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: """Return the usable form of ``version``, or raise. @@ -104,6 +111,11 @@ def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: f"wheels. Expected a version matching {VERSION_PATTERN.pattern}" ) + if VERSION_PATTERN.fullmatch(candidate): + return candidate + + # Rejected from here on; what is left is choosing the most useful reason. + # "v2.1.207" is the single most likely typo, and the installer rejects it. # Say so, rather than printing the pattern and leaving the reader to spot # the leading "v". Not normalized away: the caller asked for something we @@ -115,24 +127,12 @@ def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: ) if _DIST_TAG_SHAPED.fullmatch(candidate): - expected = ( - f"{_SUPPORTED_TAGS}, or a concrete version" - if allow_dist_tag - else "a concrete version" - ) raise ValueError( f"Invalid {source}: {candidate!r} is not a supported dist-tag; " - f"use {expected}" + f"use {_expected(allow_dist_tag)}" ) - if not VERSION_PATTERN.fullmatch(candidate): - expected = ( - f"{_SUPPORTED_TAGS}, or a version" - if allow_dist_tag - else "a concrete version" - ) - raise ValueError( - f"Invalid {source}: {version!r}. " - f"Expected {expected} matching {VERSION_PATTERN.pattern}" - ) - return candidate + raise ValueError( + f"Invalid {source}: {version!r}. " + f"Expected {_expected(allow_dist_tag)} matching {VERSION_PATTERN.pattern}" + ) diff --git a/scripts/download_cli.py b/scripts/download_cli.py index 1efe67199..8e8d05fbc 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -129,11 +129,6 @@ def _decode(stream: bytes | str | None) -> str: return stream -def _output_of(error: subprocess.CalledProcessError) -> str: - """The failed command's stdout and stderr, decoded, as one string.""" - return f"{_decode(error.stdout)}\n{_decode(error.stderr)}" - - def is_argument_rejection(error: subprocess.CalledProcessError) -> bool: """True when the installer rejected its arguments rather than failing to run. @@ -144,8 +139,12 @@ def is_argument_rejection(error: subprocess.CalledProcessError) -> bool: so retrying only burns the backoff and then reports the deterministic rejection as "Error downloading CLI after 3 attempts", which reads like a network problem. Fail fast instead. + + install.ps1 prints its usage line to stdout and install.sh to stderr, so + both streams are searched. """ - return "usage:" in _output_of(error).lower() + output = f"{_decode(error.stdout)}\n{_decode(error.stderr)}" + return "usage:" in output.lower() def _fail(headline: str, error: subprocess.CalledProcessError) -> NoReturn: diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index 82969baca..780a8acca 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -4,7 +4,8 @@ import os import subprocess import sys -from collections.abc import Iterator +from collections.abc import Callable, Iterator +from contextlib import contextmanager from pathlib import Path from typing import Any from unittest.mock import MagicMock, patch @@ -41,6 +42,10 @@ def test_default_is_latest(self, monkeypatch: pytest.MonkeyPatch) -> None: "version", [ "latest", + # `stable` is a moving tag the installer resolves, like `latest`: + # allowed here (a download resolves it at install time) and + # rejected by update_cli_version.py (a pin must name one concrete + # build). "stable", "1.2.3", "2.1.195", @@ -53,17 +58,6 @@ def test_accepted(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: monkeypatch.setenv("CLAUDE_CLI_VERSION", version) assert download_cli.get_cli_version() == version - def test_stable_is_a_downloadable_dist_tag( - self, monkeypatch: pytest.MonkeyPatch - ) -> None: - """`stable` is a moving tag the installer resolves, like `latest`. - - Allowed here (a download resolves it at install time) and rejected by - update_cli_version.py (a pin must name one concrete build). - """ - monkeypatch.setenv("CLAUDE_CLI_VERSION", "stable") - assert download_cli.get_cli_version() == "stable" - @pytest.mark.parametrize( ("version", "expected"), [ @@ -228,7 +222,12 @@ def no_sleep() -> Iterator[None]: yield -def _fake_curl(body: bytes = b"#!/bin/bash\necho install\n") -> object: +SHEBANG_BODY = b"#!/bin/bash\necho install\n" + +_RunSideEffect = Callable[..., MagicMock] + + +def _fake_curl(body: bytes = SHEBANG_BODY) -> _RunSideEffect: """A subprocess.run side effect that makes `curl -o PATH` write ``body``.""" def side_effect(command: list[str], **kwargs: object) -> MagicMock: @@ -239,17 +238,30 @@ def side_effect(command: list[str], **kwargs: object) -> MagicMock: return side_effect -def _run_unix_download( - version: str, body: bytes = b"#!/bin/bash\necho install\n" -) -> list[list[str]]: - """Run download_cli() on the Unix path, returning the argv of each subprocess.""" +@contextmanager +def _unix_install( + version: str, side_effect: _RunSideEffect | None = None +) -> Iterator[MagicMock]: + """Pin download_cli() to the Unix path, yielding the patched subprocess.run. + + ``side_effect`` defaults to a curl that writes a real shebang script; pass + one that raises to exercise a failing command. + """ with ( patch.object(download_cli.platform, "system", return_value="Linux"), patch.object( - download_cli.subprocess, "run", side_effect=_fake_curl(body) + download_cli.subprocess, + "run", + side_effect=side_effect or _fake_curl(), ) as mock_run, patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": version}), ): + yield mock_run + + +def _run_unix_download(version: str, body: bytes = SHEBANG_BODY) -> list[list[str]]: + """Run download_cli() on the Unix path, returning the argv of each subprocess.""" + with _unix_install(version, _fake_curl(body)) as mock_run: download_cli.download_cli() return [call.args[0] for call in mock_run.call_args_list] @@ -294,13 +306,7 @@ def test_curl_short_flags_present(self, flag: str) -> None: def test_stdin_is_devnull(self) -> None: """install.sh runs `claude install`, which branches on `[ -t 0 ]`. The old `curl | bash` gave it a pipe; it must never inherit a real TTY.""" - with ( - patch.object(download_cli.platform, "system", return_value="Linux"), - patch.object( - download_cli.subprocess, "run", side_effect=_fake_curl() - ) as mock_run, - patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), - ): + with _unix_install("1.2.3") as mock_run: download_cli.download_cli() assert mock_run.call_args_list @@ -341,13 +347,7 @@ def test_version_is_its_own_argv_element(self, version: str) -> None: assert bash_cmd == ["bash", bash_cmd[1], version] def test_never_uses_shell_true(self) -> None: - with ( - patch.object(download_cli.platform, "system", return_value="Linux"), - patch.object( - download_cli.subprocess, "run", side_effect=_fake_curl() - ) as mock_run, - patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), - ): + with _unix_install("1.2.3") as mock_run: download_cli.download_cli() for call in mock_run.call_args_list: @@ -369,11 +369,7 @@ def test_non_shebang_body_is_rejected_before_bash( """claude.ai serves HTTP 200 + HTML for unknown paths, which `curl -f` cannot detect. Such a body must never reach bash.""" with ( - patch.object(download_cli.platform, "system", return_value="Linux"), - patch.object( - download_cli.subprocess, "run", side_effect=_fake_curl(body) - ) as mock_run, - patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), + _unix_install("1.2.3", _fake_curl(body)) as mock_run, pytest.raises(SystemExit) as excinfo, ): download_cli.download_cli() @@ -397,9 +393,7 @@ def fake_run(command: list[str], **kwargs: object) -> MagicMock: return MagicMock() with ( - patch.object(download_cli.platform, "system", return_value="Linux"), - patch.object(download_cli.subprocess, "run", side_effect=fake_run) as run, - patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), + _unix_install("1.2.3", fake_run) as run, pytest.raises(SystemExit) as excinfo, ): download_cli.download_cli() @@ -433,9 +427,7 @@ def fake_run(command: list[str], **kwargs: object) -> MagicMock: raise error with ( - patch.object(download_cli.platform, "system", return_value="Linux"), - patch.object(download_cli.subprocess, "run", side_effect=fake_run) as run, - patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": "1.2.3"}), + _unix_install("1.2.3", fake_run) as run, pytest.raises(SystemExit) as excinfo, ): download_cli.download_cli() From 87cc5d6576740ac28b9319ce403ae210ccf5b84d Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Tue, 14 Jul 2026 06:37:09 +0000 Subject: [PATCH 09/13] Stop guessing at installer argument rejections; tighten dist-tag matching The retry loop tried to detect an installer that rejected its arguments by searching its combined output for "usage:". That check can never fire on a true positive: the version validator only ever emits latest, stable, or N.N.N(-suffix), all of which install.sh's own grammar accepts, so the usage path is unreachable by construction. install.ps1 does not print a usage line at all -- it validates with ValidatePattern and raises. What the check could do is fire on an unrelated transient failure whose output happens to contain "usage:", aborting the build with zero retries. Delete it and restore the plain three-attempt retry. Also match dist-tags exactly instead of lowercasing the candidate. install.sh is case-sensitive, so accepting "Latest" widened what reaches a real network install for no benefit; reject it and suggest the lowercase spelling. Append scripts/ to sys.path rather than prepending it: the tests import these files by path, so the entry outlives the import and a future scripts/json.py would shadow the stdlib for the whole pytest process. :house: Remote-Dev: homespace --- .github/workflows/test.yml | 8 +-- scripts/_cli_version_validation.py | 25 ++++++--- scripts/download_cli.py | 53 +++++-------------- scripts/update_cli_version.py | 7 ++- tests/test_download_cli.py | 83 +++++++++--------------------- tests/test_update_cli_version.py | 6 ++- 6 files changed, 70 insertions(+), 112 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 17b391b19..bdcca334d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -60,9 +60,11 @@ jobs: - name: Install Claude Code (Linux/macOS) if: runner.os == 'Linux' || runner.os == 'macOS' - # `shell: bash` runs with -eo pipefail. Without pipefail the step takes - # its status from `bash`, which exits 0 on an empty script, so a failed - # `curl` leaves the CLI uninstalled and the step green. + # `shell: bash` runs with -eo pipefail. Without pipefail the pipeline + # takes its status from `bash`, which exits 0 on an empty script, so a + # failed `curl` shows up later as a confusing "claude: not found" in a + # different step. With it, the step that actually broke is the one that + # fails. shell: bash run: | curl -fsSL https://claude.ai/install.sh | bash diff --git a/scripts/_cli_version_validation.py b/scripts/_cli_version_validation.py index 38ecabfcd..a62c1a7d7 100644 --- a/scripts/_cli_version_validation.py +++ b/scripts/_cli_version_validation.py @@ -56,8 +56,10 @@ # versions -- see the module docstring. VERSION_PATTERN = re.compile(r"[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z.+-]+)?") -# The moving tags the installer resolves at install time. Compared lowercased, -# so "LATEST" is the sentinel rather than a mysterious "concrete version". +# The moving tags the installer resolves at install time. Matched exactly: +# install.sh's own grammar is case-sensitive, so "Latest" is not a tag it would +# resolve, and accepting it here would widen what reaches a real network +# install. "Latest" is rejected, with the lowercase spelling suggested. DIST_TAGS = ("latest", "stable") # Anything word-shaped that is not a version: "next", "beta", "nightly". Named @@ -92,7 +94,7 @@ def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: name the one concrete build that went into the wheels. Returns: - The stripped version, with a dist-tag normalized to lowercase. + The stripped version. Raises: ValueError: If ``version`` is neither an allowed dist-tag nor a @@ -100,11 +102,11 @@ def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: """ candidate = version.strip() - # A dist-tag fails VERSION_PATTERN, so it is recognized by name -- and - # case-insensitively, so "LATEST" is not mistaken for something else. - if candidate.lower() in DIST_TAGS: + # A dist-tag fails VERSION_PATTERN, so it is recognized by name -- exactly, + # matching the installer's own case-sensitive grammar. + if candidate in DIST_TAGS: if allow_dist_tag: - return candidate.lower() + return candidate raise ValueError( f"Invalid {source}: {candidate!r} is a moving dist-tag, not a concrete " f"version. A pinned version must name the one build that goes into the " @@ -116,6 +118,15 @@ def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: # Rejected from here on; what is left is choosing the most useful reason. + # "Latest" is a tag the installer would not resolve either, so it is an + # error rather than something to normalize -- but name the spelling that + # works. Only where a tag is a legal answer at all; for a pin, the + # dist-tag-shaped branch below correctly says to use a concrete version. + if allow_dist_tag and candidate.lower() in DIST_TAGS: + raise ValueError( + f"Invalid {source}: {candidate!r}. Did you mean {candidate.lower()!r}?" + ) + # "v2.1.207" is the single most likely typo, and the installer rejects it. # Say so, rather than printing the pattern and leaving the reader to spot # the leading "v". Not normalized away: the caller asked for something we diff --git a/scripts/download_cli.py b/scripts/download_cli.py index 8e8d05fbc..98039e43d 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -15,15 +15,17 @@ import time from collections.abc import Callable from pathlib import Path -from typing import NoReturn # scripts/ is not a package. Running this file directly -- as build_wheel.py # does, via `python scripts/download_cli.py` -- already puts scripts/ on # sys.path, but loading it by path (importlib.spec_from_file_location, as the -# tests do) does not. Add it either way so the shared module resolves. +# tests do) does not. Add it either way so the shared module resolves. Appended, +# not prepended: the tests import this file by path, so the entry outlives the +# import and would otherwise let a future scripts/json.py shadow the stdlib for +# the whole pytest process. _SCRIPTS_DIR = str(Path(__file__).parent) if _SCRIPTS_DIR not in sys.path: - sys.path.insert(0, _SCRIPTS_DIR) + sys.path.append(_SCRIPTS_DIR) import _cli_version_validation as version_validation # noqa: E402 @@ -41,8 +43,8 @@ def get_cli_version() -> str: """Get the CLI version to download from environment or default. - Returns the stripped, dist-tag-normalized value -- use it, rather than the - raw environment string, for everything downstream. + Returns the stripped value -- use it, rather than the raw environment + string, for everything downstream. Raises: ValueError: If CLAUDE_CLI_VERSION is set to something other than a @@ -129,32 +131,6 @@ def _decode(stream: bytes | str | None) -> str: return stream -def is_argument_rejection(error: subprocess.CalledProcessError) -> bool: - """True when the installer rejected its arguments rather than failing to run. - - install.sh and install.ps1 validate the version they are handed against - their own grammar -- roughly `stable|latest|N.N.N(-suffix)?` -- and answer a - mismatch by printing a usage line and exiting. Like the shebang check, that - verdict is deterministic: the same arguments fail the same way every time, - so retrying only burns the backoff and then reports the deterministic - rejection as "Error downloading CLI after 3 attempts", which reads like a - network problem. Fail fast instead. - - install.ps1 prints its usage line to stdout and install.sh to stderr, so - both streams are searched. - """ - output = f"{_decode(error.stdout)}\n{_decode(error.stderr)}" - return "usage:" in output.lower() - - -def _fail(headline: str, error: subprocess.CalledProcessError) -> NoReturn: - """Report a failed install command and exit.""" - print(headline, file=sys.stderr) - print(f"stdout: {_decode(error.stdout)}", file=sys.stderr) - print(f"stderr: {_decode(error.stderr)}", file=sys.stderr) - sys.exit(1) - - def retry_install(attempt: Callable[[], None]) -> None: """Run an install attempt, retrying the whole attempt on command failure.""" # Small jitter to stagger parallel matrix builds hitting the same endpoint @@ -170,19 +146,14 @@ def retry_install(attempt: Callable[[], None]) -> None: attempt() return except subprocess.CalledProcessError as e: - if is_argument_rejection(e): - _fail( - f"Error: the installer rejected its arguments (exit " - f"{e.returncode}). This is not a network failure and will " - f"not succeed on a retry.", - e, - ) - if attempt_num == MAX_INSTALL_ATTEMPTS: - _fail( + print( f"Error downloading CLI after {MAX_INSTALL_ATTEMPTS} attempts: {e}", - e, + file=sys.stderr, ) + print(f"stdout: {_decode(e.stdout)}", file=sys.stderr) + print(f"stderr: {_decode(e.stderr)}", file=sys.stderr) + sys.exit(1) delay = 2**attempt_num print( diff --git a/scripts/update_cli_version.py b/scripts/update_cli_version.py index 40ca57acb..b9fef187b 100755 --- a/scripts/update_cli_version.py +++ b/scripts/update_cli_version.py @@ -9,10 +9,13 @@ # scripts/ is not a package. Running this file directly -- `python # scripts/update_cli_version.py 1.2.3` -- already puts scripts/ on sys.path, # but loading it by path (importlib.spec_from_file_location, as the tests do) -# does not. Add it either way so the shared module resolves. +# does not. Add it either way so the shared module resolves. Appended, not +# prepended: the tests import this file by path, so the entry outlives the +# import and would otherwise let a future scripts/json.py shadow the stdlib for +# the whole pytest process. _SCRIPTS_DIR = str(Path(__file__).parent) if _SCRIPTS_DIR not in sys.path: - sys.path.insert(0, _SCRIPTS_DIR) + sys.path.append(_SCRIPTS_DIR) import _cli_version_validation as version_validation # noqa: E402 diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index 780a8acca..3b21c7a34 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -59,7 +59,7 @@ def test_accepted(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: assert download_cli.get_cli_version() == version @pytest.mark.parametrize( - ("version", "expected"), + ("version", "suggestion"), [ ("LATEST", "latest"), ("Latest", "latest"), @@ -67,12 +67,17 @@ def test_accepted(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: ("Stable", "stable"), ], ) - def test_dist_tags_are_case_insensitive_and_normalized( - self, monkeypatch: pytest.MonkeyPatch, version: str, expected: str + def test_dist_tags_are_case_sensitive( + self, monkeypatch: pytest.MonkeyPatch, version: str, suggestion: str ) -> None: - """ "LATEST" is the sentinel, not a mysterious "concrete version".""" + """The installer's own grammar is case-sensitive, so "Latest" is not a + tag it would resolve. Reject it -- naming the spelling that works -- + rather than quietly turning it into a live install.""" monkeypatch.setenv("CLAUDE_CLI_VERSION", version) - assert download_cli.get_cli_version() == expected + with pytest.raises(ValueError, match="Invalid CLAUDE_CLI_VERSION") as excinfo: + download_cli.get_cli_version() + + assert f"Did you mean {suggestion!r}?" in str(excinfo.value) @pytest.mark.parametrize( ("version", "expected"), @@ -402,28 +407,20 @@ def fake_run(command: list[str], **kwargs: object) -> MagicMock: # curl attempted 3 times, bash never reached. assert [call.args[0][0] for call in run.call_args_list] == ["curl"] * 3 + def test_a_failing_install_retries_the_whole_attempt( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + """A failing bash re-runs curl too: the script is re-downloaded, so a + truncated or half-written body is not reused across attempts.""" + error = subprocess.CalledProcessError( + 1, ["bash"], output=b"", stderr=b"curl: (6) could not resolve\n" + ) -@pytest.mark.usefixtures("no_sleep") -class TestArgumentRejectionIsNotRetried: - """An installer that rejects its arguments is not a flaky network. - - The installer validates the version it is handed and answers a mismatch - with a usage line. Retrying that burns ~11s of backoff and then reports it - as "Error downloading CLI after 3 attempts", which reads like a download - failure. Same rationale as the shebang check: deterministic, so fail fast. - """ - - USAGE = b"Usage: install.sh [version|stable|latest]\n" - - def _run_with_failing_bash( - self, stderr: bytes = USAGE, stdout: bytes = b"" - ) -> list[list[str]]: - error = subprocess.CalledProcessError(1, ["bash"], output=stdout, stderr=stderr) + curl = _fake_curl() def fake_run(command: list[str], **kwargs: object) -> MagicMock: if command[0] == "curl": - Path(command[command.index("-o") + 1]).write_bytes(b"#!/bin/bash\n") - return MagicMock() + return curl(command, **kwargs) raise error with ( @@ -433,43 +430,13 @@ def fake_run(command: list[str], **kwargs: object) -> MagicMock: download_cli.download_cli() assert excinfo.value.code == 1 - return [call.args[0][0] for call in run.call_args_list] - - def test_usage_rejection_runs_bash_once( - self, capsys: pytest.CaptureFixture[str] - ) -> None: - assert self._run_with_failing_bash() == ["curl", "bash"] - - err = capsys.readouterr().err - assert "rejected its arguments" in err - assert "not a network failure" in err - assert "after 3 attempts" not in err - - def test_usage_on_stdout_is_also_a_rejection(self) -> None: - """install.ps1 writes its usage line to stdout, not stderr.""" - assert self._run_with_failing_bash(stderr=b"", stdout=b"Usage: ...\n") == [ + assert [call.args[0][0] for call in run.call_args_list] == [ "curl", "bash", - ] - - def test_a_real_install_failure_still_retries(self) -> None: - """Only the usage verdict short-circuits; a genuine failure keeps its - three attempts.""" - commands = self._run_with_failing_bash(stderr=b"curl: (6) could not resolve\n") - assert commands == ["curl", "bash"] * download_cli.MAX_INSTALL_ATTEMPTS - - @pytest.mark.parametrize( - ("stderr", "expected"), - [ - (b"Usage: install.sh [version]", True), - (b"usage: install.ps1 ", True), - (b"error: unknown option", False), - (b"", False), - ], - ) - def test_is_argument_rejection(self, stderr: bytes, expected: bool) -> None: - error = subprocess.CalledProcessError(1, ["bash"], output=b"", stderr=stderr) - assert download_cli.is_argument_rejection(error) is expected + ] * download_cli.MAX_INSTALL_ATTEMPTS + err = capsys.readouterr().err + assert f"after {download_cli.MAX_INSTALL_ATTEMPTS} attempts" in err + assert "could not resolve" in err def _run_windows_download(version: str) -> Any: diff --git a/tests/test_update_cli_version.py b/tests/test_update_cli_version.py index 8889b03b2..b10224f0d 100644 --- a/tests/test_update_cli_version.py +++ b/tests/test_update_cli_version.py @@ -138,6 +138,10 @@ class TestRejectedVersions: "v2.1.207", "next", "beta", + # Case variants of the dist-tags: the installer's grammar is + # case-sensitive, so these are not tags it would resolve either. + "LATEST", + "Stable", ], ) def test_rejected_and_file_untouched( @@ -147,7 +151,7 @@ def test_rejected_and_file_untouched( update_cli_version.update_cli_version(version, version_file) assert version_file.read_text() == ORIGINAL - @pytest.mark.parametrize("tag", ["latest", "stable", "LATEST", "Stable"]) + @pytest.mark.parametrize("tag", ["latest", "stable"]) def test_dist_tags_are_rejected(self, version_file: Path, tag: str) -> None: """_cli_version.py must name one concrete build, never a moving tag. From 35ed7ed47af1e61c0e8c9b3a187d1c862ea1f04c Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Tue, 14 Jul 2026 17:44:15 +0000 Subject: [PATCH 10/13] Fail fast on the install failures a retry cannot fix Six gaps around the CLI download and pin, all of the same shape: a deterministic failure treated as a transient one, or a check that stops short of the path it guards. - download_cli.py: retry_install() caught only CalledProcessError, so a missing curl/bash/powershell -- now exec'd directly rather than through a shell -- escaped as a raw FileNotFoundError traceback. A binary that is not there will not appear on the second attempt: report it in one line and exit 1. - download_cli.py: the Windows path piped the install.ps1 response body straight into iex, so claude.ai's HTTP 200 + HTML answer for an unknown path was parsed as PowerShell and retried three times. Download, check the body, then execute -- the same sequence the Unix path already used, with an HTML/empty check standing in for the shebang a .ps1 lacks. - build_wheel.py: get_bundled_cli_version() fell back to the moving "latest" dist-tag whenever _cli_version.py was missing or its regex missed, silently building unpinned wheels on all five release runners. It now runs the pin through the same validator that refuses to write a moving tag, and fails the build instead. - publish.yml: the release-gate lint job checked strictly less than lint.yml (no scripts/), and it is the one gate in front of this code. CLAUDE.md and scripts/pre-push documented the same narrow scope. - build-wheel-check.yml: _cli_version_validation.py and _cli_version.py are build-critical but were not in the paths filter, so a PR touching only them skipped the dry-run wheel build. - Dockerfile.test: `curl | bash` under /bin/sh reports only the last status, so a failed download left a green build with no CLI. Same fix as test.yml: a pipefail SHELL directive. - pyproject.toml: the sdist shipped /tests without /scripts, and three test modules load their subject out of scripts/ at import time, so the shipped suite aborted at collection. :house: Remote-Dev: homespace --- .github/workflows/build-wheel-check.yml | 4 + .github/workflows/publish.yml | 6 +- CLAUDE.md | 8 +- Dockerfile.test | 5 + pyproject.toml | 4 + scripts/build_wheel.py | 65 +++++- scripts/download_cli.py | 105 ++++++++-- scripts/pre-push | 13 +- tests/test_build_wheel.py | 98 +++++++++- tests/test_download_cli.py | 250 +++++++++++++++++++++--- 10 files changed, 486 insertions(+), 72 deletions(-) diff --git a/.github/workflows/build-wheel-check.yml b/.github/workflows/build-wheel-check.yml index 9168d6f56..e320096c4 100644 --- a/.github/workflows/build-wheel-check.yml +++ b/.github/workflows/build-wheel-check.yml @@ -7,6 +7,10 @@ on: paths: - 'scripts/build_wheel.py' - 'scripts/download_cli.py' + # Imported by download_cli.py: it decides which versions the build accepts. + - 'scripts/_cli_version_validation.py' + # The pin build_wheel.py reads to decide which CLI goes into the wheels. + - 'src/claude_agent_sdk/_cli_version.py' - '.github/workflows/build-and-publish.yml' - '.github/workflows/build-wheel-check.yml' - 'pyproject.toml' diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index cc87c1402..edd8b7266 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -50,12 +50,12 @@ jobs: - name: Run ruff run: | - ruff check src/ tests/ - ruff format --check src/ tests/ + ruff check src/ tests/ scripts/ + ruff format --check src/ tests/ scripts/ - name: Run mypy run: | - mypy src/ + mypy src/ scripts/ get-previous-tag: runs-on: ubuntu-latest diff --git a/CLAUDE.md b/CLAUDE.md index 189cdefb6..bcf912900 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -3,11 +3,11 @@ ```bash # Lint and style # Check for issues and fix automatically -python -m ruff check src/ tests/ --fix -python -m ruff format src/ tests/ +python -m ruff check src/ tests/ scripts/ --fix +python -m ruff format src/ tests/ scripts/ -# Typecheck (only done for src/) -python -m mypy src/ +# Typecheck +python -m mypy src/ scripts/ # Run all tests python -m pytest tests/ diff --git a/Dockerfile.test b/Dockerfile.test index 22adf2eca..48c6c3941 100644 --- a/Dockerfile.test +++ b/Dockerfile.test @@ -3,6 +3,11 @@ FROM python:3.12-slim +# Docker's default `RUN` shell is `/bin/sh -c`, which reports only the last +# status of a pipeline: a failed `curl` would be masked by a successful `bash`, +# leaving a green build with no CLI installed. Fail on any element instead. +SHELL ["/bin/bash", "-o", "pipefail", "-c"] + # Install dependencies for Claude CLI and git (needed for some tests) RUN apt-get update && apt-get install -y \ curl \ diff --git a/pyproject.toml b/pyproject.toml index 5ce6db6ad..6094ba54d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -66,6 +66,10 @@ only-include = ["src/claude_agent_sdk"] include = [ "/src", "/tests", + # tests/test_build_wheel.py, test_download_cli.py and test_update_cli_version.py + # load their subject out of scripts/ at import time, so an sdist that ships + # /tests without /scripts cannot even collect its own test suite. + "/scripts", "/examples", "/README.md", "/LICENSE", diff --git a/scripts/build_wheel.py b/scripts/build_wheel.py index ec6799af1..a47ad2e6a 100755 --- a/scripts/build_wheel.py +++ b/scripts/build_wheel.py @@ -22,6 +22,17 @@ import subprocess import sys from pathlib import Path +from typing import NoReturn + +# scripts/ is not a package. Running this file directly already puts scripts/ on +# sys.path, but loading it by path (importlib.spec_from_file_location, as the +# tests do) does not. Add it either way so the shared module resolves. Appended, +# not prepended, for the reason spelled out in download_cli.py. +_SCRIPTS_DIR = str(Path(__file__).parent) +if _SCRIPTS_DIR not in sys.path: + sys.path.append(_SCRIPTS_DIR) + +import _cli_version_validation as version_validation # noqa: E402 try: import twine # noqa: F401 @@ -69,17 +80,51 @@ def update_version(version: str) -> None: ) +CLI_VERSION_FILE = Path("src/claude_agent_sdk/_cli_version.py") + +# The assignment update_cli_version.py writes. +CLI_VERSION_PATTERN = re.compile(r'__cli_version__ = "([^"]+)"') + + +def _fail_unpinned(reason: str) -> NoReturn: + """Report an unusable CLI pin and stop the build.""" + print( + f"Error: cannot determine the CLI version to bundle: {reason}", file=sys.stderr + ) + print( + f"Expected a concrete version in {CLI_VERSION_FILE} " + f'(written as `__cli_version__ = "2.1.207"`). ' + f"Fix the pin, or pass --cli-version explicitly.", + file=sys.stderr, + ) + sys.exit(1) + + def get_bundled_cli_version() -> str: - """Get the CLI version that should be bundled from _cli_version.py.""" - version_file = Path("src/claude_agent_sdk/_cli_version.py") - if not version_file.exists(): - return "latest" - - content = version_file.read_text() - match = re.search(r'__cli_version__ = "([^"]+)"', content) - if match: - return match.group(1) - return "latest" + """Get the CLI version that should be bundled from _cli_version.py. + + Fails the build rather than falling back to a dist-tag. _cli_version.py is + the only record of which CLI build goes into the wheels; defaulting to the + moving "latest" when the pin is missing, unparseable, or itself a dist-tag + would silently publish an unpinned -- and, across the release matrix, + potentially inconsistent -- set of wheels. update_cli_version.py refuses to + *write* a moving tag; this is the same rule on the read side. + """ + if not CLI_VERSION_FILE.exists(): + _fail_unpinned(f"{CLI_VERSION_FILE} does not exist") + + match = CLI_VERSION_PATTERN.search(CLI_VERSION_FILE.read_text()) + if not match: + _fail_unpinned( + f"no `{CLI_VERSION_PATTERN.pattern}` assignment found in {CLI_VERSION_FILE}" + ) + + try: + return version_validation.validate_version( + match.group(1), source=str(CLI_VERSION_FILE), allow_dist_tag=False + ) + except ValueError as exc: + _fail_unpinned(str(exc)) def download_cli(cli_version: str | None = None) -> None: diff --git a/scripts/download_cli.py b/scripts/download_cli.py index 98039e43d..a52d8ab8e 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -15,6 +15,7 @@ import time from collections.abc import Callable from pathlib import Path +from typing import NoReturn # scripts/ is not a package. Running this file directly -- as build_wheel.py # does, via `python scripts/download_cli.py` -- already puts scripts/ on @@ -36,6 +37,11 @@ # name instead of having it spliced into the PowerShell command text. INSTALL_VERSION_ENV_VAR = "CLAUDE_CLI_INSTALL_VERSION" +# Where the downloaded PowerShell installer is written. Handed to PowerShell in +# the environment for the same reason the version is: a temp path is not part +# of the command text PowerShell parses. +INSTALL_SCRIPT_ENV_VAR = "CLAUDE_CLI_INSTALL_SCRIPT" + # How many times retry_install() runs an attempt before giving up. MAX_INSTALL_ATTEMPTS = 3 @@ -104,6 +110,17 @@ def run_command(command: list[str], env: dict[str, str] | None = None) -> None: ) +def _reject_install_script(script_path: str, reason: str) -> NoReturn: + """Report a downloaded installer we refuse to execute, and exit.""" + head = Path(script_path).read_bytes()[:64] + print( + f"Error: downloaded install script {reason} (first bytes: {head!r}). " + f"Refusing to execute it.", + file=sys.stderr, + ) + sys.exit(1) + + def check_install_script(script_path: str) -> None: """Reject a downloaded install script that is not a shell script. @@ -114,12 +131,27 @@ def check_install_script(script_path: str) -> None: with Path(script_path).open("rb") as f: magic = f.read(2) if magic != b"#!": - print( - f"Error: downloaded install script does not start with a shebang " - f"(first bytes: {magic!r}). Refusing to execute it.", - file=sys.stderr, - ) - sys.exit(1) + _reject_install_script(script_path, "does not start with a shebang") + + +def check_powershell_install_script(script_path: str) -> None: + """Reject a downloaded install.ps1 that is not a PowerShell script. + + The Windows counterpart of check_install_script(): the same HTTP 200 + + HTML body that `curl -f` cannot detect is also invisible to + Invoke-RestMethod, and would otherwise be parsed and executed by + PowerShell. A .ps1 has no shebang to check, so instead reject what the + error page actually is -- an XML/HTML document, whose first non-blank + character is '<' -- and an empty body, which is never a valid installer. + A wrong body is deterministic, so this fails fast instead of retrying. + """ + body = Path(script_path).read_bytes() + # A UTF-8 BOM is legal in a .ps1 and common in PowerShell-authored files. + stripped = body.removeprefix(b"\xef\xbb\xbf").lstrip() + if not stripped: + _reject_install_script(script_path, "is empty") + if stripped.startswith(b"<"): + _reject_install_script(script_path, "looks like an HTML or XML document") def _decode(stream: bytes | str | None) -> str: @@ -145,6 +177,18 @@ def retry_install(attempt: Callable[[], None]) -> None: try: attempt() return + except (FileNotFoundError, OSError) as e: + # The command could not be started at all: no `curl`, no `bash`, no + # `powershell` on PATH. Deterministic -- a second attempt cannot + # make the binary appear -- so fail immediately rather than + # sleeping through three of them and then blaming the download. + # The old `bash -c` form surfaced this as a plain exit 127; keep it + # just as legible now that the binaries are exec'd directly. + print( + f"Error: could not run the install command: {e}", + file=sys.stderr, + ) + sys.exit(1) except subprocess.CalledProcessError as e: if attempt_num == MAX_INSTALL_ATTEMPTS: print( @@ -173,36 +217,55 @@ def download_cli() -> None: # Build install command based on platform if system == "Windows": - # Use PowerShell installer on Windows. The version is handed to - # PowerShell in the environment and referenced by name, so it is never - # part of the command text that PowerShell parses. `$env:NAME` in - # argument position expands to exactly one argument -- PowerShell does - # not re-split or re-parse it -- which is the argv separation the Unix - # path gets from `["bash", script, version]`. + # Use the PowerShell installer on Windows, downloaded to a file and + # checked before it is executed -- the same download / verify / execute + # sequence as the Unix path below, rather than piping the response body + # straight into `iex`. + # + # Both the script path and the version are handed to PowerShell in the + # environment and referenced by name, so neither is part of the command + # text that PowerShell parses. `$env:NAME` in argument position expands + # to exactly one argument -- PowerShell does not re-split or re-parse + # it -- which is the argv separation the Unix path gets from + # `["bash", script, version]`. + # # "latest" is the installer's own default, so it is expressed by # passing no argument at all. Every other accepted value -- a concrete # version, or the "stable" dist-tag -- is passed through the same # argument path. - install_env: dict[str, str] | None = None - if version == "latest": - install_cmd = [ + with tempfile.TemporaryDirectory() as tmpdir: + ps_script_path = str(Path(tmpdir) / "install.ps1") + + download_env = {**os.environ, INSTALL_SCRIPT_ENV_VAR: ps_script_path} + download_cmd = [ "powershell", "-ExecutionPolicy", "Bypass", "-Command", - "irm https://claude.ai/install.ps1 | iex", + "$ProgressPreference = 'SilentlyContinue'; " + "Invoke-RestMethod -Uri https://claude.ai/install.ps1 " + f"-OutFile $env:{INSTALL_SCRIPT_ENV_VAR}", ] - else: + + install_env = dict(download_env) + ps_install = f"& $env:{INSTALL_SCRIPT_ENV_VAR}" + if version != "latest": + ps_install += f" $env:{INSTALL_VERSION_ENV_VAR}" + install_env[INSTALL_VERSION_ENV_VAR] = version install_cmd = [ "powershell", "-ExecutionPolicy", "Bypass", "-Command", - "& ([scriptblock]::Create((irm https://claude.ai/install.ps1))) " - f"$env:{INSTALL_VERSION_ENV_VAR}", + ps_install, ] - install_env = {**os.environ, INSTALL_VERSION_ENV_VAR: version} - retry_install(lambda: run_command(install_cmd, env=install_env)) + + def windows_attempt() -> None: + run_command(download_cmd, env=download_env) + check_powershell_install_script(ps_script_path) + run_command(install_cmd, env=install_env) + + retry_install(windows_attempt) return # Download install.sh to a file and run it directly rather than piping diff --git a/scripts/pre-push b/scripts/pre-push index f009b614d..60c4567e3 100755 --- a/scripts/pre-push +++ b/scripts/pre-push @@ -1,27 +1,30 @@ #!/bin/bash -# Pre-push hook to run lint checks (matches .github/workflows/lint.yml) +# Pre-push hook to run lint checks. +# +# Runs the ruff half of .github/workflows/lint.yml, over the same paths it +# checks (src/ tests/ scripts/). CI additionally runs `mypy src/ scripts/`. echo "Running lint checks before push..." echo "" # Run ruff check echo "→ Running ruff check..." -python -m ruff check src/ tests/ +python -m ruff check src/ tests/ scripts/ if [ $? -ne 0 ]; then echo "" echo "❌ ruff check failed. Fix lint issues before pushing." - echo " Run: python -m ruff check src/ tests/ --fix" + echo " Run: python -m ruff check src/ tests/ scripts/ --fix" exit 1 fi # Run ruff format check echo "→ Running ruff format check..." -python -m ruff format --check src/ tests/ +python -m ruff format --check src/ tests/ scripts/ if [ $? -ne 0 ]; then echo "" echo "❌ ruff format check failed. Fix formatting before pushing." - echo " Run: python -m ruff format src/ tests/" + echo " Run: python -m ruff format src/ tests/ scripts/" exit 1 fi diff --git a/tests/test_build_wheel.py b/tests/test_build_wheel.py index 95cb7aae6..8774816b2 100644 --- a/tests/test_build_wheel.py +++ b/tests/test_build_wheel.py @@ -1,4 +1,4 @@ -"""Tests for scripts/build_wheel.py platform tagging.""" +"""Tests for scripts/build_wheel.py platform tagging and CLI-pin reading.""" import importlib.util import sys @@ -7,10 +7,12 @@ import pytest +REPO_ROOT = Path(__file__).parent.parent + # scripts/ is not a package, so load build_wheel.py by path _spec = importlib.util.spec_from_file_location( "build_wheel", - Path(__file__).parent.parent / "scripts" / "build_wheel.py", + REPO_ROOT / "scripts" / "build_wheel.py", ) assert _spec is not None and _spec.loader is not None build_wheel = importlib.util.module_from_spec(_spec) @@ -58,3 +60,95 @@ def test_unknown_system_falls_through(self) -> None: patch("platform.machine", return_value="amd64"), ): assert build_wheel.get_platform_tag() == "freebsd_amd64" + + +@pytest.fixture +def pinned(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A stand-in repo whose _cli_version.py the caller writes.""" + monkeypatch.chdir(tmp_path) + version_file = tmp_path / build_wheel.CLI_VERSION_FILE + version_file.parent.mkdir(parents=True) + return version_file + + +class TestGetBundledCliVersion: + """The CLI pin is the only record of which build goes into the wheels. + + An unreadable pin must fail the build. Falling back to the moving "latest" + dist-tag -- which download_cli.py accepts -- would publish an unpinned set + of wheels, each of the five release runners resolving "latest" + independently. update_cli_version.py refuses to write a moving tag; this is + the same rule on the read side. + """ + + def test_reads_the_pinned_version(self, pinned: Path) -> None: + pinned.write_text('__cli_version__ = "2.1.207"\n') + assert build_wheel.get_bundled_cli_version() == "2.1.207" + + def test_reads_a_dev_version(self, pinned: Path) -> None: + dev = "2.1.146-dev.20260519.t105443.shaece3dab" + pinned.write_text(f'__cli_version__ = "{dev}"\n') + assert build_wheel.get_bundled_cli_version() == dev + + def test_missing_file_fails( + self, pinned: Path, capsys: pytest.CaptureFixture[str] + ) -> None: + with pytest.raises(SystemExit) as excinfo: + build_wheel.get_bundled_cli_version() + + assert excinfo.value.code == 1 + assert "does not exist" in capsys.readouterr().err + + def test_unparseable_pin_fails( + self, pinned: Path, capsys: pytest.CaptureFixture[str] + ) -> None: + """A single-quoted reformat is the realistic way the regex stops + matching -- and used to silently downgrade the build to "latest".""" + pinned.write_text("__cli_version__ = '2.1.207'\n") + + with pytest.raises(SystemExit) as excinfo: + build_wheel.get_bundled_cli_version() + + assert excinfo.value.code == 1 + assert "no `" in capsys.readouterr().err + + @pytest.mark.parametrize("tag", ["latest", "stable"]) + def test_a_moving_dist_tag_pin_fails( + self, pinned: Path, tag: str, capsys: pytest.CaptureFixture[str] + ) -> None: + pinned.write_text(f'__cli_version__ = "{tag}"\n') + + with pytest.raises(SystemExit) as excinfo: + build_wheel.get_bundled_cli_version() + + assert excinfo.value.code == 1 + assert "moving dist-tag" in capsys.readouterr().err + + @pytest.mark.parametrize("value", ["", "not-a-version", "v2.1.207", "2.1"]) + def test_a_bad_pin_fails(self, pinned: Path, value: str) -> None: + pinned.write_text(f'__cli_version__ = "{value}"\n') + + with pytest.raises(SystemExit) as excinfo: + build_wheel.get_bundled_cli_version() + + assert excinfo.value.code == 1 + + def test_the_repo_pin_is_readable(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The pin actually checked in must satisfy the rule the build enforces.""" + monkeypatch.chdir(REPO_ROOT) + assert build_wheel.get_bundled_cli_version() not in ("latest", "stable") + + +class TestSdistShipsTheScriptsItsTestsImport: + """The sdist ships /tests, and these test modules exec their subject out of + scripts/ at import time, so /scripts has to ship with them -- otherwise the + shipped suite aborts at collection with FileNotFoundError.""" + + def test_scripts_is_in_the_sdist(self) -> None: + tomllib = pytest.importorskip("tomllib") # stdlib from 3.11 + + config = tomllib.loads((REPO_ROOT / "pyproject.toml").read_text()) + include = config["tool"]["hatch"]["build"]["targets"]["sdist"]["include"] + + assert "/tests" in include + assert "/scripts" in include diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index 3b21c7a34..6f6fc2511 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -439,22 +439,60 @@ def fake_run(command: list[str], **kwargs: object) -> MagicMock: assert "could not resolve" in err -def _run_windows_download(version: str) -> Any: - """Run download_cli() on the Windows path, returning the single run() call.""" +SCRIPT_ENV_VAR = download_cli.INSTALL_SCRIPT_ENV_VAR + +PS_BODY = b"# install.ps1\nWrite-Host installing\n" + + +def _fake_irm(body: bytes = PS_BODY) -> _RunSideEffect: + """A subprocess.run side effect making the download step write ``body``. + + The Windows download step names its output file only in the environment, so + that is where the fake finds the path to write -- the same indirection the + real PowerShell command resolves. + """ + + def side_effect(command: list[str], **kwargs: Any) -> MagicMock: + env = kwargs.get("env") or {} + path = env.get(SCRIPT_ENV_VAR) + if path and "Invoke-RestMethod" in command[-1]: + Path(path).write_bytes(body) + return MagicMock() + + return side_effect + + +@contextmanager +def _windows_install( + version: str, side_effect: _RunSideEffect | None = None +) -> Iterator[MagicMock]: + """Pin download_cli() to the Windows path, yielding the patched run.""" with ( patch.object(download_cli.platform, "system", return_value="Windows"), - patch.object(download_cli.subprocess, "run") as mock_run, + patch.object( + download_cli.subprocess, + "run", + side_effect=side_effect or _fake_irm(), + ) as mock_run, patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": version}), ): + yield mock_run + + +def _run_windows_download(version: str, body: bytes = PS_BODY) -> tuple[Any, Any]: + """Run download_cli() on Windows, returning its (download, install) calls.""" + with _windows_install(version, _fake_irm(body)) as mock_run: download_cli.download_cli() - (call,) = mock_run.call_args_list - return call + download_call, install_call = mock_run.call_args_list + return download_call, install_call @pytest.mark.usefixtures("no_sleep") class TestWindowsInstall: - """The PowerShell branch routes through the same validation.""" + """The PowerShell branch downloads install.ps1, checks the body, then runs + it -- the same download / verify / execute shape as the Unix path -- and + routes the version through the same validation.""" def test_rejects_injected_version_before_running_anything(self) -> None: with ( @@ -470,12 +508,38 @@ def test_rejects_injected_version_before_running_anything(self) -> None: mock_run.assert_not_called() + def test_downloads_then_executes_script(self) -> None: + download_call, install_call = _run_windows_download("1.2.3") + + download_cmd = download_call.args[0] + assert download_cmd[0] == "powershell" + assert ( + "Invoke-RestMethod -Uri https://claude.ai/install.ps1" in (download_cmd[-1]) + ) + + # The download writes install.ps1 into a temp dir; the install step runs + # that same file, both reaching it through the environment. + script_path = download_call.kwargs["env"][SCRIPT_ENV_VAR] + assert Path(script_path).name == "install.ps1" + assert install_call.kwargs["env"][SCRIPT_ENV_VAR] == script_path + assert install_call.args[0][-1].startswith(f"& $env:{SCRIPT_ENV_VAR}") + + def test_script_path_is_never_in_the_powershell_command_text(self) -> None: + """A temp path can contain spaces (Windows: "C:\\Users\\Foo Bar\\..."), + so it goes through the environment too, never into the command text.""" + download_call, install_call = _run_windows_download("1.2.3") + + script_path = download_call.kwargs["env"][SCRIPT_ENV_VAR] + for call in (download_call, install_call): + for arg in call.args[0]: + assert script_path not in arg, f"script path interpolated into {arg!r}" + def test_valid_version_reaches_powershell_command(self) -> None: - call = _run_windows_download(DEV_VERSION) - cmd = call.args[0] + _, install_call = _run_windows_download(DEV_VERSION) + cmd = install_call.args[0] assert cmd[0] == "powershell" assert cmd[-1].endswith(f"$env:{ENV_VAR}") - assert call.kwargs["stdin"] is subprocess.DEVNULL + assert install_call.kwargs["stdin"] is subprocess.DEVNULL @pytest.mark.parametrize("version", ["1.2.3", DEV_VERSION]) def test_version_is_never_in_the_powershell_command_text( @@ -483,16 +547,15 @@ def test_version_is_never_in_the_powershell_command_text( ) -> None: """Regression guard: the version must reach PowerShell through the environment, never spliced into the `-Command` string it parses.""" - call = _run_windows_download(version) - - for arg in call.args[0]: - assert version not in arg, f"version interpolated into {arg!r}" + for call in _run_windows_download(version): + for arg in call.args[0]: + assert version not in arg, f"version interpolated into {arg!r}" @pytest.mark.parametrize("version", ["1.2.3", DEV_VERSION]) def test_version_is_carried_in_the_environment(self, version: str) -> None: - call = _run_windows_download(version) + _, install_call = _run_windows_download(version) - env = call.kwargs["env"] + env = install_call.kwargs["env"] assert env is not None, "PowerShell must be given an explicit environment" assert env[ENV_VAR] == version # The child still needs PATH, SystemRoot, etc. to run at all. @@ -501,24 +564,157 @@ def test_version_is_carried_in_the_environment(self, version: str) -> None: def test_command_references_the_env_var_it_sets(self) -> None: """The name in the command text and the name in the environment are the same constant, so a rename cannot desynchronize them silently.""" - call = _run_windows_download("1.2.3") - assert f"$env:{ENV_VAR}" in call.args[0][-1] - assert ENV_VAR in call.kwargs["env"] + _, install_call = _run_windows_download("1.2.3") + assert f"$env:{ENV_VAR}" in install_call.args[0][-1] + assert ENV_VAR in install_call.kwargs["env"] - def test_latest_uses_plain_installer(self) -> None: - call = _run_windows_download("latest") - assert call.args[0][-1] == "irm https://claude.ai/install.ps1 | iex" + def test_stdin_is_devnull(self) -> None: + for call in _run_windows_download("1.2.3"): + assert call.kwargs["stdin"] is subprocess.DEVNULL def test_latest_passes_no_version_argument(self) -> None: """`latest` invokes the installer with no argument at all -- not with an empty string -- and sets no version in the child's environment.""" - call = _run_windows_download("latest") + _, install_call = _run_windows_download("latest") + + command = install_call.args[0][-1] + assert command == f"& $env:{SCRIPT_ENV_VAR}" + assert ENV_VAR not in command + assert ENV_VAR not in install_call.kwargs["env"] + + def test_stable_is_passed_as_an_argument(self) -> None: + _, install_call = _run_windows_download("stable") + assert install_call.args[0][-1] == (f"& $env:{SCRIPT_ENV_VAR} $env:{ENV_VAR}") + assert install_call.kwargs["env"][ENV_VAR] == "stable" + + def test_never_pipes_the_response_body_into_iex(self) -> None: + """Regression guard for the body-integrity gap: an unchecked body must + never be executed straight off the wire.""" + for version in ("latest", "1.2.3"): + for call in _run_windows_download(version): + command = call.args[0][-1] + assert "iex" not in command + assert "scriptblock" not in command + + @pytest.mark.parametrize( + "body", + [ + b"\nNot found", + b"\n oops", + b'', + b"", + b" \n\t\n", + ], + ) + def test_non_powershell_body_is_rejected_before_execution( + self, body: bytes, capsys: pytest.CaptureFixture[str] + ) -> None: + """claude.ai answers unknown paths with HTTP 200 + HTML, which + Invoke-RestMethod cannot detect either. Such a body must never be run, + and -- being deterministic -- must not be retried.""" + with ( + _windows_install("1.2.3", _fake_irm(body)) as mock_run, + pytest.raises(SystemExit) as excinfo, + ): + download_cli.download_cli() + + assert excinfo.value.code == 1 + assert "Refusing to execute it" in capsys.readouterr().err + # The download ran once; the installer never, and nothing was retried. + assert len(mock_run.call_args_list) == 1 + + def test_powershell_body_is_executed(self) -> None: + """A body with a UTF-8 BOM -- legal, and common, in a real .ps1 -- is + not mistaken for a bad one.""" + download_call, install_call = _run_windows_download( + "1.2.3", body=b"\xef\xbb\xbf# install.ps1\nWrite-Host hi\n" + ) + assert "Invoke-RestMethod" in download_call.args[0][-1] + assert install_call.args[0][-1].startswith(f"& $env:{SCRIPT_ENV_VAR}") - command = call.args[0][-1] - assert "scriptblock" not in command - assert "$env:" not in command - # env=None inherits ours, so no version is injected there either. - assert call.kwargs["env"] is None + +@pytest.mark.usefixtures("no_sleep") +class TestMissingBinaryFailsFast: + """A command that cannot be started at all is deterministic. + + The Unix path exec's `curl` and `bash` directly, so a missing binary raises + FileNotFoundError rather than exiting 127 through a shell. That must be a + one-line error and an exit 1 -- not a traceback, and not three attempts and + a misleading "Error downloading CLI after 3 attempts". + """ + + def _missing(self, name: str) -> _RunSideEffect: + def side_effect(command: list[str], **kwargs: object) -> MagicMock: + raise FileNotFoundError(2, "No such file or directory", name) + + return side_effect + + def test_missing_curl_is_not_retried( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + with ( + _unix_install("1.2.3", self._missing("curl")) as mock_run, + pytest.raises(SystemExit) as excinfo, + ): + download_cli.download_cli() + + assert excinfo.value.code == 1 + assert len(mock_run.call_args_list) == 1 + err = capsys.readouterr().err + assert "could not run the install command" in err + assert "curl" in err + assert "after 3 attempts" not in err + + def test_missing_bash_is_not_retried( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + curl = _fake_curl() + + def side_effect(command: list[str], **kwargs: object) -> MagicMock: + if command[0] == "curl": + return curl(command, **kwargs) + raise FileNotFoundError(2, "No such file or directory", "bash") + + with ( + _unix_install("1.2.3", side_effect) as mock_run, + pytest.raises(SystemExit) as excinfo, + ): + download_cli.download_cli() + + assert excinfo.value.code == 1 + assert [call.args[0][0] for call in mock_run.call_args_list] == ["curl", "bash"] + assert "could not run the install command" in capsys.readouterr().err + + def test_missing_powershell_is_not_retried( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + with ( + _windows_install("1.2.3", self._missing("powershell")) as mock_run, + pytest.raises(SystemExit) as excinfo, + ): + download_cli.download_cli() + + assert excinfo.value.code == 1 + assert len(mock_run.call_args_list) == 1 + assert "could not run the install command" in capsys.readouterr().err + + def test_other_os_errors_are_not_retried_either( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + """PermissionError (a noexec tmpdir, say) is just as deterministic.""" + + def side_effect(command: list[str], **kwargs: object) -> MagicMock: + raise PermissionError(13, "Permission denied", "curl") + + with ( + _unix_install("1.2.3", side_effect) as mock_run, + pytest.raises(SystemExit) as excinfo, + ): + download_cli.download_cli() + + assert excinfo.value.code == 1 + assert len(mock_run.call_args_list) == 1 + assert "could not run the install command" in capsys.readouterr().err class TestCommandLine: From 54fd51c2231e34a0054a05b7891ef7cb58ffc569 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Wed, 15 Jul 2026 00:32:02 +0000 Subject: [PATCH 11/13] Accept comment-based-help installers; simplify the validator and install paths Review follow-ups: - check_powershell_install_script() rejected any body whose first non-blank byte is '<', but a .ps1 may legitimately open with '<#' -- a block comment, which is where about_Comment_Based_Help puts a script's help block, ahead of param(). Real installers do exactly this, so a valid install.ps1 would have been refused as an "HTML or XML document" and deterministically failed every Windows wheel build with no retry. Exempt that one opener; no HTML or XML document begins with it. Pinned by two accepted-body cases. - .claude/settings.json still carried the pre-widening lint scope: its PostToolUse hook ran ruff over src/ tests/ only, so edits to the scripts/ files were never auto-linted even though lint.yml now enforces `ruff format --check scripts/`, and its permission allowlist pre-approved only the old narrow commands. Widened both, matching CLAUDE.md, lint.yml, publish.yml and scripts/pre-push. Simplification (no behavior change): - validate_version() repeated `f"Invalid {source}: "` across five raise sites while choosing a reason. Split the reason-picking into _rejection(), leaving the validator as two accept checks and one raise. - download_cli() was a 90-line function with two long inline platform branches and a mid-function return. Split into _install_on_unix() / _install_on_windows() behind a dispatcher, with a _powershell() helper for the repeated invocation prefix. Named the "latest" literal DEFAULT_VERSION -- it is both the default for an unset CLAUDE_CLI_VERSION and the value both paths express by passing no argument. - retry_install() caught `(FileNotFoundError, OSError)`; FileNotFoundError is an OSError. CalledProcessError is not, so the retry branch is unaffected. - Deduplicated the sys.path bootstrap comment across the three scripts. Adds .claude/skills/verify/SKILL.md: the scripts are CLIs whose install path overwrites the developer's claude binary if run for real, so the recipe for driving them against stub curl/bash/powershell is worth keeping. :house: Remote-Dev: homespace --- .claude/settings.json | 8 +- .claude/skills/verify/SKILL.md | 79 ++++++++++++++++ scripts/_cli_version_validation.py | 87 ++++++++++-------- scripts/download_cli.py | 142 +++++++++++++++-------------- scripts/update_cli_version.py | 11 +-- tests/test_download_cli.py | 23 +++-- 6 files changed, 228 insertions(+), 122 deletions(-) create mode 100644 .claude/skills/verify/SKILL.md diff --git a/.claude/settings.json b/.claude/settings.json index 33ef1223f..845a12b56 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -1,9 +1,9 @@ { "permissions": { "allow": [ - "Bash(python -m ruff check src/ tests/ --fix)", - "Bash(python -m ruff format src/ tests/)", - "Bash(python -m mypy src/)", + "Bash(python -m ruff check src/ tests/ scripts/ --fix)", + "Bash(python -m ruff format src/ tests/ scripts/)", + "Bash(python -m mypy src/ scripts/)", "Bash(python -m pytest tests/)", "Bash(python -m pytest tests/*)" ], @@ -15,7 +15,7 @@ "hooks": [ { "type": "command", - "command": "python -m ruff check src/ tests/ --fix && python -m ruff format src/ tests/" + "command": "python -m ruff check src/ tests/ scripts/ --fix && python -m ruff format src/ tests/ scripts/" } ], "matcher": "Edit|Write|MultiEdit" diff --git a/.claude/skills/verify/SKILL.md b/.claude/skills/verify/SKILL.md new file mode 100644 index 000000000..49b608dde --- /dev/null +++ b/.claude/skills/verify/SKILL.md @@ -0,0 +1,79 @@ +--- +name: verify +description: Drive this repo's build/release scripts end-to-end without touching the network. Use when verifying changes to scripts/download_cli.py, scripts/build_wheel.py, scripts/update_cli_version.py, or scripts/_cli_version_validation.py. +--- + +# Verifying the build scripts + +The SDK itself is a library (drive it through `import claude_agent_sdk`), but +the `scripts/` directory is a set of **CLIs** run by the release workflows. +Verify them by running them, not by importing them. + +## NEVER run the real installer + +`scripts/download_cli.py` shells out to `curl https://claude.ai/install.sh | bash` +(and the PowerShell equivalent). Running it for real **overwrites the `claude` +binary on the developer's machine**. Do not run `bash install.sh`, and do not +invoke `download_cli.py` with `latest`/`stable` (or any case variant) against +the real network. + +Instead, put stub `curl` / `bash` / `powershell` on a temp `PATH` and drive the +script under `env -i` with an isolated `HOME` (so `find_installed_cli()` cannot +discover the real binary and copy it into `src/claude_agent_sdk/_bundled/`). + +## The harness + +```bash +SB=$(mktemp -d); mkdir -p $SB/stub $SB/home $SB/repo/src/claude_agent_sdk +cp scripts/{download_cli.py,build_wheel.py,update_cli_version.py,_cli_version_validation.py} $SB/repo/scripts/ +printf '__cli_version__ = "2.1.208"\n' > $SB/repo/src/claude_agent_sdk/_cli_version.py + +cat > $SB/stub/curl <<'EOF' +#!/bin/bash +out=""; prev=""; for a in "$@"; do [ "$prev" = "-o" ] && out="$a"; prev="$a"; done +echo "[stub curl] argv: $*" >> "$MARKER" +[ -n "$out" ] && printf '%b' "${CURL_BODY:-#!/bin/bash\necho hi\n}" > "$out" +exit ${CURL_EXIT:-0} +EOF +cat > $SB/stub/bash <<'EOF' +#!/bin/bash +{ echo "[stub bash] argc=$#"; for a in "$@"; do echo " <$a>"; done; } >> "$MARKER" +EOF +chmod +x $SB/stub/* + +cd $SB/repo +env -i HOME=$SB/home PATH=$SB/stub:/usr/bin:/bin MARKER=$SB/m \ + CLAUDE_CLI_VERSION=2.1.208 .venv/bin/python scripts/download_cli.py +``` + +`run_command()` captures the child's output, so the stubs must log to a +`$MARKER` file — printing to stderr is swallowed. + +## Flows worth driving + +- **`update_cli_version.py `** — the whole validator surface is + reachable here: a concrete version writes the file; `latest`/`stable`, + `v2.1.207`, `next`, `2.1`, and a quote-breakout string each exit 1 with a + distinct message and leave the file untouched. +- **`build_wheel.py --skip-sdist`** — reads the pin, then chains into + `download_cli.py`. Rewrite `_cli_version.py` in the sandbox to a moving tag / + single quotes / garbage / delete it: each must fail *before* any subprocess + is spawned. `--cli-version ` bypasses the pin (intentional escape hatch; + the release workflow does not use it). +- **`download_cli.py`** — set `CURL_BODY` to an HTML error page or an empty + string to prove the body check refuses it with no retry; empty the `PATH` to + prove a missing `curl` fails fast in one attempt; `CURL_EXIT=22` to prove a + genuine transient failure still retries 3×. +- **The Windows path is unreachable on Linux.** It is behind + `platform.system() == "Windows"`. Drive it with a small runner that + `patch.object(m.platform, "system", return_value="Windows")` and a stub + `powershell` that reads the `-Command` text and honours + `$env:CLAUDE_CLI_INSTALL_SCRIPT`. This is the one place where forcing the + platform is legitimate. + +## Gotchas + +- `${CURL_BODY:-default}` treats an *empty* body as unset. Use a separate stub + that does `: > "$out"` to test the empty-body branch. +- The retry path really sleeps (jitter up to 5s, then 2s + 4s). A full + three-attempt run takes ~10s; budget for it rather than assuming a hang. diff --git a/scripts/_cli_version_validation.py b/scripts/_cli_version_validation.py index a62c1a7d7..8571f47e0 100644 --- a/scripts/_cli_version_validation.py +++ b/scripts/_cli_version_validation.py @@ -76,6 +76,51 @@ def _expected(allow_dist_tag: bool) -> str: return "a concrete version" +def _rejection(version: str, allow_dist_tag: bool) -> str: + """Why ``version`` is unusable -- the most specific reason that applies. + + The caller prefixes "Invalid : ", so each reason here reads as the + rest of that sentence. + """ + candidate = version.strip() + + # Reachable only when a tag is not a legal answer: validate_version() + # returns the tag otherwise. + if candidate in DIST_TAGS: + return ( + f"{candidate!r} is a moving dist-tag, not a concrete version. A pinned " + f"version must name the one build that goes into the wheels. Expected " + f"a version matching {VERSION_PATTERN.pattern}" + ) + + # "Latest" is a tag the installer would not resolve either, so it is an + # error rather than something to normalize -- but name the spelling that + # works. Only where a tag is a legal answer at all; for a pin, the + # dist-tag-shaped branch below correctly says to use a concrete version. + if allow_dist_tag and candidate.lower() in DIST_TAGS: + return f"{candidate!r}. Did you mean {candidate.lower()!r}?" + + # "v2.1.207" is the single most likely typo, and the installer rejects it. + # Say so, rather than printing the pattern and leaving the reader to spot + # the leading "v". Not normalized away: the caller asked for something we + # do not support, and silently installing a different string is worse. + if candidate[:1] in ("v", "V") and VERSION_PATTERN.fullmatch(candidate[1:]): + return f"{candidate!r}. Did you mean {candidate[1:]!r}? (no leading 'v')" + + if _DIST_TAG_SHAPED.fullmatch(candidate): + return ( + f"{candidate!r} is not a supported dist-tag; " + f"use {_expected(allow_dist_tag)}" + ) + + # Nothing recognizable. Name the raw value, not the stripped one: if the + # whitespace is the problem, the reader has to be able to see it. + return ( + f"{version!r}. Expected {_expected(allow_dist_tag)} " + f"matching {VERSION_PATTERN.pattern}" + ) + + def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: """Return the usable form of ``version``, or raise. @@ -104,46 +149,10 @@ def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: # A dist-tag fails VERSION_PATTERN, so it is recognized by name -- exactly, # matching the installer's own case-sensitive grammar. - if candidate in DIST_TAGS: - if allow_dist_tag: - return candidate - raise ValueError( - f"Invalid {source}: {candidate!r} is a moving dist-tag, not a concrete " - f"version. A pinned version must name the one build that goes into the " - f"wheels. Expected a version matching {VERSION_PATTERN.pattern}" - ) + if candidate in DIST_TAGS and allow_dist_tag: + return candidate if VERSION_PATTERN.fullmatch(candidate): return candidate - # Rejected from here on; what is left is choosing the most useful reason. - - # "Latest" is a tag the installer would not resolve either, so it is an - # error rather than something to normalize -- but name the spelling that - # works. Only where a tag is a legal answer at all; for a pin, the - # dist-tag-shaped branch below correctly says to use a concrete version. - if allow_dist_tag and candidate.lower() in DIST_TAGS: - raise ValueError( - f"Invalid {source}: {candidate!r}. Did you mean {candidate.lower()!r}?" - ) - - # "v2.1.207" is the single most likely typo, and the installer rejects it. - # Say so, rather than printing the pattern and leaving the reader to spot - # the leading "v". Not normalized away: the caller asked for something we - # do not support, and silently installing a different string is worse. - if candidate[:1] in ("v", "V") and VERSION_PATTERN.fullmatch(candidate[1:]): - raise ValueError( - f"Invalid {source}: {candidate!r}. " - f"Did you mean {candidate[1:]!r}? (no leading 'v')" - ) - - if _DIST_TAG_SHAPED.fullmatch(candidate): - raise ValueError( - f"Invalid {source}: {candidate!r} is not a supported dist-tag; " - f"use {_expected(allow_dist_tag)}" - ) - - raise ValueError( - f"Invalid {source}: {version!r}. " - f"Expected {_expected(allow_dist_tag)} matching {VERSION_PATTERN.pattern}" - ) + raise ValueError(f"Invalid {source}: {_rejection(version, allow_dist_tag)}") diff --git a/scripts/download_cli.py b/scripts/download_cli.py index a52d8ab8e..605fd796f 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -45,6 +45,10 @@ # How many times retry_install() runs an attempt before giving up. MAX_INSTALL_ATTEMPTS = 3 +# What an unset CLAUDE_CLI_VERSION means, and -- being the installer's own +# default -- the one value both install paths express by passing no argument. +DEFAULT_VERSION = "latest" + def get_cli_version() -> str: """Get the CLI version to download from environment or default. @@ -56,7 +60,7 @@ def get_cli_version() -> str: ValueError: If CLAUDE_CLI_VERSION is set to something other than a dist-tag ("latest", "stable") or a value matching VERSION_PATTERN. """ - version = os.environ.get("CLAUDE_CLI_VERSION", "latest") + version = os.environ.get("CLAUDE_CLI_VERSION", DEFAULT_VERSION) return version_validation.validate_version( version, source="CLAUDE_CLI_VERSION", allow_dist_tag=True ) @@ -144,13 +148,18 @@ def check_powershell_install_script(script_path: str) -> None: error page actually is -- an XML/HTML document, whose first non-blank character is '<' -- and an empty body, which is never a valid installer. A wrong body is deterministic, so this fails fast instead of retrying. + + '<' on its own would be too blunt: a .ps1 may legitimately open with '<#', + the start of a block comment, which is where about_Comment_Based_Help puts + a script's help block. Exempt that one opener -- no HTML or XML document + begins with it. """ body = Path(script_path).read_bytes() # A UTF-8 BOM is legal in a .ps1 and common in PowerShell-authored files. stripped = body.removeprefix(b"\xef\xbb\xbf").lstrip() if not stripped: _reject_install_script(script_path, "is empty") - if stripped.startswith(b"<"): + if stripped.startswith(b"<") and not stripped.startswith(b"<#"): _reject_install_script(script_path, "looks like an HTML or XML document") @@ -177,7 +186,7 @@ def retry_install(attempt: Callable[[], None]) -> None: try: attempt() return - except (FileNotFoundError, OSError) as e: + except OSError as e: # The command could not be started at all: no `curl`, no `bash`, no # `powershell` on PATH. Deterministic -- a second attempt cannot # make the binary appear -- so fail immediately rather than @@ -208,70 +217,55 @@ def retry_install(attempt: Callable[[], None]) -> None: time.sleep(delay) -def download_cli() -> None: - """Download Claude Code CLI using the official install script.""" - version = get_cli_version() - system = platform.system() +def _powershell(command: str) -> list[str]: + """A PowerShell invocation of ``command``.""" + return ["powershell", "-ExecutionPolicy", "Bypass", "-Command", command] - print(f"Downloading Claude Code CLI version: {version}") - # Build install command based on platform - if system == "Windows": - # Use the PowerShell installer on Windows, downloaded to a file and - # checked before it is executed -- the same download / verify / execute - # sequence as the Unix path below, rather than piping the response body - # straight into `iex`. - # - # Both the script path and the version are handed to PowerShell in the - # environment and referenced by name, so neither is part of the command - # text that PowerShell parses. `$env:NAME` in argument position expands - # to exactly one argument -- PowerShell does not re-split or re-parse - # it -- which is the argv separation the Unix path gets from - # `["bash", script, version]`. - # - # "latest" is the installer's own default, so it is expressed by - # passing no argument at all. Every other accepted value -- a concrete - # version, or the "stable" dist-tag -- is passed through the same - # argument path. - with tempfile.TemporaryDirectory() as tmpdir: - ps_script_path = str(Path(tmpdir) / "install.ps1") - - download_env = {**os.environ, INSTALL_SCRIPT_ENV_VAR: ps_script_path} - download_cmd = [ - "powershell", - "-ExecutionPolicy", - "Bypass", - "-Command", - "$ProgressPreference = 'SilentlyContinue'; " - "Invoke-RestMethod -Uri https://claude.ai/install.ps1 " - f"-OutFile $env:{INSTALL_SCRIPT_ENV_VAR}", - ] - - install_env = dict(download_env) - ps_install = f"& $env:{INSTALL_SCRIPT_ENV_VAR}" - if version != "latest": - ps_install += f" $env:{INSTALL_VERSION_ENV_VAR}" - install_env[INSTALL_VERSION_ENV_VAR] = version - install_cmd = [ - "powershell", - "-ExecutionPolicy", - "Bypass", - "-Command", - ps_install, - ] - - def windows_attempt() -> None: - run_command(download_cmd, env=download_env) - check_powershell_install_script(ps_script_path) - run_command(install_cmd, env=install_env) - - retry_install(windows_attempt) - return - - # Download install.sh to a file and run it directly rather than piping - # curl into bash through a shell string: nothing is interpolated into a - # command line, and check=True sees curl's and bash's exit codes - # separately instead of only the last status of a pipeline. +def _install_on_windows(version: str) -> None: + """Download install.ps1, check the body, then execute it. + + The same download / verify / execute sequence as the Unix path, rather than + piping the response body straight into `iex`. + + Both the script path and the version are handed to PowerShell in the + environment and referenced by name, so neither is part of the command text + that PowerShell parses. `$env:NAME` in argument position expands to exactly + one argument -- PowerShell does not re-split or re-parse it -- which is the + argv separation the Unix path gets from `["bash", script, version]`. + """ + with tempfile.TemporaryDirectory() as tmpdir: + script_path = str(Path(tmpdir) / "install.ps1") + + download_env = {**os.environ, INSTALL_SCRIPT_ENV_VAR: script_path} + download_cmd = _powershell( + "$ProgressPreference = 'SilentlyContinue'; " + "Invoke-RestMethod -Uri https://claude.ai/install.ps1 " + f"-OutFile $env:{INSTALL_SCRIPT_ENV_VAR}" + ) + + install_env = dict(download_env) + install_command = f"& $env:{INSTALL_SCRIPT_ENV_VAR}" + if version != DEFAULT_VERSION: + install_command += f" $env:{INSTALL_VERSION_ENV_VAR}" + install_env[INSTALL_VERSION_ENV_VAR] = version + install_cmd = _powershell(install_command) + + def attempt() -> None: + run_command(download_cmd, env=download_env) + check_powershell_install_script(script_path) + run_command(install_cmd, env=install_env) + + retry_install(attempt) + + +def _install_on_unix(version: str) -> None: + """Download install.sh, check the body, then execute it. + + Run directly rather than piped from curl through a shell string: nothing is + interpolated into a command line, and check=True sees curl's and bash's + exit codes separately instead of only the last status of a pipeline. + """ with tempfile.TemporaryDirectory() as tmpdir: script_path = str(Path(tmpdir) / "install.sh") @@ -291,7 +285,7 @@ def windows_attempt() -> None: "https://claude.ai/install.sh", ] bash_cmd = ["bash", script_path] - if version != "latest": + if version != DEFAULT_VERSION: bash_cmd.append(version) def attempt() -> None: @@ -302,6 +296,22 @@ def attempt() -> None: retry_install(attempt) +def download_cli() -> None: + """Download Claude Code CLI using the official install script. + + Both platform paths pass "latest" by passing no argument at all: it is the + installer's own default. Every other accepted value -- a concrete version, + or the "stable" dist-tag -- goes through the argument path. + """ + version = get_cli_version() + print(f"Downloading Claude Code CLI version: {version}") + + if platform.system() == "Windows": + _install_on_windows(version) + else: + _install_on_unix(version) + + def copy_cli_to_bundle() -> None: """Copy the installed CLI to the package _bundled directory.""" # Find project root (parent of scripts directory) diff --git a/scripts/update_cli_version.py b/scripts/update_cli_version.py index b9fef187b..58efa07df 100755 --- a/scripts/update_cli_version.py +++ b/scripts/update_cli_version.py @@ -6,13 +6,10 @@ import sys from pathlib import Path -# scripts/ is not a package. Running this file directly -- `python -# scripts/update_cli_version.py 1.2.3` -- already puts scripts/ on sys.path, -# but loading it by path (importlib.spec_from_file_location, as the tests do) -# does not. Add it either way so the shared module resolves. Appended, not -# prepended: the tests import this file by path, so the entry outlives the -# import and would otherwise let a future scripts/json.py shadow the stdlib for -# the whole pytest process. +# scripts/ is not a package. Running this file directly already puts scripts/ on +# sys.path, but loading it by path (importlib.spec_from_file_location, as the +# tests do) does not. Add it either way so the shared module resolves. Appended, +# not prepended, for the reason spelled out in download_cli.py. _SCRIPTS_DIR = str(Path(__file__).parent) if _SCRIPTS_DIR not in sys.path: sys.path.append(_SCRIPTS_DIR) diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index 6f6fc2511..00cc653c1 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -623,12 +623,23 @@ def test_non_powershell_body_is_rejected_before_execution( # The download ran once; the installer never, and nothing was retried. assert len(mock_run.call_args_list) == 1 - def test_powershell_body_is_executed(self) -> None: - """A body with a UTF-8 BOM -- legal, and common, in a real .ps1 -- is - not mistaken for a bad one.""" - download_call, install_call = _run_windows_download( - "1.2.3", body=b"\xef\xbb\xbf# install.ps1\nWrite-Host hi\n" - ) + @pytest.mark.parametrize( + "body", + [ + PS_BODY, + # A UTF-8 BOM is legal, and common, in a real .ps1. + b"\xef\xbb\xbf# install.ps1\nWrite-Host hi\n", + # A block comment opens with '<' -- and about_Comment_Based_Help + # puts exactly such a help block at the top of a script, ahead of + # param(), which is what real installers do. The '<' check must not + # mistake it for an HTML error page. + b"<#\n.SYNOPSIS\nInstalls Claude Code.\n#>\nparam([string]$Version)\n", + b"\xef\xbb\xbf\r\n<#\n.SYNOPSIS\nInstalls Claude Code.\n#>\n", + ], + ) + def test_powershell_body_is_executed(self, body: bytes) -> None: + """A valid installer body reaches PowerShell.""" + download_call, install_call = _run_windows_download("1.2.3", body=body) assert "Invoke-RestMethod" in download_call.args[0][-1] assert install_call.args[0][-1].startswith(f"& $env:{SCRIPT_ENV_VAR}") From 1676543b92fbfcd226017cb6de0a04ceaa9e7c56 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Wed, 15 Jul 2026 00:36:45 +0000 Subject: [PATCH 12/13] Fold the two install-path test harnesses into one _unix_install() and _windows_install() had drifted into the same context manager twice: fix platform.system(), stub subprocess.run, put the version in the environment. Only the platform string and the default download stub differed, so a change to how either path is driven had to be made in two places or silently apply to one. Extract that harness into _install(system, version, side_effect); the two named wrappers keep their call signatures and their defaults, and now just name the platform. Tests only -- no production code and no coverage changes. :house: Remote-Dev: homespace --- tests/test_download_cli.py | 42 ++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index 00cc653c1..a219d35a0 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -5,7 +5,7 @@ import subprocess import sys from collections.abc import Callable, Iterator -from contextlib import contextmanager +from contextlib import AbstractContextManager, contextmanager from pathlib import Path from typing import Any from unittest.mock import MagicMock, patch @@ -244,26 +244,38 @@ def side_effect(command: list[str], **kwargs: object) -> MagicMock: @contextmanager -def _unix_install( - version: str, side_effect: _RunSideEffect | None = None +def _install( + system: str, version: str, side_effect: _RunSideEffect ) -> Iterator[MagicMock]: - """Pin download_cli() to the Unix path, yielding the patched subprocess.run. + """Pin download_cli() to one platform's path, yielding the patched run. - ``side_effect`` defaults to a curl that writes a real shebang script; pass - one that raises to exercise a failing command. + The two platform paths differ in what they run, not in how they are driven: + both are reached by fixing platform.system(), stubbing subprocess.run, and + putting the version in the environment. That harness lives here once. """ with ( - patch.object(download_cli.platform, "system", return_value="Linux"), + patch.object(download_cli.platform, "system", return_value=system), patch.object( download_cli.subprocess, "run", - side_effect=side_effect or _fake_curl(), + side_effect=side_effect, ) as mock_run, patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": version}), ): yield mock_run +def _unix_install( + version: str, side_effect: _RunSideEffect | None = None +) -> AbstractContextManager[MagicMock]: + """Pin download_cli() to the Unix path, yielding the patched subprocess.run. + + ``side_effect`` defaults to a curl that writes a real shebang script; pass + one that raises to exercise a failing command. + """ + return _install("Linux", version, side_effect or _fake_curl()) + + def _run_unix_download(version: str, body: bytes = SHEBANG_BODY) -> list[list[str]]: """Run download_cli() on the Unix path, returning the argv of each subprocess.""" with _unix_install(version, _fake_curl(body)) as mock_run: @@ -462,21 +474,11 @@ def side_effect(command: list[str], **kwargs: Any) -> MagicMock: return side_effect -@contextmanager def _windows_install( version: str, side_effect: _RunSideEffect | None = None -) -> Iterator[MagicMock]: +) -> AbstractContextManager[MagicMock]: """Pin download_cli() to the Windows path, yielding the patched run.""" - with ( - patch.object(download_cli.platform, "system", return_value="Windows"), - patch.object( - download_cli.subprocess, - "run", - side_effect=side_effect or _fake_irm(), - ) as mock_run, - patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": version}), - ): - yield mock_run + return _install("Windows", version, side_effect or _fake_irm()) def _run_windows_download(version: str, body: bytes = PS_BODY) -> tuple[Any, Any]: From a8c73e975fd1a5d1e8306cbe311df09afb082c30 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Thu, 16 Jul 2026 00:50:22 +0000 Subject: [PATCH 13/13] Cut the comment bloat, and share the install scaffolding across platforms The install paths, the validator and their tests had accumulated commentary that narrates the code or argues a case rather than explaining anything a reader would otherwise get wrong. Keep the four comments that carry real weight -- why VERSION_PATTERN is unanchored yet used with fullmatch(), why the version reaches PowerShell through the environment, why the allowlist is a strict subset of the installer's grammar, and why a deterministic failure is not retried -- and drop the rest. The two install paths were each carrying their own copy of the tempdir / attempt / retry scaffolding. Both now describe themselves as a plan (where to download, how to check the body, what to execute) and share one runner, which is also the clearest statement that they have the same download-verify-execute shape. Collapse the near-duplicate tests: the accept/reject tables merge into one parametrized case each, the shell-and-stdin invariants into a single test run against both platforms, and the fail-fast cases into one table. Every input and every security guard is still exercised. No behavior change: the validator returns the same value or raises the same message for every one of 7530 (input, allow_dist_tag) pairs, and the install paths issue the same subprocess calls across 162 (platform, version, body) traces. :house: Remote-Dev: homespace --- scripts/_cli_version_validation.py | 107 +++---- scripts/build_wheel.py | 14 +- scripts/download_cli.py | 224 +++++++-------- scripts/update_cli_version.py | 25 +- tests/test_build_wheel.py | 8 +- tests/test_download_cli.py | 442 ++++++++++++----------------- tests/test_update_cli_version.py | 18 +- 7 files changed, 335 insertions(+), 503 deletions(-) diff --git a/scripts/_cli_version_validation.py b/scripts/_cli_version_validation.py index 8571f47e0..e276f0e07 100644 --- a/scripts/_cli_version_validation.py +++ b/scripts/_cli_version_validation.py @@ -1,65 +1,37 @@ r"""Shared validation for Claude Code CLI version strings. -Two scripts constrain the same value: update_cli_version.py writes it into -src/claude_agent_sdk/_cli_version.py, and download_cli.py (reached from -build_wheel.py) reads it back out and hands it to an installer. A second copy -of the rule would let the writer emit a value the reader rejects, so the -pattern and its validation helper live here once. +update_cli_version.py writes the version into src/claude_agent_sdk/_cli_version.py +and download_cli.py reads it back out and hands it to an installer, so the rule +lives here once rather than in two copies that could drift apart. -The installer is the authority on what a version may be. install.sh enforces +The installer is the authority on what a version may be. install.sh (and +install.ps1) enforce ^(stable|latest|[0-9]+\.[0-9]+\.[0-9]+(-[^[:space:]]+)?)$ -and install.ps1 enforces the same rule, so a value this module admits but the -installer does not is not a version -- it is an error we defer to install time, -where it surfaces behind a retry loop and a misleading "Error downloading CLI" -headline. VERSION_PATTERN therefore mirrors that grammar: three dot-separated -numeric components with an optional prerelease/build suffix, which covers both -releases ("2.1.207") and dev builds -("2.1.146-dev.20260519.t105443.shaece3dab"). - -We deliberately accept a strict *subset* of what the installer allows: the -installer's suffix is `-[^\s]+`, which would admit quotes, backslashes, -semicolons and every other non-space character, so the suffix here is narrowed -to the alphanumeric/dot/plus/hyphen set that real versions use. Never widen -this pattern back toward the installer's. - -That narrowing is a security boundary, not just input hygiene: - - * update_cli_version.update_cli_version() writes the version into a Python - string literal in a real source file, so it must never admit a double - quote, a backslash, or a newline. - * download_cli.download_cli() hands the version to an installer. Neither of - its paths interpolates it into a command string -- Unix passes it as its - own argv element, Windows passes it in the environment -- so for that - caller the allowlist is defense in depth rather than the only barrier. - -"latest" and "stable" are the installer's dist-tags. Both are *moving*: they -resolve to whatever build is current at install time. That is fine for a -download, and wrong for a pin -- _cli_version.py is the only record of which -build went into the wheels, so it must name one concrete build. Hence -``allow_dist_tag``. - -Widening any of this requires re-reading tests/test_download_cli.py and -tests/test_update_cli_version.py. - -VERSION_PATTERN is deliberately unanchored, and matched with fullmatch() -rather than match(): with "^...$" a swap to match() would silently accept a -trailing newline ("1.0.0\n"); unanchored, the same swap accepts obvious -prefixes like "1.0.0; id" and fails immediately in tests. +VERSION_PATTERN admits a deliberate *subset* of that: the installer's `-[^\s]+` +suffix would accept quotes, backslashes and semicolons, so the suffix here is +narrowed to the characters real versions use. Never widen it back toward the +installer's -- update_cli_version.py writes this value into a Python string +literal in a real source file. + +"latest" and "stable" are the installer's dist-tags, and they are *moving*: +fine for a download, wrong for a pin, since _cli_version.py is the only record +of which build went into the wheels. Hence ``allow_dist_tag``. + +VERSION_PATTERN is deliberately unanchored, and matched with fullmatch(): with +"^...$" a swap to match() would silently accept a trailing newline ("1.0.0\n"); +unanchored, the same swap accepts an obvious prefix like "1.0.0; id" and fails +loudly in tests. """ import re -# A concrete version: MAJOR.MINOR.PATCH with an optional suffix. The suffix is -# the installer's `-[^\s]+` narrowed to characters that appear in real -# versions -- see the module docstring. VERSION_PATTERN = re.compile(r"[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z.+-]+)?") -# The moving tags the installer resolves at install time. Matched exactly: -# install.sh's own grammar is case-sensitive, so "Latest" is not a tag it would -# resolve, and accepting it here would widen what reaches a real network -# install. "Latest" is rejected, with the lowercase spelling suggested. +# Matched exactly: install.sh's grammar is case-sensitive, so "Latest" is not a +# tag it would resolve, and accepting it here would widen what reaches a real +# network install. DIST_TAGS = ("latest", "stable") # Anything word-shaped that is not a version: "next", "beta", "nightly". Named @@ -79,8 +51,8 @@ def _expected(allow_dist_tag: bool) -> str: def _rejection(version: str, allow_dist_tag: bool) -> str: """Why ``version`` is unusable -- the most specific reason that applies. - The caller prefixes "Invalid : ", so each reason here reads as the - rest of that sentence. + The caller prefixes "Invalid : ", so each reason reads as the rest + of that sentence. """ candidate = version.strip() @@ -93,17 +65,13 @@ def _rejection(version: str, allow_dist_tag: bool) -> str: f"a version matching {VERSION_PATTERN.pattern}" ) - # "Latest" is a tag the installer would not resolve either, so it is an - # error rather than something to normalize -- but name the spelling that - # works. Only where a tag is a legal answer at all; for a pin, the + # Only where a tag is a legal answer at all; when pinning, the # dist-tag-shaped branch below correctly says to use a concrete version. if allow_dist_tag and candidate.lower() in DIST_TAGS: return f"{candidate!r}. Did you mean {candidate.lower()!r}?" - # "v2.1.207" is the single most likely typo, and the installer rejects it. - # Say so, rather than printing the pattern and leaving the reader to spot - # the leading "v". Not normalized away: the caller asked for something we - # do not support, and silently installing a different string is worse. + # Never normalized away: the caller asked for something we do not support, + # and silently installing a different string is worse. if candidate[:1] in ("v", "V") and VERSION_PATTERN.fullmatch(candidate[1:]): return f"{candidate!r}. Did you mean {candidate[1:]!r}? (no leading 'v')" @@ -113,8 +81,8 @@ def _rejection(version: str, allow_dist_tag: bool) -> str: f"use {_expected(allow_dist_tag)}" ) - # Nothing recognizable. Name the raw value, not the stripped one: if the - # whitespace is the problem, the reader has to be able to see it. + # Name the raw value, not the stripped one: if the whitespace is the + # problem, the reader has to be able to see it. return ( f"{version!r}. Expected {_expected(allow_dist_tag)} " f"matching {VERSION_PATTERN.pattern}" @@ -124,22 +92,17 @@ def _rejection(version: str, allow_dist_tag: bool) -> str: def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: """Return the usable form of ``version``, or raise. - Surrounding whitespace is stripped before anything else: a trailing "\\n" - from a file read, a "\\r" from a CRLF checkout, or a stray space from YAML - is unambiguous in intent, and the stripped value is what the caller gets - back and must use downstream. + Surrounding whitespace is stripped first -- a trailing "\\n" from a file + read or a "\\r" from a CRLF checkout is unambiguous in intent -- and the + stripped value is what the caller gets back and must use downstream. Args: version: The candidate version string. - source: Name of where the value came from, used in the error message + source: Where the value came from, named in the error message (e.g. "CLAUDE_CLI_VERSION"). allow_dist_tag: Whether a moving dist-tag ("latest", "stable") is acceptable. It is for a download, which resolves it at install - time; it is not for a value pinned into _cli_version.py, which must - name the one concrete build that went into the wheels. - - Returns: - The stripped version. + time; it is not for a value pinned into _cli_version.py. Raises: ValueError: If ``version`` is neither an allowed dist-tag nor a @@ -147,8 +110,6 @@ def validate_version(version: str, *, source: str, allow_dist_tag: bool) -> str: """ candidate = version.strip() - # A dist-tag fails VERSION_PATTERN, so it is recognized by name -- exactly, - # matching the installer's own case-sensitive grammar. if candidate in DIST_TAGS and allow_dist_tag: return candidate diff --git a/scripts/build_wheel.py b/scripts/build_wheel.py index a47ad2e6a..d77fe0fd6 100755 --- a/scripts/build_wheel.py +++ b/scripts/build_wheel.py @@ -24,10 +24,7 @@ from pathlib import Path from typing import NoReturn -# scripts/ is not a package. Running this file directly already puts scripts/ on -# sys.path, but loading it by path (importlib.spec_from_file_location, as the -# tests do) does not. Add it either way so the shared module resolves. Appended, -# not prepended, for the reason spelled out in download_cli.py. +# scripts/ is not a package; see the note in download_cli.py. _SCRIPTS_DIR = str(Path(__file__).parent) if _SCRIPTS_DIR not in sys.path: sys.path.append(_SCRIPTS_DIR) @@ -103,12 +100,9 @@ def _fail_unpinned(reason: str) -> NoReturn: def get_bundled_cli_version() -> str: """Get the CLI version that should be bundled from _cli_version.py. - Fails the build rather than falling back to a dist-tag. _cli_version.py is - the only record of which CLI build goes into the wheels; defaulting to the - moving "latest" when the pin is missing, unparseable, or itself a dist-tag - would silently publish an unpinned -- and, across the release matrix, - potentially inconsistent -- set of wheels. update_cli_version.py refuses to - *write* a moving tag; this is the same rule on the read side. + Fails the build rather than falling back to a dist-tag: _cli_version.py is + the only record of which CLI build goes into the wheels, and each of the + release matrix's runners would resolve a moving "latest" independently. """ if not CLI_VERSION_FILE.exists(): _fail_unpinned(f"{CLI_VERSION_FILE} does not exist") diff --git a/scripts/download_cli.py b/scripts/download_cli.py index 605fd796f..c0ac39a75 100755 --- a/scripts/download_cli.py +++ b/scripts/download_cli.py @@ -15,15 +15,12 @@ import time from collections.abc import Callable from pathlib import Path -from typing import NoReturn - -# scripts/ is not a package. Running this file directly -- as build_wheel.py -# does, via `python scripts/download_cli.py` -- already puts scripts/ on -# sys.path, but loading it by path (importlib.spec_from_file_location, as the -# tests do) does not. Add it either way so the shared module resolves. Appended, -# not prepended: the tests import this file by path, so the entry outlives the -# import and would otherwise let a future scripts/json.py shadow the stdlib for -# the whole pytest process. +from typing import NamedTuple, NoReturn + +# scripts/ is not a package. Running this file directly puts scripts/ on +# sys.path, but loading it by path (as the tests do) does not. Appended, not +# prepended: the tests' entry outlives the import, and a prepended scripts/ +# could shadow a stdlib module for the whole pytest process. _SCRIPTS_DIR = str(Path(__file__).parent) if _SCRIPTS_DIR not in sys.path: sys.path.append(_SCRIPTS_DIR) @@ -33,13 +30,12 @@ # Re-exported: this module's callers and tests refer to download_cli.VERSION_PATTERN. VERSION_PATTERN = version_validation.VERSION_PATTERN -# The Windows installer reads the version out of the environment under this -# name instead of having it spliced into the PowerShell command text. +# The Windows installer reads the version and the downloaded script's path out +# of the environment under these names, so neither is part of the command text +# PowerShell parses. `$env:NAME` in argument position expands to exactly one +# argument -- the same argv separation the Unix path gets from +# `["bash", script, version]`. INSTALL_VERSION_ENV_VAR = "CLAUDE_CLI_INSTALL_VERSION" - -# Where the downloaded PowerShell installer is written. Handed to PowerShell in -# the environment for the same reason the version is: a temp path is not part -# of the command text PowerShell parses. INSTALL_SCRIPT_ENV_VAR = "CLAUDE_CLI_INSTALL_SCRIPT" # How many times retry_install() runs an attempt before giving up. @@ -126,7 +122,7 @@ def _reject_install_script(script_path: str, reason: str) -> NoReturn: def check_install_script(script_path: str) -> None: - """Reject a downloaded install script that is not a shell script. + """Reject a downloaded install.sh that is not a shell script. claude.ai answers unknown paths with HTTP 200 and an HTML body, which `curl -f` cannot detect, so check the shebang before executing the file. @@ -141,18 +137,14 @@ def check_install_script(script_path: str) -> None: def check_powershell_install_script(script_path: str) -> None: """Reject a downloaded install.ps1 that is not a PowerShell script. - The Windows counterpart of check_install_script(): the same HTTP 200 + - HTML body that `curl -f` cannot detect is also invisible to - Invoke-RestMethod, and would otherwise be parsed and executed by - PowerShell. A .ps1 has no shebang to check, so instead reject what the - error page actually is -- an XML/HTML document, whose first non-blank - character is '<' -- and an empty body, which is never a valid installer. - A wrong body is deterministic, so this fails fast instead of retrying. + The same HTTP 200 + HTML body is invisible to Invoke-RestMethod. A .ps1 has + no shebang to check, so reject what the error page actually is -- an + XML/HTML document, whose first non-blank character is '<' -- and an empty + body. A wrong body is deterministic, so this fails fast instead of retrying. - '<' on its own would be too blunt: a .ps1 may legitimately open with '<#', - the start of a block comment, which is where about_Comment_Based_Help puts - a script's help block. Exempt that one opener -- no HTML or XML document - begins with it. + '<' alone would be too blunt: a .ps1 may legitimately open with '<#', the + comment-based-help block real installers start with. No HTML or XML document + begins with that, so exempt it. """ body = Path(script_path).read_bytes() # A UTF-8 BOM is legal in a .ps1 and common in PowerShell-authored files. @@ -177,11 +169,6 @@ def retry_install(attempt: Callable[[], None]) -> None: # Small jitter to stagger parallel matrix builds hitting the same endpoint time.sleep(random.uniform(0, 5)) - # The failure is reported from inside the handler for the last attempt, so - # the error is in scope and non-None by construction. Stashing it in a - # `CalledProcessError | None` and reading it after the loop instead would - # rely on the reader -- and the type checker -- knowing that range(1, 4) - # cannot be empty. for attempt_num in range(1, MAX_INSTALL_ATTEMPTS + 1): try: attempt() @@ -189,14 +176,9 @@ def retry_install(attempt: Callable[[], None]) -> None: except OSError as e: # The command could not be started at all: no `curl`, no `bash`, no # `powershell` on PATH. Deterministic -- a second attempt cannot - # make the binary appear -- so fail immediately rather than - # sleeping through three of them and then blaming the download. - # The old `bash -c` form surfaced this as a plain exit 127; keep it - # just as legible now that the binaries are exec'd directly. - print( - f"Error: could not run the install command: {e}", - file=sys.stderr, - ) + # make the binary appear -- so fail immediately rather than sleeping + # through three of them and then blaming the download. + print(f"Error: could not run the install command: {e}", file=sys.stderr) sys.exit(1) except subprocess.CalledProcessError as e: if attempt_num == MAX_INSTALL_ATTEMPTS: @@ -217,99 +199,105 @@ def retry_install(attempt: Callable[[], None]) -> None: time.sleep(delay) -def _powershell(command: str) -> list[str]: - """A PowerShell invocation of ``command``.""" - return ["powershell", "-ExecutionPolicy", "Bypass", "-Command", command] +# One command to run: its argv, and the environment to run it with (None +# inherits ours). +_Command = tuple[list[str], dict[str, str] | None] -def _install_on_windows(version: str) -> None: - """Download install.ps1, check the body, then execute it. +class _InstallPlan(NamedTuple): + """How one platform downloads the installer, checks its body, and runs it.""" - The same download / verify / execute sequence as the Unix path, rather than - piping the response body straight into `iex`. + script_path: str + download: _Command + check: Callable[[str], None] + install: _Command - Both the script path and the version are handed to PowerShell in the - environment and referenced by name, so neither is part of the command text - that PowerShell parses. `$env:NAME` in argument position expands to exactly - one argument -- PowerShell does not re-split or re-parse it -- which is the - argv separation the Unix path gets from `["bash", script, version]`. - """ - with tempfile.TemporaryDirectory() as tmpdir: - script_path = str(Path(tmpdir) / "install.ps1") - - download_env = {**os.environ, INSTALL_SCRIPT_ENV_VAR: script_path} - download_cmd = _powershell( - "$ProgressPreference = 'SilentlyContinue'; " - "Invoke-RestMethod -Uri https://claude.ai/install.ps1 " - f"-OutFile $env:{INSTALL_SCRIPT_ENV_VAR}" - ) - - install_env = dict(download_env) - install_command = f"& $env:{INSTALL_SCRIPT_ENV_VAR}" - if version != DEFAULT_VERSION: - install_command += f" $env:{INSTALL_VERSION_ENV_VAR}" - install_env[INSTALL_VERSION_ENV_VAR] = version - install_cmd = _powershell(install_command) - def attempt() -> None: - run_command(download_cmd, env=download_env) - check_powershell_install_script(script_path) - run_command(install_cmd, env=install_env) +def _powershell(command: str) -> list[str]: + """A PowerShell invocation of ``command``.""" + return ["powershell", "-ExecutionPolicy", "Bypass", "-Command", command] - retry_install(attempt) +def _windows_plan(tmpdir: str, version: str) -> _InstallPlan: + script_path = str(Path(tmpdir) / "install.ps1") -def _install_on_unix(version: str) -> None: - """Download install.sh, check the body, then execute it. + download_env = {**os.environ, INSTALL_SCRIPT_ENV_VAR: script_path} + download_cmd = _powershell( + "$ProgressPreference = 'SilentlyContinue'; " + "Invoke-RestMethod -Uri https://claude.ai/install.ps1 " + f"-OutFile $env:{INSTALL_SCRIPT_ENV_VAR}" + ) - Run directly rather than piped from curl through a shell string: nothing is - interpolated into a command line, and check=True sees curl's and bash's - exit codes separately instead of only the last status of a pipeline. - """ - with tempfile.TemporaryDirectory() as tmpdir: - script_path = str(Path(tmpdir) / "install.sh") - - # -L follows the cross-host redirect to the bootstrap script. - # --retry-all-errors covers 429 from claude.ai when multiple matrix - # jobs fetch install.sh simultaneously. - curl_cmd = [ - "curl", - "-fsSL", - "--retry", - "5", - "--retry-delay", - "2", - "--retry-all-errors", - "-o", - script_path, - "https://claude.ai/install.sh", - ] - bash_cmd = ["bash", script_path] - if version != DEFAULT_VERSION: - bash_cmd.append(version) + install_env = dict(download_env) + install_command = f"& $env:{INSTALL_SCRIPT_ENV_VAR}" + if version != DEFAULT_VERSION: + install_command += f" $env:{INSTALL_VERSION_ENV_VAR}" + install_env[INSTALL_VERSION_ENV_VAR] = version + + return _InstallPlan( + script_path=script_path, + download=(download_cmd, download_env), + check=check_powershell_install_script, + install=(_powershell(install_command), install_env), + ) - def attempt() -> None: - run_command(curl_cmd) - check_install_script(script_path) - run_command(bash_cmd) - retry_install(attempt) +def _unix_plan(tmpdir: str, version: str) -> _InstallPlan: + script_path = str(Path(tmpdir) / "install.sh") + + # -L follows the cross-host redirect to the bootstrap script. + # --retry-all-errors covers 429 from claude.ai when multiple matrix jobs + # fetch install.sh simultaneously. + curl_cmd = [ + "curl", + "-fsSL", + "--retry", + "5", + "--retry-delay", + "2", + "--retry-all-errors", + "-o", + script_path, + "https://claude.ai/install.sh", + ] + bash_cmd = ["bash", script_path] + if version != DEFAULT_VERSION: + bash_cmd.append(version) + + return _InstallPlan( + script_path=script_path, + download=(curl_cmd, None), + check=check_install_script, + install=(bash_cmd, None), + ) def download_cli() -> None: """Download Claude Code CLI using the official install script. - Both platform paths pass "latest" by passing no argument at all: it is the - installer's own default. Every other accepted value -- a concrete version, - or the "stable" dist-tag -- goes through the argument path. + Both platforms download the installer to a temp file, check its body, and + only then execute it -- rather than piping the response straight into a + shell or into `iex`. The whole sequence is retried, so a truncated body is + never reused across attempts. + + Both pass "latest" by passing no argument at all: it is the installer's own + default. Every other accepted value -- a concrete version, or "stable" -- + goes through the argument path. """ version = get_cli_version() print(f"Downloading Claude Code CLI version: {version}") - if platform.system() == "Windows": - _install_on_windows(version) - else: - _install_on_unix(version) + build_plan = _windows_plan if platform.system() == "Windows" else _unix_plan + + with tempfile.TemporaryDirectory() as tmpdir: + plan = build_plan(tmpdir, version) + + def attempt() -> None: + run_command(*plan.download) + plan.check(plan.script_path) + run_command(*plan.install) + + retry_install(attempt) def copy_cli_to_bundle() -> None: @@ -368,12 +356,10 @@ def main() -> None: if __name__ == "__main__": - # get_cli_version() raises on a bad CLAUDE_CLI_VERSION, and this script - # runs as a build step. Report that the way every other failure here is - # reported -- one line on stderr, exit 1 -- instead of letting a traceback - # out. The shared validator keeps raising, because update_cli_version.py - # and the tests read the exception; only the entry point turns it into an - # exit status. Matches update_cli_version.py's __main__. + # This script runs as a build step, so report a bad CLAUDE_CLI_VERSION the + # way every other failure here is reported -- one line on stderr, exit 1 -- + # instead of letting a traceback out. The shared validator keeps raising: + # only the entry point turns it into an exit status. try: main() except ValueError as exc: diff --git a/scripts/update_cli_version.py b/scripts/update_cli_version.py index 58efa07df..92affedbb 100755 --- a/scripts/update_cli_version.py +++ b/scripts/update_cli_version.py @@ -6,10 +6,7 @@ import sys from pathlib import Path -# scripts/ is not a package. Running this file directly already puts scripts/ on -# sys.path, but loading it by path (importlib.spec_from_file_location, as the -# tests do) does not. Add it either way so the shared module resolves. Appended, -# not prepended, for the reason spelled out in download_cli.py. +# scripts/ is not a package; see the note in download_cli.py. _SCRIPTS_DIR = str(Path(__file__).parent) if _SCRIPTS_DIR not in sys.path: sys.path.append(_SCRIPTS_DIR) @@ -30,14 +27,10 @@ def update_cli_version(new_version: str, version_path: Path | None = None) -> No target file has no ``__cli_version__`` assignment to replace. The file is left untouched in both cases. """ - # _cli_version.py is a real source file that gets imported, and the value - # written here is later read back by build_wheel.py and passed to - # download_cli.py. Validate before touching the file: an unvalidated value - # closes the string literal and injects arbitrary Python. The moving - # dist-tags ("latest", "stable") are rejected -- unlike the download, which - # resolves them at install time, the pinned file has to name the one build - # that went into the wheels. Write the *validated* value: it is the input - # with surrounding whitespace stripped. + # Validate before touching the file: this writes into a real source file + # that later gets imported, so an unvalidated value closes the string + # literal and injects arbitrary Python. The validated value is the input + # with surrounding whitespace stripped -- write that, not the raw input. new_version = version_validation.validate_version( new_version, source="CLI version", allow_dist_tag=False ) @@ -48,12 +41,8 @@ def update_cli_version(new_version: str, version_path: Path | None = None) -> No # json.dumps() rather than an f-string: it always emits a closed, # double-quoted, fully escaped literal, so a widened VERSION_PATTERN could - # never make this file unparseable or inject code. It is byte-identical to - # `"{new_version}"` for every version the pattern admits today. Note it is - # only a containment barrier: a version holding a quote would emit `"a\"b"`, - # which build_wheel.py's `"([^"]+)"` reader truncates to `a\` rather than - # round-tripping. repr() is not an option -- it emits single quotes, which - # that same reader would not match at all. + # never make this file unparseable or inject code. repr() is not an option + # -- it emits single quotes, which build_wheel.py's reader would not match. literal = f"__cli_version__ = {json.dumps(new_version)}" # A callable replacement, because re.sub() applies backslash-escape diff --git a/tests/test_build_wheel.py b/tests/test_build_wheel.py index 8774816b2..6fd2f66a9 100644 --- a/tests/test_build_wheel.py +++ b/tests/test_build_wheel.py @@ -74,11 +74,9 @@ def pinned(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: class TestGetBundledCliVersion: """The CLI pin is the only record of which build goes into the wheels. - An unreadable pin must fail the build. Falling back to the moving "latest" - dist-tag -- which download_cli.py accepts -- would publish an unpinned set - of wheels, each of the five release runners resolving "latest" - independently. update_cli_version.py refuses to write a moving tag; this is - the same rule on the read side. + An unreadable pin must fail the build: falling back to the moving "latest" + -- which download_cli.py accepts -- would publish an unpinned set of wheels, + each of the five release runners resolving it independently. """ def test_reads_the_pinned_version(self, pinned: Path) -> None: diff --git a/tests/test_download_cli.py b/tests/test_download_cli.py index a219d35a0..6d2d5f68c 100644 --- a/tests/test_download_cli.py +++ b/tests/test_download_cli.py @@ -23,6 +23,7 @@ DEV_VERSION = "2.1.146-dev.20260519.t105443.shaece3dab" ENV_VAR = download_cli.INSTALL_VERSION_ENV_VAR +SCRIPT_ENV_VAR = download_cli.INSTALL_SCRIPT_ENV_VAR class TestGetCliVersion: @@ -44,8 +45,7 @@ def test_default_is_latest(self, monkeypatch: pytest.MonkeyPatch) -> None: "latest", # `stable` is a moving tag the installer resolves, like `latest`: # allowed here (a download resolves it at install time) and - # rejected by update_cli_version.py (a pin must name one concrete - # build). + # rejected by update_cli_version.py (a pin must name one build). "stable", "1.2.3", "2.1.195", @@ -58,27 +58,6 @@ def test_accepted(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: monkeypatch.setenv("CLAUDE_CLI_VERSION", version) assert download_cli.get_cli_version() == version - @pytest.mark.parametrize( - ("version", "suggestion"), - [ - ("LATEST", "latest"), - ("Latest", "latest"), - ("STABLE", "stable"), - ("Stable", "stable"), - ], - ) - def test_dist_tags_are_case_sensitive( - self, monkeypatch: pytest.MonkeyPatch, version: str, suggestion: str - ) -> None: - """The installer's own grammar is case-sensitive, so "Latest" is not a - tag it would resolve. Reject it -- naming the spelling that works -- - rather than quietly turning it into a live install.""" - monkeypatch.setenv("CLAUDE_CLI_VERSION", version) - with pytest.raises(ValueError, match="Invalid CLAUDE_CLI_VERSION") as excinfo: - download_cli.get_cli_version() - - assert f"Did you mean {suggestion!r}?" in str(excinfo.value) - @pytest.mark.parametrize( ("version", "expected"), [ @@ -92,70 +71,64 @@ def test_dist_tags_are_case_sensitive( def test_surrounding_whitespace_is_stripped( self, monkeypatch: pytest.MonkeyPatch, version: str, expected: str ) -> None: - """A trailing newline from a file read or a CRLF checkout is - unambiguous in intent; the stripped value is what is used downstream.""" monkeypatch.setenv("CLAUDE_CLI_VERSION", version) assert download_cli.get_cli_version() == expected @pytest.mark.parametrize( - "version", + ("version", "expected_message"), [ - "1.0.0; touch /tmp/pwned", - "--help", - "-s", - "$(id)", - "`id`", - "1.0.0 && id", - "1.0.0 | id", - "1.0.0\nid", - "1.0.0 2.0.0", - "$VERSION", - "../../etc/passwd", - "", - ".1.2.3", + # Injection and flag shapes. + ("1.0.0; touch /tmp/pwned", None), + ("--help", None), + ("-s", None), + ("$(id)", None), + ("`id`", None), + ("1.0.0 && id", None), + ("1.0.0 | id", None), + ("1.0.0\nid", None), + ("1.0.0 2.0.0", None), + ("$VERSION", None), + ("../../etc/passwd", None), + ("", None), + (".1.2.3", None), # Not versions at all -- the old allowlist took these for concrete # versions and let the installer reject them 11 seconds later. - "0", - "1.2", - "1.2.3.4", - "1.2.3+build.4", # the installer's grammar has no bare "+" suffix - "v2.1.207", - "next", - "beta", - "nightly", + ("0", None), + ("1.2", None), + ("1.2.3.4", None), + ("1.2.3+build.4", None), # the installer has no bare "+" suffix + # The likeliest typo is told what to type, never silently rewritten + # into a different version than the caller asked for. + ("v2.1.207", "Did you mean '2.1.207'? (no leading 'v')"), + ("V1.2.3", "Did you mean '1.2.3'? (no leading 'v')"), + # Word-shaped, but not a tag the installer resolves. + ("next", "not a supported dist-tag; use 'latest', 'stable'"), + ("beta", "not a supported dist-tag; use 'latest', 'stable'"), + ("nightly", "not a supported dist-tag; use 'latest', 'stable'"), + # The installer's grammar is case-sensitive, so "Latest" is not a + # tag it would resolve: reject it -- naming the spelling that works + # -- rather than quietly turning it into a live install. + ("LATEST", "Did you mean 'latest'?"), + ("Latest", "Did you mean 'latest'?"), + ("STABLE", "Did you mean 'stable'?"), + ("Stable", "Did you mean 'stable'?"), ], ) - def test_rejected(self, monkeypatch: pytest.MonkeyPatch, version: str) -> None: - monkeypatch.setenv("CLAUDE_CLI_VERSION", version) - with pytest.raises(ValueError, match="Invalid CLAUDE_CLI_VERSION"): - download_cli.get_cli_version() - - @pytest.mark.parametrize("version", ["v2.1.207", "V1.2.3"]) - def test_leading_v_is_named_not_normalized( - self, monkeypatch: pytest.MonkeyPatch, version: str + def test_rejected( + self, + monkeypatch: pytest.MonkeyPatch, + version: str, + expected_message: str | None, ) -> None: - """The likeliest typo gets told what to type, and is never silently - rewritten into a different version than the caller asked for.""" monkeypatch.setenv("CLAUDE_CLI_VERSION", version) - with pytest.raises(ValueError) as excinfo: - download_cli.get_cli_version() - - message = str(excinfo.value) - assert f"Did you mean {version[1:]!r}?" in message - assert "no leading 'v'" in message - - @pytest.mark.parametrize("version", ["next", "beta", "nightly"]) - def test_unsupported_dist_tag_is_named( - self, monkeypatch: pytest.MonkeyPatch, version: str - ) -> None: - monkeypatch.setenv("CLAUDE_CLI_VERSION", version) - with pytest.raises(ValueError) as excinfo: + with pytest.raises(ValueError, match="Invalid CLAUDE_CLI_VERSION") as excinfo: download_cli.get_cli_version() message = str(excinfo.value) - assert "not a supported dist-tag" in message - assert "'latest'" in message - assert "'stable'" in message + # The offending value is always named, so the reader can see it. + assert repr(version) in message or version in message + if expected_message is not None: + assert expected_message in message @pytest.mark.parametrize("version", ["2.1.207", DEV_VERSION, "1.0.0-alpha"]) def test_pattern_admits_every_published_version_shape(self, version: str) -> None: @@ -163,29 +136,15 @@ def test_pattern_admits_every_published_version_shape(self, version: str) -> Non rejecting anything real, releases or dev builds.""" assert download_cli.VERSION_PATTERN.fullmatch(version) - def test_error_names_the_offending_value( - self, monkeypatch: pytest.MonkeyPatch - ) -> None: - monkeypatch.setenv("CLAUDE_CLI_VERSION", "1.0.0; id") - with pytest.raises(ValueError) as excinfo: - download_cli.get_cli_version() - assert "1.0.0; id" in str(excinfo.value) - @pytest.mark.parametrize( "char", [" ", ";", "$", "`", '"', "'", "(", ")", "&", "|", "\n", "\r"], ) def test_version_pattern_admits_no_shell_metacharacters(self, char: str) -> None: - """VERSION_PATTERN must never admit a shell metacharacter. - - No install path depends on this any more: the Unix path passes the - version as its own argv element, and the Windows path hands it to - PowerShell in the environment rather than in the command text. The - allowlist is still the invariant we want -- a version that can express - `;` or `$` is not a version -- so keep it as defense in depth. Widening - the pattern (to permit `_` or `~`, say) must not let any of these - through. - """ + """No install path depends on this any more -- the version is an argv + element on Unix and an environment variable on Windows -- but a version + that can express `;` or `$` is not a version. Keep it as defense in + depth: widening the pattern must not let any of these through.""" assert not download_cli.VERSION_PATTERN.fullmatch(char) assert not download_cli.VERSION_PATTERN.fullmatch(f"1.2.3{char}") assert not download_cli.VERSION_PATTERN.fullmatch(f"1.2.3{char}whoami") @@ -198,27 +157,6 @@ def test_version_pattern_is_unanchored(self) -> None: assert "$" not in download_cli.VERSION_PATTERN.pattern assert not download_cli.VERSION_PATTERN.fullmatch("1.0.0\n") - def test_script_validates_when_run_directly(self, tmp_path: Path) -> None: - """build_wheel.py runs this file as a subprocess, so the shared - validation module must import without scripts/ being a package. - - PATH is emptied so that if validation ever regresses this reaches a - missing `curl` instead of really installing the CLI; sys.executable is - absolute, so python itself still starts. The timeout bounds the retry - sleeps on that path. - """ - script = Path(__file__).parent.parent / "scripts" / "download_cli.py" - result = subprocess.run( - [sys.executable, str(script)], - env=os.environ | {"CLAUDE_CLI_VERSION": "1.0.0; id", "PATH": str(tmp_path)}, - capture_output=True, - text=True, - timeout=60, - ) - assert result.returncode != 0 - assert "Invalid CLAUDE_CLI_VERSION" in result.stderr - assert "ModuleNotFoundError" not in result.stderr - @pytest.fixture def no_sleep() -> Iterator[None]: @@ -228,6 +166,7 @@ def no_sleep() -> Iterator[None]: SHEBANG_BODY = b"#!/bin/bash\necho install\n" +PS_BODY = b"# install.ps1\nWrite-Host installing\n" _RunSideEffect = Callable[..., MagicMock] @@ -243,6 +182,24 @@ def side_effect(command: list[str], **kwargs: object) -> MagicMock: return side_effect +def _fake_irm(body: bytes = PS_BODY) -> _RunSideEffect: + """A subprocess.run side effect making the download step write ``body``. + + The Windows download step names its output file only in the environment, so + that is where the fake finds the path to write -- the same indirection the + real PowerShell command resolves. + """ + + def side_effect(command: list[str], **kwargs: Any) -> MagicMock: + env = kwargs.get("env") or {} + path = env.get(SCRIPT_ENV_VAR) + if path and "Invoke-RestMethod" in command[-1]: + Path(path).write_bytes(body) + return MagicMock() + + return side_effect + + @contextmanager def _install( system: str, version: str, side_effect: _RunSideEffect @@ -255,27 +212,27 @@ def _install( """ with ( patch.object(download_cli.platform, "system", return_value=system), - patch.object( - download_cli.subprocess, - "run", - side_effect=side_effect, - ) as mock_run, + patch.object(download_cli.subprocess, "run", side_effect=side_effect) as run, patch.dict(download_cli.os.environ, {"CLAUDE_CLI_VERSION": version}), ): - yield mock_run + yield run def _unix_install( version: str, side_effect: _RunSideEffect | None = None ) -> AbstractContextManager[MagicMock]: - """Pin download_cli() to the Unix path, yielding the patched subprocess.run. - - ``side_effect`` defaults to a curl that writes a real shebang script; pass - one that raises to exercise a failing command. - """ + """Pin download_cli() to the Unix path; ``side_effect`` defaults to a curl + that writes a real shebang script.""" return _install("Linux", version, side_effect or _fake_curl()) +def _windows_install( + version: str, side_effect: _RunSideEffect | None = None +) -> AbstractContextManager[MagicMock]: + """Pin download_cli() to the Windows path, yielding the patched run.""" + return _install("Windows", version, side_effect or _fake_irm()) + + def _run_unix_download(version: str, body: bytes = SHEBANG_BODY) -> list[list[str]]: """Run download_cli() on the Unix path, returning the argv of each subprocess.""" with _unix_install(version, _fake_curl(body)) as mock_run: @@ -283,6 +240,40 @@ def _run_unix_download(version: str, body: bytes = SHEBANG_BODY) -> list[list[st return [call.args[0] for call in mock_run.call_args_list] +def _run_windows_download(version: str, body: bytes = PS_BODY) -> tuple[Any, Any]: + """Run download_cli() on Windows, returning its (download, install) calls.""" + with _windows_install(version, _fake_irm(body)) as mock_run: + download_cli.download_cli() + + download_call, install_call = mock_run.call_args_list + return download_call, install_call + + +@pytest.mark.usefixtures("no_sleep") +@pytest.mark.parametrize( + ("installer", "side_effect"), + [(_unix_install, _fake_curl()), (_windows_install, _fake_irm())], + ids=["unix", "windows"], +) +def test_no_command_uses_a_shell_or_inherits_stdin( + installer: Callable[..., AbstractContextManager[MagicMock]], + side_effect: _RunSideEffect, +) -> None: + """install.sh runs `claude install`, which branches on `[ -t 0 ]`. The old + `curl | bash` gave it a pipe; it must never inherit a real TTY. And nothing + may reintroduce a shell, on either platform.""" + with installer("1.2.3", side_effect) as mock_run: + download_cli.download_cli() + + assert mock_run.call_args_list + for call in mock_run.call_args_list: + assert call.kwargs["stdin"] is subprocess.DEVNULL, ( + f"{call.args[0][0]} may inherit the caller's TTY" + ) + assert call.kwargs.get("shell") is not True + assert call.kwargs["check"] is True + + @pytest.mark.usefixtures("no_sleep") class TestUnixInstall: """The Unix path downloads install.sh, then executes it without a shell.""" @@ -320,18 +311,6 @@ def test_curl_short_flags_present(self, flag: str) -> None: f"curl -{flag} missing from {curl_cmd!r}" ) - def test_stdin_is_devnull(self) -> None: - """install.sh runs `claude install`, which branches on `[ -t 0 ]`. The - old `curl | bash` gave it a pipe; it must never inherit a real TTY.""" - with _unix_install("1.2.3") as mock_run: - download_cli.download_cli() - - assert mock_run.call_args_list - for call in mock_run.call_args_list: - assert call.kwargs["stdin"] is subprocess.DEVNULL, ( - f"{call.args[0][0]} may inherit the caller's TTY" - ) - def test_latest_passes_no_version_argument(self) -> None: _, bash_cmd = _run_unix_download("latest") assert bash_cmd[0] == "bash" @@ -363,14 +342,6 @@ def test_version_is_its_own_argv_element(self, version: str) -> None: bash_cmd = commands[-1] assert bash_cmd == ["bash", bash_cmd[1], version] - def test_never_uses_shell_true(self) -> None: - with _unix_install("1.2.3") as mock_run: - download_cli.download_cli() - - for call in mock_run.call_args_list: - assert call.kwargs.get("shell") is not True - assert call.kwargs["check"] is True - @pytest.mark.parametrize( "body", [ @@ -451,45 +422,6 @@ def fake_run(command: list[str], **kwargs: object) -> MagicMock: assert "could not resolve" in err -SCRIPT_ENV_VAR = download_cli.INSTALL_SCRIPT_ENV_VAR - -PS_BODY = b"# install.ps1\nWrite-Host installing\n" - - -def _fake_irm(body: bytes = PS_BODY) -> _RunSideEffect: - """A subprocess.run side effect making the download step write ``body``. - - The Windows download step names its output file only in the environment, so - that is where the fake finds the path to write -- the same indirection the - real PowerShell command resolves. - """ - - def side_effect(command: list[str], **kwargs: Any) -> MagicMock: - env = kwargs.get("env") or {} - path = env.get(SCRIPT_ENV_VAR) - if path and "Invoke-RestMethod" in command[-1]: - Path(path).write_bytes(body) - return MagicMock() - - return side_effect - - -def _windows_install( - version: str, side_effect: _RunSideEffect | None = None -) -> AbstractContextManager[MagicMock]: - """Pin download_cli() to the Windows path, yielding the patched run.""" - return _install("Windows", version, side_effect or _fake_irm()) - - -def _run_windows_download(version: str, body: bytes = PS_BODY) -> tuple[Any, Any]: - """Run download_cli() on Windows, returning its (download, install) calls.""" - with _windows_install(version, _fake_irm(body)) as mock_run: - download_cli.download_cli() - - download_call, install_call = mock_run.call_args_list - return download_call, install_call - - @pytest.mark.usefixtures("no_sleep") class TestWindowsInstall: """The PowerShell branch downloads install.ps1, checks the body, then runs @@ -516,7 +448,7 @@ def test_downloads_then_executes_script(self) -> None: download_cmd = download_call.args[0] assert download_cmd[0] == "powershell" assert ( - "Invoke-RestMethod -Uri https://claude.ai/install.ps1" in (download_cmd[-1]) + "Invoke-RestMethod -Uri https://claude.ai/install.ps1" in download_cmd[-1] ) # The download writes install.ps1 into a temp dir; the install step runs @@ -536,44 +468,27 @@ def test_script_path_is_never_in_the_powershell_command_text(self) -> None: for arg in call.args[0]: assert script_path not in arg, f"script path interpolated into {arg!r}" - def test_valid_version_reaches_powershell_command(self) -> None: - _, install_call = _run_windows_download(DEV_VERSION) - cmd = install_call.args[0] - assert cmd[0] == "powershell" - assert cmd[-1].endswith(f"$env:{ENV_VAR}") - assert install_call.kwargs["stdin"] is subprocess.DEVNULL - @pytest.mark.parametrize("version", ["1.2.3", DEV_VERSION]) - def test_version_is_never_in_the_powershell_command_text( + def test_version_is_carried_in_the_environment_not_the_command_text( self, version: str ) -> None: """Regression guard: the version must reach PowerShell through the - environment, never spliced into the `-Command` string it parses.""" - for call in _run_windows_download(version): + environment, never spliced into the `-Command` string it parses. The + name in the command text and the name in the environment are the same + constant, so a rename cannot desynchronize them silently.""" + download_call, install_call = _run_windows_download(version) + + for call in (download_call, install_call): for arg in call.args[0]: assert version not in arg, f"version interpolated into {arg!r}" - @pytest.mark.parametrize("version", ["1.2.3", DEV_VERSION]) - def test_version_is_carried_in_the_environment(self, version: str) -> None: - _, install_call = _run_windows_download(version) - env = install_call.kwargs["env"] assert env is not None, "PowerShell must be given an explicit environment" assert env[ENV_VAR] == version + assert f"$env:{ENV_VAR}" in install_call.args[0][-1] # The child still needs PATH, SystemRoot, etc. to run at all. assert "CLAUDE_CLI_VERSION" in env - def test_command_references_the_env_var_it_sets(self) -> None: - """The name in the command text and the name in the environment are the - same constant, so a rename cannot desynchronize them silently.""" - _, install_call = _run_windows_download("1.2.3") - assert f"$env:{ENV_VAR}" in install_call.args[0][-1] - assert ENV_VAR in install_call.kwargs["env"] - - def test_stdin_is_devnull(self) -> None: - for call in _run_windows_download("1.2.3"): - assert call.kwargs["stdin"] is subprocess.DEVNULL - def test_latest_passes_no_version_argument(self) -> None: """`latest` invokes the installer with no argument at all -- not with an empty string -- and sets no version in the child's environment.""" @@ -632,9 +547,9 @@ def test_non_powershell_body_is_rejected_before_execution( # A UTF-8 BOM is legal, and common, in a real .ps1. b"\xef\xbb\xbf# install.ps1\nWrite-Host hi\n", # A block comment opens with '<' -- and about_Comment_Based_Help - # puts exactly such a help block at the top of a script, ahead of - # param(), which is what real installers do. The '<' check must not - # mistake it for an HTML error page. + # puts exactly such a help block at the top of a script, which is + # what real installers do. The '<' check must not mistake it for an + # HTML error page. b"<#\n.SYNOPSIS\nInstalls Claude Code.\n#>\nparam([string]$Version)\n", b"\xef\xbb\xbf\r\n<#\n.SYNOPSIS\nInstalls Claude Code.\n#>\n", ], @@ -646,27 +561,44 @@ def test_powershell_body_is_executed(self, body: bytes) -> None: assert install_call.args[0][-1].startswith(f"& $env:{SCRIPT_ENV_VAR}") +def _raise(error: Exception) -> _RunSideEffect: + def side_effect(command: list[str], **kwargs: object) -> MagicMock: + raise error + + return side_effect + + @pytest.mark.usefixtures("no_sleep") class TestMissingBinaryFailsFast: """A command that cannot be started at all is deterministic. - The Unix path exec's `curl` and `bash` directly, so a missing binary raises + The install commands are exec'd directly, so a missing binary raises FileNotFoundError rather than exiting 127 through a shell. That must be a one-line error and an exit 1 -- not a traceback, and not three attempts and a misleading "Error downloading CLI after 3 attempts". """ - def _missing(self, name: str) -> _RunSideEffect: - def side_effect(command: list[str], **kwargs: object) -> MagicMock: - raise FileNotFoundError(2, "No such file or directory", name) - - return side_effect - - def test_missing_curl_is_not_retried( - self, capsys: pytest.CaptureFixture[str] + @pytest.mark.parametrize( + ("installer", "error"), + [ + (_unix_install, FileNotFoundError(2, "No such file or directory", "curl")), + ( + _windows_install, + FileNotFoundError(2, "No such file or directory", "powershell"), + ), + # A noexec tmpdir, say, is just as deterministic as a missing binary. + (_unix_install, PermissionError(13, "Permission denied", "curl")), + ], + ids=["missing-curl", "missing-powershell", "permission-denied"], + ) + def test_first_command_failing_to_start_is_not_retried( + self, + installer: Callable[..., AbstractContextManager[MagicMock]], + error: Exception, + capsys: pytest.CaptureFixture[str], ) -> None: with ( - _unix_install("1.2.3", self._missing("curl")) as mock_run, + installer("1.2.3", _raise(error)) as mock_run, pytest.raises(SystemExit) as excinfo, ): download_cli.download_cli() @@ -675,12 +607,12 @@ def test_missing_curl_is_not_retried( assert len(mock_run.call_args_list) == 1 err = capsys.readouterr().err assert "could not run the install command" in err - assert "curl" in err assert "after 3 attempts" not in err def test_missing_bash_is_not_retried( self, capsys: pytest.CaptureFixture[str] ) -> None: + """The installer itself failing to start, after a successful download.""" curl = _fake_curl() def side_effect(command: list[str], **kwargs: object) -> MagicMock: @@ -698,81 +630,59 @@ def side_effect(command: list[str], **kwargs: object) -> MagicMock: assert [call.args[0][0] for call in mock_run.call_args_list] == ["curl", "bash"] assert "could not run the install command" in capsys.readouterr().err - def test_missing_powershell_is_not_retried( - self, capsys: pytest.CaptureFixture[str] - ) -> None: - with ( - _windows_install("1.2.3", self._missing("powershell")) as mock_run, - pytest.raises(SystemExit) as excinfo, - ): - download_cli.download_cli() - - assert excinfo.value.code == 1 - assert len(mock_run.call_args_list) == 1 - assert "could not run the install command" in capsys.readouterr().err - - def test_other_os_errors_are_not_retried_either( - self, capsys: pytest.CaptureFixture[str] - ) -> None: - """PermissionError (a noexec tmpdir, say) is just as deterministic.""" - - def side_effect(command: list[str], **kwargs: object) -> MagicMock: - raise PermissionError(13, "Permission denied", "curl") - - with ( - _unix_install("1.2.3", side_effect) as mock_run, - pytest.raises(SystemExit) as excinfo, - ): - download_cli.download_cli() - - assert excinfo.value.code == 1 - assert len(mock_run.call_args_list) == 1 - assert "could not run the install command" in capsys.readouterr().err - class TestCommandLine: """The script itself reports a bad version rather than raising through. build_wheel.py runs this file as `python scripts/download_cli.py`, so an - uncaught ValueError would surface as a traceback in a build log. Only the - rejected path is exercised here: it fails in get_cli_version() before any - curl or installer runs, so the test touches no network. + uncaught ValueError would surface as a traceback in a build log, and the + shared validation module has to import with scripts/ not a package. + + Only rejected versions are exercised: they fail in get_cli_version() before + any curl or installer runs, so no test here touches the network. PATH is + emptied as a second belt -- if validation ever regressed, the run would hit + a missing `curl` rather than really installing the CLI. """ - def _run(self, version: str) -> subprocess.CompletedProcess[str]: + def _run(self, tmp_path: Path, version: str) -> subprocess.CompletedProcess[str]: return subprocess.run( [sys.executable, str(SCRIPT_PATH)], - env={**os.environ, "CLAUDE_CLI_VERSION": version}, + env={**os.environ, "CLAUDE_CLI_VERSION": version, "PATH": str(tmp_path)}, capture_output=True, text=True, + timeout=60, ) - def test_invalid_version_exits_nonzero_without_a_traceback(self) -> None: - result = self._run("1.0.0; id") + def test_invalid_version_exits_nonzero_without_a_traceback( + self, tmp_path: Path + ) -> None: + result = self._run(tmp_path, "1.0.0; id") assert result.returncode == 1 assert "Invalid CLAUDE_CLI_VERSION" in result.stderr assert "Traceback" not in result.stderr + assert "ModuleNotFoundError" not in result.stderr - def test_error_names_the_offending_value(self) -> None: + def test_error_names_the_offending_value(self, tmp_path: Path) -> None: """Named in the one-line message, not merely in a traceback frame. Anchored to the start of a line: an uncaught raise renders the same text under `ValueError: `, which *contains* `Error: ` as a substring, so a plain `in` check would pass on the very traceback this guards - against. Scanning lines rather than stderr as a whole also keeps it - immune to any warning Python prints first. + against. """ - stderr = self._run("1.0.0; id").stderr + stderr = self._run(tmp_path, "1.0.0; id").stderr (error_line,) = [ line for line in stderr.splitlines() if line.startswith("Error: ") ] assert "1.0.0; id" in error_line - def test_nothing_is_installed_before_the_version_is_rejected(self) -> None: + def test_nothing_is_installed_before_the_version_is_rejected( + self, tmp_path: Path + ) -> None: """The banner prints, then it bails -- no download is announced.""" - result = self._run("--help") + result = self._run(tmp_path, "--help") assert result.returncode == 1 assert "Downloading Claude Code CLI version" not in result.stdout diff --git a/tests/test_update_cli_version.py b/tests/test_update_cli_version.py index b10224f0d..a5d0c2641 100644 --- a/tests/test_update_cli_version.py +++ b/tests/test_update_cli_version.py @@ -75,8 +75,7 @@ def test_round_trip(self, version_file: Path, version: str) -> None: def test_surrounding_whitespace_is_stripped_before_writing( self, version_file: Path, argument: str, written: str ) -> None: - """A version read out of a file arrives with a trailing newline. Strip - it -- and write the stripped value, not the raw one, so the newline + """The stripped value is written, so a trailing newline from a file read never lands inside the string literal.""" update_cli_version.update_cli_version(argument, version_file) assert import_version(version_file) == written @@ -155,16 +154,11 @@ def test_rejected_and_file_untouched( def test_dist_tags_are_rejected(self, version_file: Path, tag: str) -> None: """_cli_version.py must name one concrete build, never a moving tag. - build-and-publish.yml builds five wheels on five runners, each of which - independently runs build_wheel.py -> download_cli.py with this value. - A dist-tag would let the runners resolve different CLI builds into - wheels published under a single SDK version, and would leave - _cli_version.py -- the only record of what shipped -- naming nothing. - "stable" is the dangerous one: it passes the installer too, so before - this guard it silently pinned a moving tag and the build *succeeded*. - download_cli.py accepts the tags because CLAUDE_CLI_VERSION defaults to - "latest" for unpinned local builds; that is a different question from - what may be pinned into the file. + Five release runners each run build_wheel.py -> download_cli.py with + this value, so a dist-tag would let them resolve different CLI builds + into wheels published under one SDK version. "stable" is the dangerous + one: it passes the installer too, so before this guard it pinned a + moving tag and the build *succeeded*. """ with pytest.raises(ValueError, match="Invalid CLI version") as excinfo: update_cli_version.update_cli_version(tag, version_file)