Skip to content

Fix install script latest version parsing #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Sodawyx merged 4 commits into from
May 8, 2026
Merged

Fix install script latest version parsing #9

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b443254
fix install script latest version parsing
Sodawyx May 8, 2026
b377599
fix install script test shell compatibility
Sodawyx May 8, 2026
2a87530
fix integration test stderr assertions
Sodawyx May 8, 2026
ee685d9
harden install script regression test
Sodawyx May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ info "Detected target: $TARGET"
if [ -z "$VERSION" ]; then
info "Resolving latest release from github.com/${REPO}"
VERSION=$($DOWNLOAD "https://api.github.com/repos/${REPO}/releases/latest" \
| grep '"tag_name"' | head -1 | sed -E 's/.*"tag_name":\s*"([^"]+)".*/\1/')
| grep '"tag_name"' | head -1 | sed -E 's/.*"tag_name":[[:space:]]*"([^"]+)".*/\1/')

@Sodawyx Sodawyx May 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The fix itself is correct: [[:space:]] is POSIX and works on both GNU and BSD sed, while \s is a GNU extension that BSD/macOS sed does not interpret as whitespace. Diff is appropriately surgical — good.

One adjacent thought (not blocking): the parsing pipeline grep | head -1 | sed -E '...' is still a bit fragile — e.g., it would break if the assets section somewhere happened to contain a line whose substring matched "tag_name" (rare, but trivially possible if a release name itself contains that string). If you ever want to harden this further, a small python3 -c "import json,sys; print(json.load(sys.stdin)['tag_name'])" (gated on command -v python3) is one option. Out of scope for this PR.

[ -n "$VERSION" ] || err "could not resolve latest release tag"
fi
info "Version: $VERSION"
Expand Down
97 changes: 97 additions & 0 deletions tests/integration/test_install_script.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
"""Integration tests for the Unix installer script."""

import os
import stat
import subprocess
from pathlib import Path


def _write_executable(path: Path, content: str) -> None:
path.write_text(content)
path.chmod(path.stat().st_mode | stat.S_IXUSR)

@Sodawyx Sodawyx May 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nit: only S_IXUSR is set, not group/other.

If anything ever runs the test under a different uid (containerized CI with a mismatched user, sudo nuances), execution fails confusingly. Cheap fix: | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH, mirroring what chmod +x does. Not blocking, but trivial to harden.



def test_install_sh_parses_latest_release_tag_on_posix_sed(tmp_path):

@Sodawyx Sodawyx May 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The test name promises more than it delivers — it doesn't actually demonstrate the bug under POSIX sed.

The function is named test_install_sh_parses_latest_release_tag_on_posix_sed, but it just runs the script with the host's sed. On a Linux CI runner with GNU sed, \s already works, so this test passes against the old regex too — meaning it would not have caught the original bug, and it won't catch a regression of the same shape.

If you want this test to pin the portability fix, force POSIX behavior. Two options:

  1. Run sed with LC_ALL=POSIX plus --posix (GNU only) in a wrapper, or shadow sed via the same fake-bin trick used for curl/tar and have the mock invoke gsed --posix / busybox sed.
  2. Add a tiny unit test that calls sed -E 's/.*"tag_name":[[:space:]]*"([^"]+)".*/\1/' directly with LC_ALL=POSIX against a sample line, asserting it returns v0.1.0 — much smaller surface, and it'd actually fail before the fix.

Without that, this PR's biggest claim (the regression test) is mostly a smoke test for the happy path. Worth keeping the integration test, but please add something that actually locks down the portability invariant.

repo_root = Path(__file__).resolve().parents[2]
fake_bin = tmp_path / "bin"
fake_bin.mkdir()
install_dir = tmp_path / "install"

calls = tmp_path / "curl-calls.txt"
_write_executable(
fake_bin / "curl",
f"""#!/usr/bin/env sh
set -eu
url="${{@:$#}}"

@Sodawyx Sodawyx May 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Portability bug — bash-only syntax in a sh mock.

${@:$#} is a bash array-slice extension and is not POSIX. The shebang here is #!/usr/bin/env sh, so on most Linux distros (Debian/Ubuntu where sh → dash, Alpine where sh → busybox ash) this will be a syntax error and the test will fail with curl: 1: Bad substitution (or similar) — even though the very point of this PR is to harden a script that has to run under non-bash sh.

It happens to work on macOS only because /bin/sh is bash-in-POSIX-mode there, which still recognizes the bash slice syntax. Ironic for a regression test targeting BSD sed portability.

A POSIX-clean way to grab the last positional parameter:

for arg in "$@"; do url="$arg"; done

or

shift $(( $# - 1 ))
url="$1"

Same applies if/when other mocks need this idiom.

printf '%s\\n' "$url" >> "{calls}"
case "$url" in
*"/releases/latest")
printf '%s\\n' '{{'
printf '%s\\n' ' "tag_name": "v0.1.0",'
printf '%s\\n' '}}'
;;
*".sha256")
printf '%s\\n' 'expected-sha agentrun-0.1.0-darwin-arm64.tar.gz'
;;
*)
printf '%s\\n' 'fake archive'
;;
esac
""",
)
_write_executable(
fake_bin / "uname",
"""#!/usr/bin/env sh
case "${1:-}" in
-s) printf '%s\n' Darwin ;;
-m) printf '%s\n' arm64 ;;
*) printf '%s\n' Darwin ;;
esac
""",
)
_write_executable(
fake_bin / "tar",
"""#!/usr/bin/env sh
while [ "$#" -gt 0 ]; do
if [ "$1" = "-C" ]; then
shift
install_dir="$1"
break
fi
shift
done
printf '%s\n' '#!/usr/bin/env sh' > "$install_dir/agentrun"
chmod +x "$install_dir/agentrun"
""",
)
_write_executable(
fake_bin / "shasum",
"""#!/usr/bin/env sh
printf '%s\n' 'expected-sha agentrun-0.1.0-darwin-arm64.tar.gz'
""",
)

env = {
**os.environ,
"PATH": f"{fake_bin}:{os.environ['PATH']}",
"AGENTRUN_INSTALL": str(install_dir),
"AGENTRUN_REPO": "Serverless-Devs/agentrun-cli",
}

result = subprocess.run(
["sh", str(repo_root / "scripts" / "install.sh")],
env=env,
text=True,
stdout=subprocess.PIPE,

@Sodawyx Sodawyx May 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nit: os.environ['PATH'] will KeyError if PATH is unset.

Edge case in stripped-down sandboxes / minimal CI containers. os.environ.get("PATH", "") is safer and reads identically. Minor.

stderr=subprocess.PIPE,
check=False,
)

assert result.returncode == 0, result.stdout + result.stderr
assert "Version: v0.1.0" in result.stdout
assert "Downloading agentrun-0.1.0-darwin-arm64.tar.gz" in result.stdout
assert (install_dir / "agentrun").exists()
assert (
"https://github.com/Serverless-Devs/agentrun-cli/releases/download/"
"v0.1.0/agentrun-0.1.0-darwin-arm64.tar.gz"
) in calls.read_text()
Loading