release: externalize and locally test releases index generation

[tazarov]

Summary

• externalize releases.json generation into scripts/build_releases_index.sh
• make release index generation resilient to missing/malformed/wrong-schema input
• add scripts/test_build_releases_index.sh and make test-release-index for local validation
• wire rust-release.yml to call the script instead of inline jq
• validate jq availability and --date RFC3339 format in the script

Why

The previous inline workflow logic was hard to test locally. This change keeps behavior in one script, makes edge-case handling explicit, and adds a local test harness.

Validation

• make test-release-index
• bash -n scripts/build_releases_index.sh scripts/test_build_releases_index.sh
• actionlint .github/workflows/rust-release.yml

[claude]

Review

Good refactoring. Externalizing the jq logic makes it genuinely easier to test locally, and the edge-case coverage is solid.

Minor issues

Arg-parsing silently accepts a missing flag value (build_releases_index.sh)

The argument loop uses a default-to-empty pattern so passing a flag as the last argument without a value silently sets the variable to empty string. The script then fails with the generic Missing required arguments message. A non-empty check before assigning each value would make CI failures easier to diagnose.

No test for omitting --existing entirely

--existing is documented as optional, but every test case passes the flag. A case that omits it altogether would confirm the optional path works independently of the file-existence check.

dedupe_first_by_version is O(n^2)

Fine at max=100, but worth a short comment so a future reader does not replace it with unique_by (which would change dedup semantics).

Nits

• The workflow now does rm -f on the existing file on S3 failure, then unconditionally passes --existing pointing at the possibly absent path. The script handles this correctly, but a brief inline comment in the workflow step would help future readers understand the intent.
• Consider wiring shellcheck into the existing lint target to catch shell script issues in CI automatically.

What is working well

• set -euo pipefail + mktemp + trap is the right pattern.
• fromdateiso8601 for RFC3339 validation is a good use of jq built-ins.
• The normalize + dedupe_first_by_version pipeline correctly prioritises the new entry over an existing duplicate (prepend then dedupe from the front).
• Test cases 1-7 cover the important edge cases: malformed JSON, wrong schema, mixed array, deduplication, max bound, and invalid date.

Overall a net improvement - the logic is easier to reason about and validate.

[claude]

Supplementing the existing review with two minor points not covered:

Workflow: releases_fetch_err temp file leaks on unexpected S3 error

releases_fetch_err="/tmp/releases-fetch.err"
if ! aws s3 cp ... 2>"${releases_fetch_err}"; then
  if grep -qiE '404|NoSuchKey|Not Found|HeadObject operation' "${releases_fetch_err}"; then
    rm -f /tmp/releases-existing.json
  else
    cat "${releases_fetch_err}" >&2
    exit 1          # ← releases_fetch_err not cleaned up here
  fi
fi
rm -f "${releases_fetch_err}"   # only reached on success or 404 branch

On the exit 1 path the error temp file is left behind. Minor on ephemeral CI runners, but using trap (as the script itself does with mktemp) would be consistent.

Workflow: HeadObject operation in the error regex is fragile

grep -qiE '404|NoSuchKey|Not Found|HeadObject operation'

HeadObject operation is an AWS SDK phrase that could change across CLI versions or differ on R2. NoSuchKey and 404 already cover the canonical "object not found" cases — the third pattern adds noise without clear benefit and could hide future regressions where a genuine non-404 error message happens to contain those words.

[claude]

Correction to first review: The first review states there is no test for omitting --existing entirely, but Case 9 in test_build_releases_index.sh explicitly omits the flag and asserts correct output, so that path is already covered.

Date validation gap for existing entries: normalize in build_releases_index.sh validates that .date is a non-empty string but not that it is a valid ISO8601 timestamp. An existing index entry with a malformed date (e.g. from a past manual edit) would survive normalization, be included in the output, and sort incorrectly under sort_by(.date) (which is lexicographic). Low risk in practice since entries are written by this same script, but worth being aware of.

[claude]

Following up on the previous reviews after the latest commits:

What was addressed:

• The fragile HeadObject operation grep pattern is now 404|NoSuchKey — good.
• Case 9 in the test script already covers omitting --existing entirely (corrected in earlier comment).

Still open — temp file cleanup on exit 1 paths in the workflow:

The pattern now appears in two places without cleanup on failure:

immutability_check_err="/tmp/immutability-check.err"
# ...
exit 1  # immutability_check_err not removed

releases_fetch_err="/tmp/releases-fetch.err"
# ...
exit 1  # releases_fetch_err not removed

Inconsequential on ephemeral runners, but inconsistent with the script's own trap pattern. A single trap at the top of the step (or rm -f before each exit 1) would resolve both.

[claude]

One remaining nit in build_releases_index.sh (not covered in previous reviews):

The EXIT trap is updated to drop $tmp_output before the mv that consumes it:

jq -e '.releases | type == array' "$tmp_output" >/dev/null
trap 'rm -f "$tmp_existing"' EXIT   # ← resets trap before mv
mv "$tmp_output" "$output_path"   # ← if this fails, tmp_output leaks

If mv fails (unlikely but possible on a full disk or cross-device move), $tmp_output is not cleaned up. Swapping the last two lines fixes it:

jq -e '.releases | type == array' "$tmp_output" >/dev/null
mv "$tmp_output" "$output_path"
trap 'rm -f "$tmp_existing"' EXIT   # update trap only after successful mv

All prior feedback has been addressed — LGTM overall.