From 2887f8accf3960fe243e9cda1c6bd3a5952d9e24 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Mon, 5 Oct 2026 11:43:54 -0400 Subject: [PATCH 1/4] fix: Address post-merge review findings on the Linux release smoke test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1286. - Drop the checkout's `ref:` pin to the triggering release's own commit. This script didn't exist yet at v0.8.0/v0.8.1 (confirmed via the GitHub API: 404 for scripts/release_smoke_test_linux.sh at both commits), so re-running an old release's workflow failed before the test even started. The checkout is only for the test script itself; the release assets and install.sh both come over the network regardless of what's checked out. - Replace the whole OLDER_TAG lookup. `--limit 20` capped the candidate pool before filtering, so once 20+ stable releases exist, re-testing an old one could find zero candidates "older than TAG" and silently skip the upgrade leg while still reporting success. Date-based "older" also breaks across maintenance branches (release-0.6/release-0.7 both exist) — a patch tagged later on an older line could resolve to something with a HIGHER version number, an unlabeled downgrade. main-latest now just takes the single newest stable release (no date comparison needed); a real v* tag compares by version number (sort -V across up to 200 releases, not a 20-release date-ordered cap). Verified against the live release list and a simulated maintenance-branch case. - assert_healthy now checks agentop service status's exit code directly instead of piping through tee, which lost it under dash — a missing agentop (exit 127) used to read as "did not report healthy" instead of "command not found". - Uninstall now also checks `systemctl --user is-active` directly. The three existing checks (printed "Removed", unit file gone, config unchanged) can all pass even if the proxy never actually stopped — removeService only logs a failed `disable --now`, then deletes the unit file and prints success regardless. - install_cortex now fetches the installer from a SHA-pinned URL when installing main-latest, not just raw.githubusercontent.com/main/... (cached up to 5 minutes). A commit that changes the installer and the binaries together (as the rename PR did) had a narrow window to be tested with a mismatched pairing. - Dropped detect_cli entirely (decided in conversation): matches this repo's own clean-break policy for the abctl rename more closely than a one-off compatibility fallback for a single already-superseded release. - Verified, before touching anything: a separate PR (the agentop setup rearchitecture) landed mid-review. Confirmed by reading the new cmd/agentop/cmd_setup.go and setup_step_*.go directly that the on-disk layout, agentop service uninstall, the SHA-256 stamp check, and the enable+restart split are all unchanged in substance — none of this script's assertions needed to change because of it. - Corrected two of this file's own comments that overclaimed (one said a cost "stops being reachable... once a newer release exists" when it's actually permanently reachable; another misdescribed which error message a pre-rename re-test produces), and fixed capitalization drift ("Release Binaries" vs the workflow's real name) in prose that didn't get the trigger's own earlier fix. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .github/workflows/release-smoke-linux.yaml | 21 ++- .../2026-09-29-linux-release-smoke-test.md | 79 ++++++++- scripts/release_smoke_test_linux.sh | 167 ++++++++++++------ 3 files changed, 204 insertions(+), 63 deletions(-) diff --git a/.github/workflows/release-smoke-linux.yaml b/.github/workflows/release-smoke-linux.yaml index be3528ba7..8da299f5a 100644 --- a/.github/workflows/release-smoke-linux.yaml +++ b/.github/workflows/release-smoke-linux.yaml @@ -4,7 +4,7 @@ # downloading from GitHub Releases — there is no "install from a local build" # mode), then exercises the systemd install/upgrade/uninstall path for real. # -# Triggers on Release Binaries finishing, not on the tag/main push directly: +# Triggers on Release binaries finishing, not on the tag/main push directly: # that guarantees the release assets this pulls from already exist, rather # than racing the upload. # @@ -34,7 +34,7 @@ permissions: jobs: linux-smoke-test: name: Linux install/upgrade/uninstall smoke test - # event == 'push' excludes Release Binaries' workflow_dispatch dry runs, + # event == 'push' excludes Release binaries' workflow_dispatch dry runs, # which build but never publish — nothing would exist yet to install. if: >- github.event.workflow_run.conclusion == 'success' && @@ -42,9 +42,18 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 steps: + # No `ref:` pin to the triggering release's own commit: this checkout is + # only for the TEST SCRIPT itself, not the things under test (the + # release assets and install.sh both come over the network regardless + # of what's checked out here). Pinning to the release's commit broke + # re-running an old release's workflow outright — this script didn't + # exist yet at v0.8.0/v0.8.1 (confirmed via the GitHub API: 404 for + # scripts/release_smoke_test_linux.sh at both commits), so the checkout + # would succeed and the very next step would fail to find the script to + # run. Always running main's current copy means re-runs always reach + # the actual test logic, whichever release they're re-testing. - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event.workflow_run.head_sha }} persist-credentials: false # A tag push's workflow_run.head_branch is the tag itself; a push to main @@ -83,8 +92,10 @@ jobs: sleep 1 done echo "XDG_RUNTIME_DIR=${rt}" >>"${GITHUB_ENV}" - XDG_RUNTIME_DIR="${rt}" systemctl --user show-environment || \ - echo "::warning::systemd --user session not reachable after enable-linger; the smoke test will report why" + XDG_RUNTIME_DIR="${rt}" systemctl --user show-environment || { + echo "::error::systemd --user session not reachable after enable-linger" + exit 1 + } - name: Run the Linux release smoke test env: diff --git a/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md b/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md index cd1edd62a..c7c34d15b 100644 --- a/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md +++ b/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md @@ -19,12 +19,13 @@ Reuses the exact `enable-linger` + wait-for-`/run/user//bus` recipe The actual test logic lives in `scripts/release_smoke_test_linux.sh`, kept out of the YAML for the same reason `deploy/proxy-init/test-enforce-redirect.sh` -is a separate script rather than inline `run:` steps. It always pipes -`curl | sh` rather than invoking the checked-out `install.sh` as a local -file — `install.sh`'s own re-exec bootstrap only re-fetches the tagged copy -of itself (matching the requested `--ref`) when `$0` isn't a readable file, -which is the exact condition a real `curl | sh` user hits and a local -invocation would skip. +is a separate script rather than inline `run:` steps. It downloads +`install.sh` to a temp file and feeds that in on stdin, rather than invoking +the checked-out copy as a local file argument — `install.sh`'s own re-exec +bootstrap only re-fetches the tagged copy of itself (matching the requested +`--ref`) when `$0` isn't a readable file, which is the condition a real +`curl | sh` user hits (and a local file invocation would skip) just as much +as a literal shell pipe does. **Spec:** none — #957 is its own checklist, not a design doc. @@ -131,6 +132,72 @@ invocation would skip. happens: `release-binaries.yaml` PATCHes `main-latest`'s own tag ref to the triggering commit on every push to main, and `createdAt` follows the commit its tag points at. Corrected the comment accordingly. +- [x] Fix (follow-up, decided in conversation): dropped `detect_cli` entirely + — matches this repo's own clean-break policy for the abctl rename (no + alias, no compatibility code) more closely than keeping a one-off + fallback for a single already-superseded release. Re-testing `v0.8.0` + itself now fails plainly rather than falling back to the pre-rename + CLI; accepted, since that case only affects one historical release and + the test's "no older release" first-run path isn't affected. +- [x] Fix (review, round 2 — a separate PR landed `agentop setup` + mid-review): `install.sh` was rearchitected (#1203's follow-up) to hand + installation off to a new `agentop setup` subcommand instead of doing + it directly. Verified before touching anything further: the on-disk + layout, `agentop service uninstall`, the SHA-256 "already current" + stamp check, and the `enable`+`restart` split from #1203's own fix are + all unchanged in substance, just invoked one layer up through + `setup`'s step pipeline — none of this script's assertions needed to + change because of it, confirmed by reading the new + `cmd/agentop/cmd_setup.go`/`setup_step_*.go` directly rather than + assuming. +- [x] Fix (review, round 2): the workflow's checkout pinned + `ref: ${{ github.event.workflow_run.head_sha }}` — the release's own + commit. This script didn't exist yet at `v0.8.0`/`v0.8.1` (confirmed + via the GitHub API: 404 for `scripts/release_smoke_test_linux.sh` at + both commits), so re-running an old release's workflow failed before + the test even started. Dropped the `ref:` pin — the checkout is only + for the test script itself; the release assets and `install.sh` both + come over the network regardless of what's checked out. +- [x] Fix (review, round 2): `--limit 20` on the release lookup capped the + candidate pool BEFORE filtering, and date-based "older than TAG" broke + across maintenance branches (`release-0.6`/`release-0.7` both exist; a + patch tagged later on an older line could resolve to something with a + *higher* version number). Replaced the whole lookup: `main-latest` + just takes the single newest stable release (`--limit 1`, no date + comparison needed); a real `v*` tag now compares by VERSION NUMBER + (`sort -V` across up to 200 stable releases, not a 20-release + date-ordered cap) rather than by creation date. Verified against the + live release list and a simulated maintenance-branch case + (`v0.7.1` tagged after `v0.8.1`) that it resolves to `v0.7.0`, not + `v0.8.1`. +- [x] Fix (review, round 2): `assert_healthy` still piped + `agentop service status | tee`, losing the command's own exit status + under dash — a missing `agentop` (exit 127) would read as "did not + report healthy" rather than "command not found". Checked directly now, + same pattern already used for `install_cortex`. +- [x] Fix (review, round 2): uninstall's three checks (printed "Removed", + unit file gone, config unchanged) can all pass even if the proxy never + actually stopped — `removeService` only logs a failed + `systemctl --user disable --now`, then deletes the unit file and + prints success regardless. Added a direct + `systemctl --user is-active --quiet cortex.service` check. +- [x] Fix (review, round 2): a `main-latest` run pinned the expected + *version string* to the triggering commit, but not the *installer + script* used to install it — that was always fetched from + `raw.githubusercontent.com/.../main/...`, cached up to 5 minutes. A + commit that changes the installer and the binaries together (as the + rename PR did) has a narrow window to be tested with a mismatched + pairing. `install_cortex` now fetches the installer from a SHA-pinned + URL whenever installing `main-latest`. +- [x] Fix (review, round 2): a couple of this file's own comments overclaimed + — one said a cost "stops being reachable... once a newer stable + release exists" when `OLDER_TAG` is actually anchored to "before TAG" + permanently; another said a pre-rename re-test fails with + "command not found" when it actually surfaced as a misleading + "did not report healthy" (now fixed to fail as a direct, checked exit + status instead). Corrected both. Also fixed capitalization drift + ("Release Binaries" vs the workflow's real name, "Release binaries") + in prose comments that didn't get the trigger's own earlier fix. ## Result diff --git a/scripts/release_smoke_test_linux.sh b/scripts/release_smoke_test_linux.sh index 8e5e3924e..c05275189 100644 --- a/scripts/release_smoke_test_linux.sh +++ b/scripts/release_smoke_test_linux.sh @@ -58,7 +58,20 @@ trap 'rm -rf "${TMP_DIR}"' EXIT # the bare ones under `set -e` and the one under `if !`) handles correctly. install_cortex() { tmp="$(mktemp "${TMP_DIR}/install.XXXXXX")" - if ! curl -fsSL -o "${tmp}" https://raw.githubusercontent.com/rossoctl/cortex/main/scripts/install.sh; then + installer_url="https://raw.githubusercontent.com/rossoctl/cortex/main/scripts/install.sh" + if [ "$1" = "main-latest" ]; then + # Pin the installer to the exact commit that triggered this run, + # instead of "whatever main currently looks like": raw.githubusercontent.com + # caches responses for a few minutes, and a commit that changes + # install.sh and the binaries together (as the rename PR did) has a + # narrow window to be tested with a mismatched pairing — new + # binaries served with stale installer logic, or vice versa. A + # commit SHA in the URL is immutable content; the branch name "main" + # is not. By the time TAG is "main-latest", an earlier check already + # guarantees TRIGGER_SHA is non-empty. + installer_url="https://raw.githubusercontent.com/rossoctl/cortex/${TRIGGER_SHA}/scripts/install.sh" + fi + if ! curl -fsSL -o "${tmp}" "${installer_url}"; then echo "FAIL: could not download install.sh" >&2 return 1 fi @@ -81,12 +94,24 @@ assert_contains() { fi } -# Both wordings: the fresh install below runs the PREVIOUS release's agentop, which -# says "healthy: ", where this one says "Cortex is healthy according to ". -# GNU grep's \| alternation; this runs on ubuntu. +# Checked directly, not piped through tee: a pipeline's exit status under +# dash (no pipefail) is tee's, not agentop's — a missing agentop binary +# (exit 127) would still let the pipeline "succeed" with an empty ${out}, +# so the real cause (command not found) gets reported as the unrelated +# "did not report healthy" instead. +# +# Both wordings in the needle: the fresh install below runs the PREVIOUS +# release's agentop, which says "healthy: ", where this one says +# "Cortex is healthy according to ". GNU grep's \| alternation; this +# runs on ubuntu. assert_healthy() { out="$(mktemp "${TMP_DIR}/status.XXXXXX")" - "${AGENTOP}" service status | tee "${out}" + if ! "${AGENTOP}" service status >"${out}" 2>&1; then + cat "${out}" >&2 + echo "FAIL: agentop service status exited non-zero" >&2 + exit 1 + fi + cat "${out}" assert_contains "${out}" 'healthy:\|Cortex is healthy according to' "agentop service status did not report healthy" } @@ -149,58 +174,86 @@ case "${TAG}" in ;; esac -# The most recent STABLE release created strictly BEFORE the tag under test — -# the realistic "what a user who hasn't upgraded in a while" starting point. -# Not hardcoded: a fixed "known good" version would drift out of the release -# list over time and stop being the second-most-recent release, silently -# testing a narrower jump than intended. -# -# "Before TAG", not just "the most recent other release": this workflow can be -# triggered by re-running an OLD completed Release Binaries run, at which -# point newer stable releases may already exist. Picking "the most recent -# other release" in that case would select something NEWER than TAG, silently -# turning the "upgrade" step into an unlabeled downgrade — and for a release -# old enough (pre-rename artifact names, no install.sh at any path it -# probes), the fresh install of that "older" release would fail outright. -# -# createdAt, not publishedAt: release-binaries.yaml PATCHes main-latest's own -# tag ref to the triggering commit on every push to main, and a release's -# createdAt follows the commit its tag points at — the lightweight -# main-latest tag's commit date for that one, an annotated v* tag's tagger -# date for the rest. publishedAt stays frozen at whenever the release object -# was first created and does not move with it, confirmed live across two -# different days (main-latest's createdAt advanced from 2026-09-30T13:43:16Z -# to 2026-10-01T15:37:57Z as main kept moving; publishedAt stayed at -# 2026-09-09T21:11:29Z throughout). Anchoring on publishedAt would permanently -# exclude every stable release published after that first day from ever being -# picked for a main-latest run. +# The most recent STABLE release OLDER than the tag under test — the +# realistic "what a user who hasn't upgraded in a while" starting point. Not +# hardcoded: a fixed "known good" version would drift out of the release list +# over time and stop being the second-most-recent release, silently testing +# a narrower jump than intended. # -# set -eu alone won't catch a failure inside a pipeline under dash (no -# pipefail), so each gh call below is checked explicitly rather than trusted -# to abort the script on its own. -if ! tag_created_at="$(gh release view "${TAG}" --json createdAt -q '.createdAt')"; then - echo "FAIL: could not resolve the creation date for release ${TAG}" >&2 - exit 1 -fi - -if ! OLDER_TAG="$(gh release list --exclude-drafts --exclude-pre-releases --limit 20 \ - --json tagName,createdAt \ - -q "[.[] | select(.createdAt < \"${tag_created_at}\")] | sort_by(.createdAt) | last | .tagName // \"\"")"; then - echo "FAIL: gh release list failed" >&2 - exit 1 -fi +# main-latest and a real v* tag need different notions of "older", so they're +# handled separately rather than through one date-based comparison (the +# previous approach, and why: see below). +case "${TAG}" in + main-latest) + # main-latest always means "whatever's on main right now", so the + # single most recent stable release is unambiguously the one before + # it — no version-number comparison needed. gh release list already + # returns results newest-first, so --limit 1 is exact, not a cap that + # could hide an earlier candidate. + if ! OLDER_TAG="$(gh release list --exclude-drafts --exclude-pre-releases \ + --limit 1 --json tagName -q '.[0].tagName // ""')"; then + echo "FAIL: gh release list failed" >&2 + exit 1 + fi + ;; + *) + # A real v* tag: compare by VERSION NUMBER, not by date. This repo + # keeps maintenance branches alive (release-0.6, release-0.7 both + # exist), so a patch tagged on an older line can be CREATED later in + # calendar time than a newer line's release — e.g. v0.7.1 tagged + # after v0.8.1 already exists. Date-based "older" would then pick + # v0.8.1 as the "older" release for v0.7.1, an unlabeled downgrade. + # Comparing version numbers directly sidesteps that: v0.7.1 sorts + # after v0.7.0 and before v0.8.0 regardless of when either was + # actually published. + # + # --limit 200, not the previous --limit 20: that cap applied BEFORE + # any filtering, so once 20+ stable releases existed, re-testing an + # old one could find zero candidates "older than TAG" even though + # earlier releases existed, silently skipping the upgrade leg while + # still reporting success. gh release list has no --paginate flag; + # 200 is comfortably beyond any realistic release count for a long + # while, without writing real pagination for a problem this project + # will not hit for years. + # + # set -eu alone won't catch a failure inside a pipeline under dash + # (no pipefail), so the gh call is checked explicitly. + if ! all_stable="$(gh release list --exclude-drafts --exclude-pre-releases \ + --limit 200 --json tagName -q '.[].tagName')"; then + echo "FAIL: gh release list failed" >&2 + exit 1 + fi + # sort -V orders the full line "vX.Y.Z" the same way it would order + # bare dotted numbers — the "v" prefix is common to every line, so it + # never affects relative order. The release immediately before TAG in + # that order is the one with the highest version strictly less than + # it; if TAG is itself the lowest version present (or isn't in the + # list — e.g. a prerelease not yet promoted to a real release), + # `prev` is still unset and OLDER_TAG correctly comes out empty. + OLDER_TAG="$(printf '%s\n' "${all_stable}" | sort -V | awk -v want="${TAG}" ' + $0 == want { print prev; exit } + { prev = $0 } + ')" + ;; +esac log "Testing ${TAG} (upgrading from: ${OLDER_TAG:-none found; first release})" # No compatibility path for a pre-rename INSTALL_TAG (e.g. re-testing v0.8.0 -# itself, whose own "older" release resolves to the pre-rename v0.7.0): -# matches this repo's own clean-break policy for the abctl->agentop rename -# ("no alias, no compatibility code" — see -# docs/superpowers/specs/2026-09-30-abctl-to-agentop-rename-design.md). If -# INSTALL_TAG predates the rename, this fails here with a plain -# "command not found" rather than a clearer message — accepted, since it only -# affects re-testing that one already-superseded release, a cost that stops -# being reachable at all once a newer stable release exists to upgrade from. +# itself, whose own "older" release resolves to the pre-rename v0.7.0): matches +# this repo's own clean-break policy for the abctl->agentop rename — "There is +# no abctl alias" and "No compatibility code for on-disk state either" (see +# docs/superpowers/specs/2026-09-30-abctl-to-agentop-rename-design.md). +# +# Genuinely reachable, not a historical curiosity: re-testing v0.8.0 resolves +# to v0.7.0 as its "older" release permanently (OLDER_TAG is anchored to +# "before TAG", not to how many newer releases exist since), and this is the +# intended, accepted failure for that one case — install.sh's own re-exec +# bootstrap means the checkout step above no longer blocks it either. The +# failure itself surfaces at assert_healthy below as "agentop service status +# exited non-zero", not as a bare "command not found": AGENTOP is a fixed +# path, so a missing binary there is still a real, if unglamorous, exit +# status assert_healthy now checks directly. INSTALL_TAG="${OLDER_TAG:-${TAG}}" log "Fresh install: ${INSTALL_TAG}" install_cortex "${INSTALL_TAG}" @@ -261,5 +314,15 @@ if [ "${cfg_before}" != "${cfg_after}" ]; then echo "FAIL: uninstall changed the contents of ${CFG}; it promises to leave it untouched" >&2 exit 1 fi +# None of the checks above actually prove the proxy stopped. removeService +# (cmd_service.go) only LOGS a failed `systemctl --user disable --now`, then +# deletes the unit file regardless and still prints "Removed" — so all three +# checks above would pass even if the old proxy kept serving, now with no +# unit file left to manage or diagnose it. is-active asks systemd directly, +# rather than inferring from the unit file's absence or a printed message. +if systemctl --user is-active --quiet cortex.service; then + echo "FAIL: cortex.service is still active after uninstall" >&2 + exit 1 +fi log "Linux release smoke test passed for ${TAG}" From 7734369fb08980cbfa029e10cb5f8bdf01b2a1e0 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Mon, 5 Oct 2026 11:52:46 -0400 Subject: [PATCH 2/4] fix: Check the running binary on every leg, not just the upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A post-#1288-push review pass on #1172 found one more real gap: assert_running_version_is only ran inside the upgrade block, so a run with no OLDER_TAG (first release ever, or re-testing a tag whose own "older" release doesn't exist) never checked it had installed the right thing at all. Its old comment also claimed independence from assert_running_binary_is_current that doesn't hold — it runs the binary sitting on disk, not the service's live process, so on its own it would pass even in the #1203 stale-process bug this script exists to catch. Call both checks after the fresh-install leg too (using OLDER_TAG itself as the expected version there, since it's always a real tag, never main-latest), and reword both functions' comments to spell out the dependency: assert_running_binary_is_current must run first, every time, or assert_running_version_is is checking the wrong thing. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../2026-09-29-linux-release-smoke-test.md | 14 ++++++++++ scripts/release_smoke_test_linux.sh | 26 ++++++++++++++----- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md b/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md index c7c34d15b..c72257d2c 100644 --- a/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md +++ b/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md @@ -198,6 +198,20 @@ as a literal shell pipe does. status instead). Corrected both. Also fixed capitalization drift ("Release Binaries" vs the workflow's real name, "Release binaries") in prose comments that didn't get the trigger's own earlier fix. +- [x] Fix (review, round 3): `assert_running_version_is` only ran inside the + upgrade block, so a run with no `OLDER_TAG` (the very first release, or + a `v*` tag whose own "older" release doesn't exist) never checked it had + installed the right thing at all. It also wasn't independent of + `assert_running_binary_is_current` the way its old comment implied — it + runs the binary sitting on disk, not the service's process, so on its + own it would pass even in the #1203 stale-process state (the disk + binary IS the new one there; only the running process is stale). Fixed + by calling both checks right after the fresh-install leg too (using + `OLDER_TAG` itself as the expected version there, since it's always a + real tag, never `main-latest`), and reworded both functions' comments + to document the dependency explicitly: call `assert_running_binary_is_current` + first, every time, or `assert_running_version_is` is checking the wrong + thing. ## Result diff --git a/scripts/release_smoke_test_linux.sh b/scripts/release_smoke_test_linux.sh index c05275189..5db3ee642 100644 --- a/scripts/release_smoke_test_linux.sh +++ b/scripts/release_smoke_test_linux.sh @@ -142,13 +142,20 @@ assert_running_binary_is_current() { fi } -# Confirms the running binary's own reported version matches what this run -# installed — a second, independent signal alongside the inode check above, -# and the one that also ties a main-latest run to the exact commit that -# triggered it (see TRIGGER_SHA below): a later merge can re-point main-latest -# and clobber its assets while this job is still running, so without this a -# pass could be describing the NEXT build rather than the one actually under -# test. +# NOT independent of the check above — this runs the binary sitting at +# PROXY_BIN on disk, not the service's own process, so on its own it would +# pass even in the exact #1203 stale-process bug this script exists to catch +# (the disk binary IS the new one in that state; only the running process +# is stale). It only says something about the SERVICE because the preceding +# assert_running_binary_is_current call already proved the service's +# /proc//exe points at this same PROXY_BIN path. Call both, in this +# order, or this one is checking the wrong thing. +# +# What it adds beyond that: the exe-path check only proves "the service runs +# the file at this path"; this proves that file's own self-reported version +# is the one this run installed — which also ties a main-latest run to the +# exact commit that triggered it (see TRIGGER_SHA below), since a later merge +# can re-point main-latest and clobber its assets while this job still runs. assert_running_version_is() { got="$("${PROXY_BIN}" --version)" if [ "${got}" != "cortex $1" ]; then @@ -258,6 +265,11 @@ INSTALL_TAG="${OLDER_TAG:-${TAG}}" log "Fresh install: ${INSTALL_TAG}" install_cortex "${INSTALL_TAG}" assert_healthy +assert_running_binary_is_current +# INSTALL_TAG is OLDER_TAG when it's set — always a real v* tag, never +# main-latest, so its own version string IS the tag; expected_version (which +# resolves main-latest's main- form) only applies when INSTALL_TAG == TAG. +assert_running_version_is "${OLDER_TAG:-${expected_version}}" if [ -n "${OLDER_TAG}" ]; then # A marker only this test writes, to prove config survives the upgrade From 5716877ee03576ec94dbaf851bc4976d480a3b8c Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 6 Oct 2026 14:02:50 -0400 Subject: [PATCH 3/4] fix: Restore the checkout pin and fix prerelease upgrade skipping A second review round on #1288 found real regressions in the round-2 fixes, verified empirically against the live release list and this repo's actual tag history: - The version-sort OLDER_TAG lookup silently skipped the upgrade leg for every prerelease tag: --exclude-pre-releases drops TAG itself from the candidate list whenever TAG is an -rc/-alpha, so the lookup never found it and OLDER_TAG came out empty. This repo tags prereleases routinely (v0.7.0 alone shipped 10 alphas and 3 RCs), so this was the common case, not an edge case. Fixed by injecting TAG into the sorted list before the lookup. That fix had its own bug: plain sort -V orders a GA after its own RCs, so once a GA ships, its own RC's "older" release would resolve to the GA itself. Fixed by mapping the tag's first '-' to '~' before sorting, verified against the live list and simulated pre/post-GA cases. - Merged main-latest's lookup into the same version-sorted fetch (sort -V | tail -1) instead of a separate date-ordered --limit 1 call, which could pick the wrong "newest" release across a maintenance branch for the same reason the v* lookup needed sort -V in the first place. - Restored the checkout's ref: pin, reversing the previous round's fix. Dropping it didn't make either motivating re-run case pass, and it introduced a live hazard instead: a separately merged PR changed agentop's health-check wording on main while this PR was open, which is exactly the mismatch an unpinned script risks against every older release going forward, not just the ones it was meant to help re-test. - Resolved the resulting conflict with that merged PR (#1292) in assert_healthy: kept this PR's direct exit-status check and added back main's wider healthy-wording needle, since a fresh install always runs the PREVIOUS release's agentop first. - Corrected a factual claim about gh release list's --limit semantics (it filters client-side before counting toward the limit, not after) repeated in both the script's comments and the plan doc. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .github/workflows/release-smoke-linux.yaml | 29 +++-- .../2026-09-29-linux-release-smoke-test.md | 57 ++++++++ scripts/release_smoke_test_linux.sh | 122 ++++++++++-------- 3 files changed, 142 insertions(+), 66 deletions(-) diff --git a/.github/workflows/release-smoke-linux.yaml b/.github/workflows/release-smoke-linux.yaml index 8da299f5a..033c2a99d 100644 --- a/.github/workflows/release-smoke-linux.yaml +++ b/.github/workflows/release-smoke-linux.yaml @@ -42,18 +42,27 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 steps: - # No `ref:` pin to the triggering release's own commit: this checkout is - # only for the TEST SCRIPT itself, not the things under test (the - # release assets and install.sh both come over the network regardless - # of what's checked out here). Pinning to the release's commit broke - # re-running an old release's workflow outright — this script didn't - # exist yet at v0.8.0/v0.8.1 (confirmed via the GitHub API: 404 for - # scripts/release_smoke_test_linux.sh at both commits), so the checkout - # would succeed and the very next step would fail to find the script to - # run. Always running main's current copy means re-runs always reach - # the actual test logic, whichever release they're re-testing. + # Pinned to the release's own commit: the test SCRIPT has to match the + # vintage of the binaries it's testing, same reason install_cortex pins + # the installer URL to TRIGGER_SHA for a main-latest run below. Running + # main's current copy against an older release risks exactly the + # mismatch this workflow hit live once already — a since-fixed PR + # changed agentop's health-check wording, and a run of this script + # from AFTER that change against a release from BEFORE it would assert + # on output that release never produces. + # + # This does mean a release older than this script itself can't be + # smoke-tested by re-running its workflow — e.g. v0.8.0/v0.8.1 predate + # it entirely (confirmed via the GitHub API: 404 for + # scripts/release_smoke_test_linux.sh at both commits). That's accepted: + # those releases already shipped, re-running checks out the commit + # that was main's tip when the ORIGINAL run fired (not today's main + # either way), and the alternative — always running main's bleeding + # edge — breaks the common case (every release going forward) to + # partially help the uncommon one (re-testing something ancient). - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + ref: ${{ github.event.workflow_run.head_sha }} persist-credentials: false # A tag push's workflow_run.head_branch is the tag itself; a push to main diff --git a/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md b/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md index c72257d2c..970c06dfd 100644 --- a/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md +++ b/docs/superpowers/plans/2026-09-29-linux-release-smoke-test.md @@ -212,6 +212,63 @@ as a literal shell pipe does. to document the dependency explicitly: call `assert_running_binary_is_current` first, every time, or `assert_running_version_is` is checking the wrong thing. +- [x] Fix (review, round 4): the round-2 version-sort replacement for the + `OLDER_TAG` lookup introduced a regression of its own — + `--exclude-pre-releases` drops `TAG` itself from the candidate list + whenever `TAG` is a prerelease (`-rc`/`-alpha`), so `$0 == want` never + matched and `OLDER_TAG` silently came out empty for the common case + this repo actually tags with (v0.7.0 alone shipped 10 alphas and 3 + RCs) — the exact "upgrade leg silently skipped, run stays green" bug + class the `--limit 200` fix was meant to close. Fixed by injecting + `TAG` into the sorted list before the lookup instead of only searching + for it. That fix had its own bug, caught before landing: plain + `sort -V` orders a GA release AFTER its own RCs (`v0.9.0-rc.2` sorts + after `v0.9.0`), so once a GA ships, looking up its own RC resolves to + the GA — an unlabeled downgrade in the opposite direction from the one + `--limit 200` fixed. Fixed by mapping the tag's first `-` to `~` before + sorting (GNU `sort -V` orders `~` before end-of-string, so + `v0.9.0~rc.2` correctly sorts before `v0.9.0`); verified against the + live release list plus simulated pre-GA/post-GA scenarios for both + `v0.9.0-rc.2` and `v0.8.2-rc.1`. Also merged `main-latest`'s lookup into + the same fetch (`sort -V | tail -1` instead of a separate `--limit 1` + date-ordered call) — `gh release list` orders by creation date, and a + maintenance-branch patch tagged after a newer line's release would + have made `--limit 1` pick the wrong "newest" there too, the same + date-vs-version problem the `*)` branch exists to fix. +- [x] Fix (review, round 4): restored the checkout's `ref:` pin to the + triggering commit, reversing the round-2 fix. The round-2 reasoning + didn't hold up: unpinning doesn't make either motivating re-run case + pass (an old release re-run still fails, just later and less clearly — + at the no-op leg's string match instead of at checkout), a GitHub + Actions re-run reuses the original event payload rather than + re-resolving `main`'s current tip, and running main's bleeding-edge + script against an older release's binary is an active hazard, not just + a missed opportunity — it already happened once, live, between this + PR's own reviews (see the `assert_healthy` conflict below). Pinning + means a release old enough to predate this script can't be + smoke-tested by re-running its workflow; accepted, since those + releases already shipped and the alternative breaks the common case + (every release going forward) to partially help the rare one. +- [x] Fix (review, round 4): a separate, already-merged PR (#1292) changed + `agentop service status`'s healthy-wording from `healthy: ` to + `Cortex is healthy according to ` and landed directly on main + while this PR was still open, conflicting with this branch's own + `assert_healthy` rewrite. Resolved by keeping this PR's exit-status + check (main's side still piped through `tee`, losing the real failure + mode this PR exists to fix) together with main's wider needle + (`'healthy:\|Cortex is healthy according to'`) and its comment — a + fresh install always runs the PREVIOUS release's `agentop` first, so + the needle has to match whichever wording that older binary uses, not + just the newest one. +- [x] Fix (review, round 4): corrected a factual claim repeated in both this + doc and the script's own comments — "`--limit 20` capped the candidate + pool BEFORE filtering" described `gh release list`'s behavior + backwards. `gh`'s `fetchReleases` drops drafts and prereleases + client-side and keeps paging until it collects `--limit` *matches*, so + `--limit 20` always meant 20 stable releases, with the jq/`-q` date + filter running after that, not before. The actual bug was narrower + than the original comment claimed: 20 stable releases just isn't + enough reach once this repo has shipped more than that. ## Result diff --git a/scripts/release_smoke_test_linux.sh b/scripts/release_smoke_test_linux.sh index 5db3ee642..f1af1731e 100644 --- a/scripts/release_smoke_test_linux.sh +++ b/scripts/release_smoke_test_linux.sh @@ -187,57 +187,71 @@ esac # over time and stop being the second-most-recent release, silently testing # a narrower jump than intended. # -# main-latest and a real v* tag need different notions of "older", so they're -# handled separately rather than through one date-based comparison (the -# previous approach, and why: see below). +# Compared by VERSION NUMBER, not by date. This repo keeps maintenance +# branches alive (release-0.6, release-0.7 both exist), so a patch tagged on +# an older line can be CREATED later in calendar time than a newer line's +# release — e.g. v0.7.1 tagged after v0.8.1 already exists. Date-based +# "older" would then pick v0.8.1 as the "older" release for v0.7.1, an +# unlabeled downgrade. Comparing version numbers directly sidesteps that: +# v0.7.1 sorts after v0.7.0 and before v0.8.0 regardless of when either was +# actually published. +# +# --limit 200: gh release list's --limit counts STABLE releases, not raw API +# results — it drops drafts and prereleases on the client and keeps paging +# until it collects that many (cli/cli's fetchReleases), so this is "the 200 +# most recent stable releases," comfortably beyond any realistic release +# count for a long while. gh has no --paginate flag for this subcommand, so a +# fixed high limit stands in for real pagination on a problem this project +# will not hit for years. +# +# set -eu alone won't catch a failure inside a pipeline under dash (no +# pipefail), so the gh call is checked explicitly. +if ! all_stable="$(gh release list --exclude-drafts --exclude-pre-releases \ + --limit 200 --json tagName -q '.[].tagName')"; then + echo "FAIL: gh release list failed" >&2 + exit 1 +fi + case "${TAG}" in main-latest) # main-latest always means "whatever's on main right now", so the # single most recent stable release is unambiguously the one before - # it — no version-number comparison needed. gh release list already - # returns results newest-first, so --limit 1 is exact, not a cap that - # could hide an earlier candidate. - if ! OLDER_TAG="$(gh release list --exclude-drafts --exclude-pre-releases \ - --limit 1 --json tagName -q '.[0].tagName // ""')"; then - echo "FAIL: gh release list failed" >&2 - exit 1 - fi + # it. sort -V | tail -1 picks the highest VERSION, not whichever + # release gh happened to create most recently — gh release list + # orders by creation date, and a maintenance-branch patch can be + # created after a newer line's release (the same v0.7.1-after-v0.8.1 + # case above), which would make a date-ordered "newest" pick wrong + # here too. + OLDER_TAG="$(printf '%s\n' "${all_stable}" | sort -V | tail -n 1)" ;; *) - # A real v* tag: compare by VERSION NUMBER, not by date. This repo - # keeps maintenance branches alive (release-0.6, release-0.7 both - # exist), so a patch tagged on an older line can be CREATED later in - # calendar time than a newer line's release — e.g. v0.7.1 tagged - # after v0.8.1 already exists. Date-based "older" would then pick - # v0.8.1 as the "older" release for v0.7.1, an unlabeled downgrade. - # Comparing version numbers directly sidesteps that: v0.7.1 sorts - # after v0.7.0 and before v0.8.0 regardless of when either was - # actually published. - # - # --limit 200, not the previous --limit 20: that cap applied BEFORE - # any filtering, so once 20+ stable releases existed, re-testing an - # old one could find zero candidates "older than TAG" even though - # earlier releases existed, silently skipping the upgrade leg while - # still reporting success. gh release list has no --paginate flag; - # 200 is comfortably beyond any realistic release count for a long - # while, without writing real pagination for a problem this project - # will not hit for years. - # - # set -eu alone won't catch a failure inside a pipeline under dash - # (no pipefail), so the gh call is checked explicitly. - if ! all_stable="$(gh release list --exclude-drafts --exclude-pre-releases \ - --limit 200 --json tagName -q '.[].tagName')"; then - echo "FAIL: gh release list failed" >&2 - exit 1 - fi # sort -V orders the full line "vX.Y.Z" the same way it would order # bare dotted numbers — the "v" prefix is common to every line, so it # never affects relative order. The release immediately before TAG in # that order is the one with the highest version strictly less than - # it; if TAG is itself the lowest version present (or isn't in the - # list — e.g. a prerelease not yet promoted to a real release), - # `prev` is still unset and OLDER_TAG correctly comes out empty. - OLDER_TAG="$(printf '%s\n' "${all_stable}" | sort -V | awk -v want="${TAG}" ' + # it. + # + # TAG itself is injected into the sorted list (not just looked up in + # it) because --exclude-pre-releases already dropped it if TAG is a + # prerelease (an -rc/-alpha tag) — without this, $0 == want never + # matches for any prerelease TAG, OLDER_TAG always comes out empty, + # and the upgrade leg silently never runs for the common case this + # repo actually tags with (v0.7.0 alone shipped 10 alphas and 3 RCs). + # `sort -V -u` drops the duplicate if TAG also happens to be a real + # stable release already in the list; either way TAG can never match + # its own `prev`, since the exact-match branch exits before `prev` is + # updated to TAG's own line. + # + # The first `-` is mapped to `~` before sorting because plain sort -V + # orders an RC AFTER its own GA (v0.9.0-rc.2 sorts after v0.9.0), so + # looking up v0.9.0-rc.2 unmapped would resolve to v0.9.0 once v0.9.0 + # ships — an unlabeled downgrade in the other direction from the one + # above. GNU sort -V orders `~` before end-of-string, so the mapped + # `v0.9.0~rc.2` correctly sorts before `v0.9.0`; every stable tag + # today is a plain `vX.Y.Z` with no `-`, so the substitution is a + # no-op for them. + want="$(printf '%s' "${TAG}" | sed 's/-/~/')" + OLDER_TAG="$( { printf '%s\n' "${all_stable}"; printf '%s\n' "${want}"; } | sort -V -u | awk -v want="${want}" ' $0 == want { print prev; exit } { prev = $0 } ')" @@ -246,21 +260,17 @@ esac log "Testing ${TAG} (upgrading from: ${OLDER_TAG:-none found; first release})" -# No compatibility path for a pre-rename INSTALL_TAG (e.g. re-testing v0.8.0 -# itself, whose own "older" release resolves to the pre-rename v0.7.0): matches -# this repo's own clean-break policy for the abctl->agentop rename — "There is -# no abctl alias" and "No compatibility code for on-disk state either" (see -# docs/superpowers/specs/2026-09-30-abctl-to-agentop-rename-design.md). -# -# Genuinely reachable, not a historical curiosity: re-testing v0.8.0 resolves -# to v0.7.0 as its "older" release permanently (OLDER_TAG is anchored to -# "before TAG", not to how many newer releases exist since), and this is the -# intended, accepted failure for that one case — install.sh's own re-exec -# bootstrap means the checkout step above no longer blocks it either. The -# failure itself surfaces at assert_healthy below as "agentop service status -# exited non-zero", not as a bare "command not found": AGENTOP is a fixed -# path, so a missing binary there is still a real, if unglamorous, exit -# status assert_healthy now checks directly. +# No compatibility path for a pre-rename INSTALL_TAG (e.g. v0.8.0's own +# "older" release resolving to the pre-rename v0.7.0): matches this repo's +# own clean-break policy for the abctl->agentop rename — "There is no abctl +# alias" and "No compatibility code for on-disk state either" (see +# docs/superpowers/specs/2026-09-30-abctl-to-agentop-rename-design.md). Not +# actually reachable in practice: the workflow's checkout is pinned to TAG's +# own commit, and TAG=v0.8.0 is itself old enough to predate this script +# (confirmed via the GitHub API: 404 for this file at that commit) — a run +# testing v0.8.0 fails at "sh: can't open scripts/release_smoke_test_linux.sh" +# before any of this executes. Kept rather than deleted: a future release old +# enough to have this script but still pre-rename would reach here for real. INSTALL_TAG="${OLDER_TAG:-${TAG}}" log "Fresh install: ${INSTALL_TAG}" install_cortex "${INSTALL_TAG}" From 5bdc3d0c9f60b98f9b2732bcdaa02930e137873e Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 6 Oct 2026 15:16:25 -0400 Subject: [PATCH 4/4] docs: Capture the design reasoning behind the OLDER_TAG lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This logic has drawn repeated "why not X instead" questions (date ordering, commit history, "any recent release" instead of the immediate predecessor, whether the ~ substitution is just a fragile hack) that were answered in review discussion but never written down next to the code. Future readers asking the same questions shouldn't need to dig through PR history to find the answers: - Why the IMMEDIATE predecessor, not just any older release: it's the one diff most likely to contain a regression THIS release introduced. - Why version number and not ANY timestamp (release date, commit date, or walking commit ancestry) — the problem is categorical (maintenance branches don't correlate with chronology at all), not about which clock or graph you read. - Why --exclude-pre-releases still matters even though TAG gets injected into the sorted list — the injection is only a landmark for the lookup, not an admission ticket; a prerelease must never be eligible as the answer itself. - Why the ~ substitution isn't an ad hoc trick: it's dpkg's own prerelease-ordering convention, which GNU sort -V deliberately replicates, reused here for exactly what it was designed for. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- scripts/release_smoke_test_linux.sh | 49 ++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/scripts/release_smoke_test_linux.sh b/scripts/release_smoke_test_linux.sh index f1af1731e..b280b9cf8 100644 --- a/scripts/release_smoke_test_linux.sh +++ b/scripts/release_smoke_test_linux.sh @@ -181,20 +181,33 @@ case "${TAG}" in ;; esac -# The most recent STABLE release OLDER than the tag under test — the -# realistic "what a user who hasn't upgraded in a while" starting point. Not +# The most recent STABLE release OLDER than the tag under test — specifically +# the IMMEDIATELY preceding one, not just any older release. That's the N-1 -> +# N transition every tag goes through, and the one most likely to contain a +# regression THIS release just introduced: N-2 -> N-1 was already exercised +# as a smoke test when N-1 itself shipped, so testing a wider jump re-covers +# already-checked ground instead of the actual diff under test. Not # hardcoded: a fixed "known good" version would drift out of the release list # over time and stop being the second-most-recent release, silently testing # a narrower jump than intended. # -# Compared by VERSION NUMBER, not by date. This repo keeps maintenance +# Compared by VERSION NUMBER, not by date — and not by any other timestamp +# either (a tag's own commit date has the identical problem below; this is +# categorical, not about which clock you pick). This repo keeps maintenance # branches alive (release-0.6, release-0.7 both exist), so a patch tagged on # an older line can be CREATED later in calendar time than a newer line's -# release — e.g. v0.7.1 tagged after v0.8.1 already exists. Date-based +# release — e.g. v0.7.1 tagged after v0.8.1 already exists. Any date-based # "older" would then pick v0.8.1 as the "older" release for v0.7.1, an -# unlabeled downgrade. Comparing version numbers directly sidesteps that: -# v0.7.1 sorts after v0.7.0 and before v0.8.0 regardless of when either was -# actually published. +# unlabeled downgrade: no timestamp, however it's obtained, carries the +# "which product line does this belong to" information that only the +# dotted version number preserves. Walking git ancestry instead of release +# metadata doesn't fix this either — a maintenance branch commit is often not +# even an ancestor of the newer line's commit in the DAG, so ancestry would +# answer "what's upstream of this commit" rather than "what's the previous +# version in this product line," a different (and here, wrong) question. +# Comparing version numbers directly sidesteps all of this: v0.7.1 sorts +# after v0.7.0 and before v0.8.0 regardless of when or where either was +# actually created. # # --limit 200: gh release list's --limit counts STABLE releases, not raw API # results — it drops drafts and prereleases on the client and keeps paging @@ -237,10 +250,15 @@ case "${TAG}" in # matches for any prerelease TAG, OLDER_TAG always comes out empty, # and the upgrade leg silently never runs for the common case this # repo actually tags with (v0.7.0 alone shipped 10 alphas and 3 RCs). - # `sort -V -u` drops the duplicate if TAG also happens to be a real - # stable release already in the list; either way TAG can never match - # its own `prev`, since the exact-match branch exits before `prev` is - # updated to TAG's own line. + # `--exclude-pre-releases` still has to stay on for fetching the POOL, + # though — this injection only gives the lookup a landmark to match + # against, it does not make prereleases eligible ANSWERS. We never + # want OLDER_TAG itself to be a prerelease (nobody's real upgrade + # baseline is an RC), and the algorithm can't accidentally pick TAG + # as its own answer either way: the exact-match branch exits before + # `prev` is updated to TAG's own line, and `sort -V -u` drops the + # duplicate on the rare case TAG is also already a real stable + # release in the list. # # The first `-` is mapped to `~` before sorting because plain sort -V # orders an RC AFTER its own GA (v0.9.0-rc.2 sorts after v0.9.0), so @@ -249,7 +267,14 @@ case "${TAG}" in # above. GNU sort -V orders `~` before end-of-string, so the mapped # `v0.9.0~rc.2` correctly sorts before `v0.9.0`; every stable tag # today is a plain `vX.Y.Z` with no `-`, so the substitution is a - # no-op for them. + # no-op for them. Not an invented trick: this is the same + # `~`-sorts-first convention Debian's dpkg has used for prerelease + # version comparison for years, and GNU sort -V deliberately + # replicates it — reused here for the thing it was designed for, not + # repurposed. It does assume every tag is plain `vX.Y.Z` or + # `vX.Y.Z-suffix` with the hyphen only ever marking a prerelease, + # which is true of every tag this project has cut so far and is this + # project's own naming convention to keep true. want="$(printf '%s' "${TAG}" | sed 's/-/~/')" OLDER_TAG="$( { printf '%s\n' "${all_stable}"; printf '%s\n' "${want}"; } | sort -V -u | awk -v want="${want}" ' $0 == want { print prev; exit }