Validate CLAUDE_CLI_VERSION and remove shell interpolation from the build scripts#1117
Conversation
…_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.
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.
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.
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/.
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.
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, a few other candidates were examined and ruled out: the json.dumps() literal round-trips through build_wheel.py's "([^"]+)" reader for every version the allowlist admits (no quotes/backslashes possible), and the value read back is re-validated in download_cli.get_cli_version() before reaching an installer; the Windows latest branch correctly passes env=None (inherit) with no version splice; and a claim that the "Re-exported" comment on download_cli.VERSION_PATTERN is inaccurate was checked and refuted — the tests do reference it.
Extended reasoning...
Both posted findings are diagnostic-quality nits in the retry failure path and local-tooling drift, not correctness or security issues in the hardening itself. The injection fixes (argv separation on Unix, env-var passing on Windows, json.dumps + callable re.subn for the pinned file) were checked against the consumer in build_wheel.py and hold; the extensive new tests exercise the real scripts by path. Since bugs were found, the inline comments carry the review — this note only records what else was examined so a later pass need not re-derive it.
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
|
Pushed a6a912a: tightened the version validation to the installer's own grammar, and stopped retrying the installer's deterministic rejections. The problem: false accepts, not false rejectionsThe allowlist ( and
That silently pins a moving dist-tag into the one file that records which CLI build went into the wheels — exactly what rejecting What changed
Checks
|
There was a problem hiding this comment.
Beyond the two inline nits, this run also examined and ruled out: the Windows install path executing the remote installer without the Unix path's content check (the irm fetch mechanism is pre-existing and unchanged by this PR, and an HTML body fails scriptblock::Create parsing rather than executing); an empty CLAUDE_CLI_VERSION now failing the build instead of silently defaulting (deliberate strictness — the error names the offending value); and is_argument_rejection()'s usage: substring misclassifying unrelated failures (worst case is a fast-fail that still prints the full installer output and exits 1). Also noting: a6a912a's errors='replace' decode resolves the second half of my earlier retry_install comment; the FileNotFoundError-from-missing-curl half still applies to the current code.
Extended reasoning...
Bugs were found this run (two nit-level inline comments: the build-wheel-check paths filter not covering the new scripts/_cli_version_validation.py, and Dockerfile.test retaining the curl|bash pattern fixed in test.yml), so this is the informational ruled-out note rather than an approval — the PR is a substantial, security-motivated change to build scripts and validation logic that warrants human review regardless. I verified the two new findings against the repo (the paths filter and Dockerfile.test line are as described), confirmed the three refuted candidates were genuinely examined, and confirmed the author's a6a912a push partially addressed my prior retry_install comment (strict decode fixed via _decode(errors='replace'); the CalledProcessError-only catch remains) while the CLAUDE.md/scripts/pre-push lint-scope drift from my other prior comment is still unaddressed at HEAD.
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
…hing 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
|
Follow-up commit 87cc5d6 addresses a fresh review pass on this branch: Deleted Dist-tags are now matched exactly (
Comment fix in Simplification pass: with the rejection check gone, Checks: ruff check + format clean, mypy clean, 1212 passed / 5 skipped (down from 1218 only by the removed tests that asserted on installer output the installers cannot produce; all metacharacter/injection, unanchored-pattern, and literal-replacement security coverage is intact). |
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
35ed7ed — review follow-upsAddresses 8 of the 9 review threads. All six findings were reproduced against
New tests: 26 (missing Not fixed here: the |
There was a problem hiding this comment.
Beyond the two inline nits, this run also examined and ruled out: (1) the range(1, 4) mention in retry_install()'s comment — it describes the rejected alternative design, not the live loop, so it is not stale; (2) test.yml's Windows irm | iex install steps — pre-existing CI-install code outside the wheel-build path, and irm raises a terminating error on HTTP failures under pwsh; (3) the (FileNotFoundError, OSError) catch in retry_install() swallowing file-read errors from the body checks — a read failure on a file curl just wrote is equally deterministic, and the message still names the actual exception.
Extended reasoning...
Two nit-level findings are posted inline (the <# comment-block false positive in check_powershell_install_script, and the stale .claude/settings.json lint scope). This note only records the candidates that were investigated and refuted this run, so a later review pass does not re-explore them from scratch. It is informational, not a correctness guarantee.
…all 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
54fd51c — review follow-ups + simplification passCloses the two open review nits (the
Simplification pass (no behavior change)
Deliberately kept, despite looking repetitive: the metacharacter / control-char / quote-breakout parametrizations, the unanchored-pattern guard, VerificationBeyond the suite, I drove the scripts end-to-end against stub
One thing worth flagging that is not changed here: Also adds Checks: ruff check + format clean, mypy clean, 1241 passed / 5 skipped (up from 1238 by the three new accepted-body cases). |
_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
Cleanup pass complete — all review threads closedWrapping up the review cycle on this PR. Every thread is now resolved (0 unresolved), and all 15 checks pass on Review threads (11 total, all resolved)Each finding was re-verified against the code at HEAD before being actioned — the bot self-grades and a couple of its notes had already been overtaken by earlier commits. Fixed in code:
Resolved without a code change here:
Simplification (no behavior change)
I looked hard at whether the two install paths themselves are duplicated, since both now do download → verify → execute. They aren't: one carries the version in argv and the other in the environment, with different verifiers, so a shared skeleton would need a multi-callback seam and read worse than the explicit pair. Same for the Security coverage was preserved throughout: the metacharacter/control-char/quote-breakout parametrization, the unanchored-pattern guard, 1241 passed / 5 skipped, ruff + mypy clean over |
There was a problem hiding this comment.
Beyond the two inline findings, one further candidate was examined and ruled out this run: whether a failing install.ps1 that ends in exit N would leave the build green because powershell -Command "& $env:SCRIPT" does not propagate the script's exit code. Reproduced with PowerShell 7.6: the exact code is lost, but any nonzero exit maps to process exit 1, which run_command()'s check=True catches — the build still fails. Recording it here so a later pass doesn't re-explore it from scratch.
Extended reasoning...
This run produced two inline findings (a pre-existing find_installed_cli() PATH-ordering gap and a SKILL.md documentation nit), which are posted as inline comments and need no restatement. Separately, a finder raised the candidate that the rewritten Windows install path might swallow installer failures because '& script.ps1' under -Command is said not to propagate exit codes; verifiers refuted it, and I confirmed the refutation empirically (pwsh 7.6.3: a script exiting nonzero yields process exit 1, success yields 0), so subprocess.run(check=True) still raises CalledProcessError on installer failure. Only the specific exit code is lost, not the failure itself. This note is informational only — it records what was examined, not a guarantee of the Windows path's correctness on Windows PowerShell 5.1 specifically.
…orms 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
|
Splitting this up. The security core has been extracted into #1125, which is off current #1125 (the security core) — the part that actually matters and shouldn't wait behind the rest:
What stays here — worth doing, but not urgent, and not worth holding the injection fix hostage:
Marking this a draft in the meantime. I'll rebase it onto |
There was a problem hiding this comment.
Beyond the inline nit, this pass also examined and ruled out three concerns in scripts/download_cli.py: a UTF-16-encoded HTML error body slipping past check_powershell_install_script() (it passes the '<' check, but PowerShell then fails to execute it, so check=True still fails the attempt loudly — the check is defense-in-depth, not the only gate); retry_install()'s OSError fast-fail catching a file-read error from the body-check step (the check reads a file the download command just wrote into the same tempdir, so that path is deterministic and effectively unreachable); and Windows install failures being silently swallowed under & $env:SCRIPT (a non-zero exit from the script propagates through powershell -Command, raising CalledProcessError that the retry loop reports).
Extended reasoning...
Bugs were found this run (one nit on the missing-pin-file traceback in update_cli_version.py, posted inline), so per policy I am not approving and not restating the finding. This note records the additional candidate issues that finder agents raised and verifiers refuted, so later review passes on this heavily-iterated PR have a record of what was already examined. It is informational only — not a guarantee of correctness and not an instruction to skip anything in future reviews. No prior run of mine left such a ruled-out note on this PR.
| if version_path is None: | ||
| version_path = DEFAULT_VERSION_PATH | ||
| content = version_path.read_text() |
There was a problem hiding this comment.
🟡 Running this script from any cwd other than the repo root (or with the pin file missing) dumps a raw FileNotFoundError traceback: version_path.read_text() has no missing-file handling and the __main__ block catches only ValueError — the exact output shape commit d4a6529 exists to eliminate. A two-line fix (check version_path.exists() and raise ValueError(f"{version_path} does not exist")) routes it through the existing handler, matching how build_wheel.get_bundled_cli_version() already handles its missing-file case.
Extended reasoning...
What the bug is. update_cli_version() calls version_path.read_text() at line 40 with no handling for a missing file, and the __main__ block at lines 67-71 catches only ValueError. Since DEFAULT_VERSION_PATH is the relative path Path("src/claude_agent_sdk/_cli_version.py"), running the script from any cwd other than the repo root — or against a genuinely missing/renamed pin file — lets FileNotFoundError propagate as a raw traceback.
The code path that triggers it. python scripts/update_cli_version.py 2.1.207 → argv validation passes (a concrete version) → version_path defaults to the relative DEFAULT_VERSION_PATH → read_text() raises FileNotFoundError → the except ValueError handler does not match (FileNotFoundError is an OSError, not a ValueError) → Python prints the full traceback and exits 1.
Step-by-step proof (reproduced verbatim by three independent verifiers at HEAD):
cd /tmp(orcd scripts/— a natural mistake for a script invoked ad hoc by whoever produces the "chore: bump bundled CLI version" commits).python3 <repo>/scripts/update_cli_version.py 2.1.207- Output: a raw multi-frame
Traceback (most recent call last): ... FileNotFoundError: [Errno 2] No such file or directory: 'src/claude_agent_sdk/_cli_version.py', exit 1.
Why existing code doesn't prevent it. This is the one asymmetric gap in a contract the PR itself established. Commit d4a6529's stated purpose is that these build-step entry points "turn a ValueError into one line on stderr and exit 1 rather than dumping a traceback out of a build step", and the PR applies it everywhere else: a bad version raises ValueError through the shared validator; the missing-__cli_version__-assignment case one line later (line 55-56) was deliberately converted from a silent no-op into a clean ValueError precisely so it flows through the same handler; and the sibling reader, build_wheel.get_bundled_cli_version(), handles the identical missing-file case explicitly via _fail_unpinned(f"{CLI_VERSION_FILE} does not exist") — one line + exit 1, pinned by test_missing_file_fails. Only the writer's missing-file case escapes the net. No test covers it either: every rejection test pre-creates the file (the version_file fixture writes ORIGINAL; TestCommandLine's cwd fixture always builds the full layout), and test_missing_assignment_leaves_file_untouched writes the file first.
Impact. Legibility only — the exit status is non-zero either way, so no build proceeds incorrectly. But the operator gets the exact traceback output shape this PR's d4a6529 commit exists to eliminate, in a maintainer-run release script, instead of a one-line diagnosis. While the old code also crashed on a missing file, this PR rewrote this exact function and added the clean-exit handler around it, so the gap is squarely in touched code.
How to fix. Two lines in the same style as the sibling reader: after resolving version_path, check if not version_path.exists(): raise ValueError(f"{version_path} does not exist") (or wrap the read_text() in try/except OSError and re-raise as ValueError), so it flows through the existing __main__ handler. Optionally add a CLI-level test that runs from a cwd without the file, mirroring test_missing_file_fails on the reader side.
|
Pre-merge verification summary This branch was verified end-to-end before merge:
Note: #1125 will be closed as superseded once this merges — its 5 files are byte-identical to this branch's. |
There was a problem hiding this comment.
Beyond the inline nit, this pass also examined whether Windows install failures signaled only via exit codes could be invisible to run_command()'s check=True (the powershell -Command invocation) — refuted: PowerShell propagates the command's failing exit status to the process exit code, so subprocess.run(check=True) raises CalledProcessError and the retry/report path engages as designed.
Extended reasoning...
Bugs were found this run (one nit on retry_install()'s OSError scope, posted inline), so no approval verdict is offered — the PR is also currently a draft pending the split into #1125. This note only records the one additional candidate that was examined and refuted this run, so a later pass on this PR or on #1125 (which carries the same download_cli.py code) does not re-explore it from scratch. It is informational, not a guarantee of correctness.
| 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. | ||
| print(f"Error: could not run the install command: {e}", file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🟡 The except OSError branch in retry_install() catches more than its own comment promises: the attempt() closure it wraps includes plan.check(), whose file I/O (Path.open("rb") / read_bytes()) also raises OSError subclasses, so a check-phase FileNotFoundError/PermissionError is misreported as "could not run the install command" and fast-failed with no retry. Catching OSError inside run_command() (the subprocess boundary) and re-raising a dedicated exec-failure exception would restore the handler's stated contract.
Extended reasoning...
What the bug is. download_cli() hands retry_install() a closure of three steps: run_command(*plan.download) → plan.check(plan.script_path) → run_command(*plan.install). The loop's first handler is a broad except OSError, whose own comment defines its contract: "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." But the middle step is not a command. check_install_script() does Path(script_path).open("rb"), check_powershell_install_script() does Path(script_path).read_bytes() (as does _reject_install_script()), and any OSError those raise — FileNotFoundError if the download exited 0 without writing the file, PermissionError if the file is unreadable — lands in the same handler, is printed as Error: could not run the install command: {e}, and hard-exits with no retry.\n\nStep-by-step proof (reproduced at HEAD with the real module). (1) Pin platform.system() to "Windows" and stub subprocess.run to return success without writing anything — modeling a download step that exits 0 without producing install.ps1. (2) Run download_cli() with CLAUDE_CLI_VERSION=1.2.3. (3) The download "succeeds"; check_powershell_install_script() calls read_bytes() on the missing file and raises FileNotFoundError. (4) Observed output: Error: could not run the install command: [Errno 2] No such file or directory: '/tmp/.../install.ps1', exit 1, exactly one subprocess.run call — one attempt, no retry, and a message blaming a command that ran fine.\n\nWhy existing safeguards miss it. The test suite drives the check phase only through _fake_curl/_fake_irm, which always write the file before the check runs, so the check functions never raise OSError in the suite; TestMissingBinaryFailsFast only exercises OSError raised by subprocess.run itself — the case the handler was designed for. mypy cannot distinguish OSError origins.\n\nOn the refutation. One verifier argued both triggers are implausible: Windows AV sharing violations typically hit deletes/renames/exclusive opens rather than plain reads, and Invoke-RestMethod failures are terminating errors that make powershell -Command exit 1, routing to the CalledProcessError retry branch instead. Those points are fair on trigger frequency, and they are why this is a nit rather than normal severity — on Unix, curl -f -o exiting 0 essentially guarantees the file exists, so the path is close to unreachable there. But the refutation itself concedes the code observation is accurate, and the finding does not depend on any one trigger being common: the handler's scope is structurally wider than its documented contract, so any check-phase OSError — present or introduced by a future check — gets a misattributing diagnostic and forfeits the retries retry_install() exists to provide. The refuter's fallback ("a download that deterministically reports success while writing nothing would reproduce on retry, so fail-fast is arguably right") is only true for the deterministic sub-case; a transient one (interrupted -OutFile write, rare AV lock) is exactly the retryable class.\n\nImpact. Bounded: every outcome is a loud exit-1 build failure with the true errno and path visible in the message, so nothing silently succeeds and no wrong artifact is bundled. The cost is a misattributed one-line diagnostic (the prefix names a command that was never run) plus a forfeited retry in rare transient cases on a five-platform release build.\n\nHow to fix. Move exec-failure detection to the subprocess boundary: catch OSError inside run_command() and re-raise a dedicated exception (e.g. class InstallCommandUnavailable(Exception)), and have retry_install() fast-fail on that instead of on OSError. Then a check-phase FileNotFoundError can surface via the existing _reject_install_script-style loud error (or be retried), and "could not run the install command" can only ever mean what it says. Alternatively, have the check functions convert their own file I/O errors into the reject path.
2 participants
The problem
scripts/download_cli.pytook the CLI version straight out of theCLAUDE_CLI_VERSIONenvironment variable and interpolated it into shell/PowerShell command text, with no validation:Anything in
CLAUDE_CLI_VERSIONbecame part of a command line thatbash -c(or PowerShell) parses —1.0.0; idor a backtick-quoted PowerShell payload runs as a command during a build.scripts/update_cli_version.pyhad the same shape of problem in a different medium: it wrote its argv into a Python string literal insrc/claude_agent_sdk/_cli_version.pyvia an f-string, so a version containing a double quote could close the literal and inject arbitrary Python into a module the package imports.Both scripts run as build steps, and the value flows between them (
update_cli_version.pywrites it,build_wheel.pyreads it back,download_cli.pyhands it to an installer), so both ends need to agree on what a version is.What the commits do
ae771e2ValidateCLAUDE_CLI_VERSIONand avoid shell interpolation indownload_cli.py— addsscripts/_cli_version_validation.pywith a single allowlist,[0-9A-Za-z][0-9A-Za-z.+-]*, matched withfullmatch(). It admits real versions including dev builds (2.1.146-dev.20260519.t105443.shaece3dab) and nothing with a quote, backslash, space, newline, or shell metacharacter; the leading-alphanumeric requirement also rejects flag-shaped values like--help. On top of that, the command construction stops interpolating at all: Unix now downloadsinstall.shto a temp file withcurl -oand runs["bash", script, version]— the version is its own argv element and never touched by a shell — and Windows passes the version in the environment (CLAUDE_CLI_INSTALL_VERSION) and references it as$env:...in argument position, so it is never part of the command text PowerShell parses.subprocess.runis never called withshell=True, and stdin isDEVNULL. The downloaded script's first two bytes are checked for a#!shebang before execution, because claude.ai answers unknown paths with HTTP 200 and an HTML body thatcurl -fwill not catch.118bf7cValidate the version argument toupdate_cli_version.py— runs the same shared validator on argv (withallow_latest=False, since a pinned file has to name one concrete build), before the file is touched, and writes the literal withjson.dumps()plus a callablere.subnreplacement so neither the string literal norre's backslash-escape processing can be subverted. A missing__cli_version__assignment is now an error instead of a silent no-op.cd91b25Fix two union-attr errors indownload_cli.pyretry loop — the old loop stashed the failure in aCalledProcessError | Noneand dereferenced it after the loop; reporting from inside the last-attempt handler removes the| Noneand the two mypy errors with it.6764d44Fail CI when the CLI install pipeline fails — theInstall Claude Codesteps in.github/workflows/test.ymlruncurl ... | bashunder the default shell, which has nopipefail, so the step takes its status frombashand stays green with the CLI uninstalled whencurlfails. Settingshell: bashgets-eo pipefail.7d23db2Lint and typecheckscripts/in CI —.github/workflows/lint.ymlnow runsruff check,ruff format --check, andmypyoverscripts/as well assrc//tests/, so this validation code cannot silently rot. Includes the small typing fixes inscripts/check_pypi_quota.pythat mypy surfaces, and apyproject.tomlmypy override fortwine(imported under a guardedtry/except ImportErrorinbuild_wheel.py, not a declared dependency).d4a6529Exit cleanly on an invalidCLAUDE_CLI_VERSION— the shared validator keeps raising (callers and tests read the exception), but the__main__entry points turn aValueErrorinto one line on stderr and exit 1 rather than dumping a traceback out of a build step.Testing
Two new test files, 717 lines total, both driving the real scripts (loaded by path, since
scripts/is not a package):tests/test_download_cli.py(438 lines) — accepted/rejected version tables; assertsVERSION_PATTERNadmits no shell metacharacter and is deliberately unanchored (so amatch()-for-fullmatch()swap fails loudly instead of accepting1.0.0\n); Unix install asserts the version is its own argv element, thatshell=Trueis never used, that stdin isDEVNULL, that a non-shebang body is rejected beforebashever runs, and that acurlfailure is no longer masked; Windows install asserts the version never appears in the PowerShell command text, is carried in the environment, and that an injected version is rejected before anything runs; CLI-level tests assert a bad version exits non-zero with no traceback and nothing installed.tests/test_update_cli_version.py(279 lines) — round-trips the written file by importing it back and comparing the exact string; asserts the file is left untouched on every rejection (includinglatestand a missing assignment); asserts a regex backreference in the version is not expanded into the file and that the replacement really is a callable; CLI-level tests assert non-zero exit with no write.Full suite locally:
1185 passed, 4 skipped.ruff check,ruff format --check, andmypyall clean oversrc/,tests/, andscripts/.