From 9083df9f5cf3bfe9924b13b1deb73b39f76fb23f Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:35:02 -0400 Subject: [PATCH] fix(pixlet): resolve the release tag correctly when downloading (#461) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(pixlet): resolve the release tag correctly when downloading Starlark apps render through the pixlet binary, and the installer that fetches it silently produced nothing, so every app failed with "Pixlet not available - Starlark apps will not work". Two compounding defects: The version lookup parsed the wrong token. GitHub returns the release JSON on a single line, so `grep '"tag_name"'` matches the whole document and the greedy `sed 's/.*"([^"]+)".*/\1/'` captures the LAST quoted string in it. That resolved to "mentions_count", giving a download URL for a release that does not exist. The `[ -z "$PIXLET_VERSION" ]` fallback never fired, because the value was not empty -- just wrong. And `curl -L -o` without `-f` writes a 404 body to the file and exits 0, so the download was reported as successful and the first sign of trouble was tar complaining "not in gzip format" about a page of HTML: → Downloading linux-arm64... Extracting... gzip: stdin: not in gzip format ✗ Failed to extract archive: .../pixlet_mentions_count_linux-arm64.tar.gz Download complete: 0/1 succeeded Now the tag field is matched directly and the value taken from it, and the result is checked for a version shape rather than merely being non-empty -- a wrong-but-non-empty value is exactly what made this silent. curl gets -f so an HTTP error is a failure, and the archive is gzip-tested before extraction, since a proxy can return 200 with an error page. Verified on an arm64 rig: v0.53.1 resolved, 1/1 downloaded, the binary runs, and the plugin's own detection finds it at bin/pixlet/pixlet-linux-arm64. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(pixlet): anchor the version check, and don't echo response bytes raw Both CodeRabbit findings were valid. The shape check accepted partial matches, so "v0.53garbage", "0.53" and "v0.5" passed it and built a download URL for a release that cannot exist -- the failure the check was added to stop, just one step later. Anchored at both ends now. Every tronbyt/pixlet release to date is vX.Y.Z (all 38 verified against the API), with an optional suffix left for a future -rc.1 or +build tag. The invalid-response diagnostic printed bytes straight from whatever answered the request. NUL and newline were filtered but escape, carriage return and backspace were not, so an error page could rewrite the output or bury it in a CI log. Non-printable bytes are stripped and it goes through printf. CodeRabbit suggested hex-encoding the lot; printable characters are kept instead, because "" is the diagnostic -- hex would make the line safe and useless. Also corrected the comment above the parse. It asserted GitHub returns this JSON on a single line; the API is pretty-printed by default, and I could not get a single-line response from two machines across five header variants. The single-line case is real (it is what produces "mentions_count", and the failing device's error named pixlet_mentions_count_linux-arm64.tar.gz), but it is a shape to be robust against, not a constant. As written the comment invites the next reader to check by hand, see pretty JSON, and conclude the fix was unnecessary. Tests drive the real script with a stubbed curl: the tag resolves from both response shapes, non-release values fall back, an HTTP error is reported as a download failure rather than surfacing later as a tar error, a non-archive body is rejected before extraction, and the diagnostic cannot carry control bytes. The stub honours -f the way real curl does -- without that, the HTTP-error test passed against the old script too, since both end at 0/1 and only the reporting layer differs. Mutation-checked: 10 of the 16 fail against the pre-fix script, and the 5 covering these two findings fail against this branch's previous state. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --------- Co-authored-by: Claude Opus 5 (1M context) --- scripts/download_pixlet.sh | 48 +++++++++- test/test_pixlet_download.py | 180 +++++++++++++++++++++++++++++++++++ 2 files changed, 223 insertions(+), 5 deletions(-) create mode 100644 test/test_pixlet_download.py diff --git a/scripts/download_pixlet.sh b/scripts/download_pixlet.sh index 53a38d22..80caeb74 100755 --- a/scripts/download_pixlet.sh +++ b/scripts/download_pixlet.sh @@ -24,9 +24,29 @@ echo "========================================" # Auto-detect latest version if needed if [ "$PIXLET_VERSION" = "latest" ]; then echo "Detecting latest version..." - PIXLET_VERSION=$(curl -s "https://api.github.com/repos/${REPO}/releases/latest" | grep '"tag_name"' | sed -E 's/.*"([^"]+)".*/\1/') - if [ -z "$PIXLET_VERSION" ]; then - echo "Failed to detect latest version, using fallback" + # When this response arrives on a single line -- as it did on the device + # where Starlark apps were failing -- `grep '"tag_name"'` matches the whole + # document and a greedy `sed 's/.*"([^"]+)".*/\1/'` captures the LAST + # quoted token in it rather than the tag. That resolved to + # "mentions_count", which built a download URL for a release that does not + # exist. (The API is pretty-printed by default, which is why the old + # command looks correct when you try it by hand -- but the formatting is + # not something to depend on.) Match the field itself and take the value + # after it, which is right for either shape. + PIXLET_VERSION=$(curl -fsSL "https://api.github.com/repos/${REPO}/releases/latest" \ + | grep -o '"tag_name"[[:space:]]*:[[:space:]]*"[^"]*"' \ + | head -n1 \ + | sed -E 's/.*:[[:space:]]*"([^"]*)".*/\1/') + + # A wrong-but-non-empty value is what made the old bug silent, so check the + # shape rather than just that something came back. Anchored at both ends: a + # partial match would accept "v0.53garbage" or "0.53" and build a URL for a + # release that cannot exist, which is the failure this check is here to + # stop. Every tronbyt/pixlet release to date is vX.Y.Z; the optional suffix + # leaves room for a future -rc.1 or +build tag. + if ! printf '%s' "$PIXLET_VERSION" \ + | grep -qE '^v[0-9]+\.[0-9]+\.[0-9]+([-+][0-9A-Za-z.-]+)?$'; then + echo "Could not detect the latest version (got: '${PIXLET_VERSION:-}'), using fallback" PIXLET_VERSION="v0.50.2" fi fi @@ -67,8 +87,26 @@ download_binary() { temp_dir=$(mktemp -d -p "$PROJECT_ROOT" -t pixlet_download.XXXXXXXXXX) local temp_file="$temp_dir/$archive_name" - if ! curl -L -o "$temp_file" "$url" 2>/dev/null; then - echo "✗ Failed to download $arch" + # -f so an HTTP error is a failure. Without it curl writes the 404 body + # to the file and exits 0, and the first sign of trouble is tar saying + # "not in gzip format" about what is actually a page of HTML. + if ! curl -fL -o "$temp_file" "$url" 2>/dev/null; then + echo "✗ Failed to download $arch from $url" + rm -rf "$temp_dir" + return 1 + fi + + # Belt and braces: a mirror or proxy can return 200 with an error page. + if ! gzip -t "$temp_file" 2>/dev/null; then + echo "✗ Downloaded file is not a gzip archive: $url" + # These bytes come from whatever answered the request, so strip + # everything non-printable before echoing them: an error page carrying + # terminal escapes would otherwise be able to rewrite this output or + # bury it in a CI log. Printable characters are kept rather than + # hex-encoding the lot, because "" is the diagnostic. + local first_bytes + first_bytes=$(head -c 60 "$temp_file" | tr -cd '[:print:]') + printf ' (first bytes: %s)\n' "$first_bytes" rm -rf "$temp_dir" return 1 fi diff --git a/test/test_pixlet_download.py b/test/test_pixlet_download.py new file mode 100644 index 00000000..e6fd4993 --- /dev/null +++ b/test/test_pixlet_download.py @@ -0,0 +1,180 @@ +""" +Tests for scripts/download_pixlet.sh -- release-tag resolution and download guards. + +Background: Starlark apps render through the pixlet binary, and the installer +that fetches it failed silently. It resolved the release tag by grepping the +GitHub API response for '"tag_name"' and taking the last quoted token on the +match with a greedy sed. When the response arrives on one line that token is +"mentions_count", not the tag, so the script built a URL for a release that +cannot exist -- and `curl -L -o` without -f wrote the 404 body to the file and +exited 0, so the first sign of trouble was tar reporting "not in gzip format" +about a page of HTML. + +The API is pretty-printed by default, which is exactly why this needs a test: +by hand the old command looks correct, and the failure only appears when the +formatting changes. These drive the real script with a stubbed curl on PATH, so +both response shapes are covered without touching the network. +""" + +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parent.parent / "scripts" / "download_pixlet.sh" + +PRETTY = """{ + "url": "https://api.github.com/repos/tronbyt/pixlet/releases/12345", + "id": 12345, + "tag_name": "v0.53.1", + "name": "v0.53.1", + "draft": false, + "prerelease": false, + "mentions_count": 3 +} +""" + +# The shape that broke it: one line, and the last quoted token is not the tag. +MINIFIED = ( + '{"url":"https://api.github.com/repos/tronbyt/pixlet/releases/12345",' + '"id":12345,"tag_name":"v0.53.1","name":"v0.53.1","draft":false,' + '"prerelease":false,"mentions_count":3}' +) + + +def run_script(tmp_path, api_body, download=None): + """Run the real script against a stubbed curl. + + Args: + api_body: what the stub returns for the api.github.com request. + download: bytes to write for a release-asset request, or None to make + that request fail the way `curl -f` does on an HTTP error. + """ + root = tmp_path / "project" + (root / "scripts").mkdir(parents=True) + shutil.copy(SCRIPT, root / "scripts" / "download_pixlet.sh") + + api_file = tmp_path / "api.json" + api_file.write_text(api_body) + + stub_dir = tmp_path / "stub" + stub_dir.mkdir() + asset_file = tmp_path / "asset.bin" + if download is not None: + asset_file.write_bytes(download) + + # Stands in for curl, including the -f semantics the fix turns on: without + # -f, real curl writes the error body to the output file and exits 0, which + # is what let a 404 masquerade as a successful download. The stub has to + # honour that or a test of the fix would pass against the old script too. + (stub_dir / "curl").write_text(f"""#!/bin/bash +out="" +url="" +fail_on_error=0 +while [ $# -gt 0 ]; do + case "$1" in + -o) out="$2"; shift 2 ;; + -*f*) fail_on_error=1; shift ;; + -*) shift ;; + *) url="$1"; shift ;; + esac +done +if [[ "$url" == *api.github.com* ]]; then + cat {api_file} + exit 0 +fi +if [ -f "{asset_file}" ]; then + cp "{asset_file}" "$out" + exit 0 +fi +# No asset: stand in for an HTTP 404. +if [ "$fail_on_error" = "1" ]; then + exit 22 +fi +printf '404 Not Found' > "$out" +exit 0 +""") + (stub_dir / "curl").chmod(0o755) + + return subprocess.run( + ["bash", str(root / "scripts" / "download_pixlet.sh")], + capture_output=True, text=True, + env={"PATH": f"{stub_dir}:/usr/bin:/bin:/usr/sbin:/sbin", + "PIXLET_VERSION": "latest"}, + ) + + +def resolved_version(result): + match = re.search(r"^Version: (.+)$", result.stdout, re.M) + assert match, f"no version line in output:\n{result.stdout}" + return match.group(1).strip() + + +def test_script_is_syntactically_valid(): + result = subprocess.run(["bash", "-n", str(SCRIPT)], capture_output=True, text=True) + assert result.returncode == 0, result.stderr + + +@pytest.mark.parametrize("body,label", [(PRETTY, "pretty"), (MINIFIED, "minified")]) +def test_tag_is_resolved_from_either_response_shape(tmp_path, body, label): + """The minified case is the regression: the last quoted token there is + "mentions_count", which is what the old greedy sed captured.""" + result = run_script(tmp_path, body) + assert resolved_version(result) == "v0.53.1", f"{label}: {result.stdout}" + assert "mentions_count" not in result.stdout + + +@pytest.mark.parametrize( + "tag", + ["mentions_count", "v0.53garbage", "0.53", "v0.5", "", "v0.53.1 ; echo pwned"], +) +def test_a_tag_that_is_not_a_release_falls_back(tmp_path, tag): + """A wrong-but-non-empty value is what made the original bug silent, so the + check is on the shape. Partial matches must not pass: "v0.53garbage" and + "0.53" would build a URL for a release that cannot exist.""" + result = run_script(tmp_path, '{"tag_name": "%s"}' % tag) + assert resolved_version(result) == "v0.50.2", result.stdout + assert "using fallback" in result.stdout + + +@pytest.mark.parametrize("tag", ["v0.53.1", "v1.0.0", "v0.54.0-rc.1", "v1.2.3+build.4"]) +def test_real_release_tag_shapes_are_accepted(tmp_path, tag): + assert resolved_version(run_script(tmp_path, '{"tag_name": "%s"}' % tag)) == tag + + +def test_an_http_error_is_reported_as_a_failed_download(tmp_path): + """Without curl -f the 404 body lands in the file and curl exits 0, so the + failure surfaced two steps later as tar complaining about gzip -- about + what was really a page of HTML. It has to be reported where it happened. + + Both versions end at 0/1, so asserting only on the count would pass against + the old script; the discriminating part is which layer reports it. + """ + result = run_script(tmp_path, PRETTY, download=None) + assert "Download complete: 0/1 succeeded" in result.stdout + assert "✓ Downloaded" not in result.stdout + assert "Failed to download" in result.stdout + assert "Failed to extract" not in result.stdout, ( + "an HTTP error should not surface as an extraction failure") + + +def test_a_non_archive_response_is_rejected_before_extraction(tmp_path): + result = run_script(tmp_path, PRETTY, download=b"502 Bad Gateway") + assert "not a gzip archive" in result.stdout + assert "Download complete: 0/1 succeeded" in result.stdout + + +def test_the_diagnostic_cannot_smuggle_terminal_escapes(tmp_path): + """Those bytes come from whatever answered the request. An error page + carrying escapes must not be able to rewrite the output or bury it.""" + hostile = b"\x1b[2J\x1b[31mgone\x1b[0m\rHTTP 200 OK\x08\x08" + result = run_script(tmp_path, PRETTY, download=hostile) + assert "not a gzip archive" in result.stdout + printed = re.search(r"^\s*\(first bytes: (.*)\)$", result.stdout, re.M) + assert printed, f"no diagnostic line:\n{result.stdout}" + assert "DOCTYPE" in printed.group(1), "the useful part of the page was dropped" + for forbidden in ("\x1b", "\r", "\x08", "\x00"): + assert forbidden not in printed.group(1), ( + f"control byte {forbidden!r} reached the terminal")