Fix install script latest version parsing by Sodawyx · Pull Request #9 · Serverless-Devs/agentrun-cli

Sodawyx · 2026-05-08T02:12:35Z

Summary

Fix latest release tag parsing in scripts/install.sh by replacing non-portable \s* with POSIX [[:space:]]*
Add an integration test that exercises the Unix installer with mocked tools and GitHub API output

Problem

On macOS/BSD sed, \s is not treated as a whitespace matcher. The installer therefore kept the full JSON line as the version, producing an invalid download URL such as:

https://github.com/Serverless-Devs/agentrun-cli/releases/download/  "tag_name": "v0.1.0",/agentrun-  "tag_name": "v0.1.0",-darwin-arm64.tar.gz

Tests

sh -n scripts/install.sh
.venv/bin/pytest tests/integration/test_install_script.py -v
.venv/bin/ruff check tests/integration/test_install_script.py

Signed-off-by: Sodawyx <sodawyx@126.com>

Copilot

Pull request overview

Fixes the Unix installer’s “latest release tag” extraction so it works on macOS/BSD sed, and adds an integration test that runs scripts/install.sh end-to-end using mocked curl/uname/tar/shasum.

Changes:

Update scripts/install.sh to use POSIX [[:space:]]* instead of non-portable \s* in the sed expression that extracts tag_name.
Add tests/integration/test_install_script.py to validate that the installer resolves v0.1.0 and constructs the expected download URL when run under sh with mocked tooling.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
scripts/install.sh	Makes latest GitHub release tag parsing portable across GNU/BSD `sed`.
tests/integration/test_install_script.py	Adds an integration test harness for `install.sh` with mocked external commands and API output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        fake_bin / "curl",
+        f"""#!/usr/bin/env sh
+set -eu
+url="${{@:$#}}"


Sodawyx

Thanks for the fix — the \s → [[:space:]] swap in install.sh is correct, minimal, and addresses a real macOS/BSD sed portability bug. Good problem statement in the PR description.

The integration test, however, has two issues that I think warrant changes before merge:

The mock curl uses bash-only syntax (${@:$#}) in a #!/usr/bin/env sh script. This will break on any Linux CI runner where sh is dash or busybox ash — i.e., almost all of them. The test would only happen to pass on macOS, which is an awkward outcome for a test guarding cross-platform installer behavior. See inline.
The test does not actually exercise the bug it's named after. On a Linux runner with GNU sed, \s works, so the test would pass against the old regex too. As written it's a happy-path smoke test, not a regression pin. A small direct-sed unit test with LC_ALL=POSIX (or shadowing sed in the fake-bin) would lock the portability invariant down properly. See inline.

Two minor nits inline as well (executable bits, PATH lookup). Nothing else stands out — DCO sign-off is present, the install-script change is in scope, no docs impact.

(Submitting as a comment review since GitHub doesn't allow request-changes on one's own PR.)

Sodawyx · 2026-05-08T02:16:34Z

+        fake_bin / "curl",
+        f"""#!/usr/bin/env sh
+set -eu
+url="${{@:$#}}"


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.

Sodawyx · 2026-05-08T02:16:59Z

+    path.chmod(path.stat().st_mode | stat.S_IXUSR)
+
+
+def test_install_sh_parses_latest_release_tag_on_posix_sed(tmp_path):


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:

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.

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.

Sodawyx · 2026-05-08T02:17:14Z

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


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.

Sodawyx · 2026-05-08T02:17:29Z

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


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.

Sodawyx · 2026-05-08T02:17:45Z

    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/')


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.

Signed-off-by: Sodawyx <sodawyx@126.com>

fix install script latest version parsing

b443254

Signed-off-by: Sodawyx <sodawyx@126.com>

Copilot AI review requested due to automatic review settings May 8, 2026 02:12

Copilot started reviewing on behalf of Sodawyx May 8, 2026 02:13 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

Comment thread tests/integration/test_install_script.py Outdated

fake_bin / "curl",

f"""#!/usr/bin/env sh

set -eu

url="${{@:$#}}"

Sodawyx commented May 8, 2026

View reviewed changes

Sodawyx added 3 commits May 8, 2026 10:20

fix install script test shell compatibility

b377599

Signed-off-by: Sodawyx <sodawyx@126.com>

fix integration test stderr assertions

2a87530

Signed-off-by: Sodawyx <sodawyx@126.com>

harden install script regression test

ee685d9

Signed-off-by: Sodawyx <sodawyx@126.com>

Sodawyx merged commit 09c0ec0 into main May 8, 2026
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix install script latest version parsing#9

Fix install script latest version parsing#9
Sodawyx merged 4 commits intomainfrom
fix/install-script-version-parse

Sodawyx commented May 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Sodawyx left a comment

Uh oh!

Sodawyx May 8, 2026

Uh oh!

Sodawyx May 8, 2026

Uh oh!

Sodawyx May 8, 2026

Uh oh!

Sodawyx May 8, 2026

Uh oh!

Sodawyx May 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		path.chmod(path.stat().st_mode \| stat.S_IXUSR)


		def test_install_sh_parses_latest_release_tag_on_posix_sed(tmp_path):

Conversation

Sodawyx commented May 8, 2026

Summary

Problem

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Sodawyx left a comment

Choose a reason for hiding this comment

Uh oh!

Sodawyx May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Sodawyx May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Sodawyx May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Sodawyx May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Sodawyx May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants