Skip to content

ci: harden leanSpec fixtures release-info step#507

Open
MegaRedHand wants to merge 2 commits into
mainfrom
fix/harden-fixture-release-step
Open

ci: harden leanSpec fixtures release-info step#507
MegaRedHand wants to merge 2 commits into
mainfrom
fix/harden-fixture-release-step

Conversation

@MegaRedHand

Copy link
Copy Markdown
Collaborator

Problem

The daily scheduled CI intermittently fails at the very first step of the run-fixture-tests action (before any Rust builds), e.g. run 28942947291:

##[error]Unable to process file command 'output' successfully.
##[error]Invalid format '<!--'

Root cause

The Get leanSpec fixtures release info step fetched the release metadata and the checksum asset with curl -sL (no -f). When the GitHub API / asset CDN returned a transient HTML error page (rate-limit or 5xx), that HTML was captured as data and written verbatim into $GITHUB_OUTPUT, corrupting the key=value file (<!-- is the offending line). The failure was silent because a command substitution inside an assignment (var=$(...)) does not trip set -e, so the bad value flowed straight through. Unauthenticated API calls from shared-runner IPs make the rate-limit trigger routine; re-running the job usually "fixes" it.

Fix

  • Authenticate the API call with github.token (avoids the shared unauthenticated rate-limit pool).
  • Add -f + --retry 5 --retry-all-errors so HTTP errors fail fast and transient hiccups are retried instead of captured as data.
  • Fail loudly on each curl/parse (assignments don't propagate errexit on their own).
  • Validate the checksum is a 64-char hex string before writing to $GITHUB_OUTPUT.

Testing

  • python -c yaml.safe_load + bash -n on the extracted script: pass.
  • Synthetic: asset_url lookup resolves both assets; hex guard accepts a valid sha and rejects <!--.
  • Live end-to-end against the current leanEthereum/leanSpec latest release: resolves both asset URLs and a valid 64-hex checksum.

The daily CI intermittently failed with `Invalid format '<!--'` when
writing to $GITHUB_OUTPUT. Root cause: the fixtures release-info step
fetched the release metadata and checksum with `curl -sL` (no `-f`), so a
transient HTML error page from the GitHub API/asset CDN was captured as
data and written verbatim into $GITHUB_OUTPUT, corrupting it. A command
substitution in an assignment does not trip `set -e`, so the bad value
propagated silently.

Harden the step:
- Authenticate the API call with github.token (unauthenticated requests
  share the runner IP's low rate-limit pool, the usual trigger).
- Add `-f` + `--retry`/`--retry-all-errors` so HTTP errors fail fast and
  transient hiccups are ridden out instead of captured as data.
- Fail loudly on each curl/parse (assignments don't propagate errexit).
- Validate the checksum is a 64-char hex string before writing output.
Apply the same --retry/--retry-all-errors hardening to the fixtures and
checksum downloads, so a transient blip in the download step can't flake
CI either. Still -f-guarded and sha256-verified.
@MegaRedHand MegaRedHand marked this pull request as ready for review July 8, 2026 14:24
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Review: ci: harden leanSpec fixtures release-info step (PR 507)

This is a CI-only change (.github/actions/run-fixture-tests/action.yml), no Rust/consensus code touched. Overall the fix is well-targeted at the actual root cause (unauthenticated, non--f curl silently capturing an HTML error page into $GITHUB_OUTPUT) and the reasoning in the PR description matches the diff. A few observations:

Correctness

  • Good catch on the set -e gap (line 18/24-41): assignments via var=$(...) don't trip errexit on their own, and each risky call now has an explicit || { ...; exit 1; }. This is correct and necessary.
  • asset_url() helper (lines 30-34) correctly captures $json from the enclosing scope and uses "$1" as the asset name argument — a nice dedup of what was previously two near-identical python3 -c invocations.
  • Hex validation (line 45) is a solid last line of defense against exactly the failure mode described (<!-- leaking into $sha).
  • pipefail interaction: with set -o pipefail active, curl ... | cut -d' ' -f1 (line 40) correctly propagates curl's failure through the pipeline into the $() assignment, so the || fallback triggers as intended. Verified this reasoning is sound — no bug here.

Minor / non-blocking

  1. --retry-all-errors retries on permanent errors too, not just transient ones (e.g., a genuinely missing/renamed asset returning 404 from sha_url at line 40, or a bad token producing 401 at line 24). Since --retry-all-errors doesn't distinguish transient vs. permanent failures, a real misconfiguration will now take ~5 retries (with backoff) to fail instead of failing immediately. This is a reasonable tradeoff for the stated goal (rate-limit/5xx resilience) but worth being aware of — CI failure feedback for a genuine break will be slightly slower.
  2. No --max-time/--connect-timeout on any of the curl calls. If a server accepts the connection but hangs without erroring, --retry won't help (retry only kicks in after curl gives up or errors). Not a regression from this PR, but since the step is being hardened anyway, a bounded timeout would close that gap too.
  3. Duplicated --retry 5 --retry-all-errors flags across three curl invocations (lines 24, 40, 73-74). Could be pulled into a shared array (e.g. retry_opts=(--retry 5 --retry-all-errors)) to avoid drift if the retry policy changes later — purely stylistic, low value for a 4-line shell script.
  4. GH_TOKEN env var name (line 16): reused verbatim via $GH_TOKEN in the Authorization: Bearer header, which is fine functionally, but GH_TOKEN is also the conventional env var the gh CLI auto-reads. No collision here since gh isn't invoked, just a naming note for future readers.

Security

  • Token is passed via env: and only used in an Authorization header with -sS (no -v), so it won't leak into logs under normal operation. No injection concerns — fixtures_url/sha_url/sha originate from the trusted leanEthereum/leanSpec release API, consistent with the pre-existing trust boundary (not something this PR changes or needs to address).

Verdict

No functional bugs found. The hardening logic is correct and matches the documented root cause. The retry-on-permanent-errors point (Item 1) is the only thing worth a second look, but it's a minor tradeoff, not a defect.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

No findings.

This PR only touches the composite CI action, so the consensus-layer items you listed are not in scope for this diff. The changes in action.yml look sound: set -euo pipefail, curl -f with retries, explicit failure branches around command substitutions, and the SHA-256 format check at line 45 all harden the existing fixture-fetch path without introducing obvious security, correctness, or performance regressions.

The only residual thing I’d validate is a real pull_request run from a fork, since the release-metadata fetch now depends on github.token for a cross-repo API call at line 26. I don’t see a shell bug there, but that is the one CI-only behavior change worth checking in practice.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

Context Note: This PR modifies CI infrastructure (GitHub Actions bash scripts), not Rust consensus code. Therefore, items like LMD GHOST, XMSS verification, and SSZ encoding are not applicable to this diff. The review below focuses on shell script correctness, CI security, and workflow reliability.

Issues Found

1. Curl version compatibility risk (Lines 22, 40, 70-71)
The --retry-all-errors flag requires curl 7.71.0+. Ubuntu 20.04 LTS runners (still used in some environments) ship curl 7.68.0, which will cause the script to fail with "unknown option".
Suggestion: Either remove --retry-all-errors (relying on --retry 5 alone for transient failures) or add a version check:

# Add before curl calls
curl_version=$(curl --version | head -n1 | grep -oP '\d+\.\d+\.\d+')
if [[ "$(printf '%s\n' "7.71.0" "$curl_version" | sort -V | head -n1)" == "7.71.0" ]]; then
    retry_all="--retry-all-errors"
else
    retry_all=""
fi

2. Misleading error messages on Python exceptions (Lines 35-38)
If the GitHub API returns invalid JSON (causing json.load to raise) or an asset lacks the browser_download_url key (causing KeyError), the || trap will emit "Release is missing..." which is inaccurate.
Suggestion: Distinguish between "asset not found" (StopIteration) and other failures:

asset_url() {
  echo "$json" | python3 -c \
    "import sys,json; name=sys.argv[1]; j=json.load(sys.stdin); \
     matches=[a['browser_download_url'] for a in j.get('assets',[]) if a.get('name')==name]; \
     print(matches[0]) if matches else (sys.stderr.write(f'Asset {name} not found\n') or sys.exit(2))" \
    "$1"
}
# Then handle exit code 2 separately or use a generic "failed to parse" message.

3. Potential word splitting in hash validation (Line 44)
While unlikely with hex output, unquoted $sha in the regex test could behave unexpectedly if the curl output contains newlines.
Suggestion: Use [[ ! "${sha}" =~ ^[0-9a-fA-F]{64}$ ]] (already quoted in diff, but verify the variable is trimmed).

Security & Correctness (Positive)

4. Authentication and secret handling (Line 4-6, 24)
Correctly uses github.token via environment variable (GH_TOKEN) rather than command-line arguments, preventing token leakage in process listings. The -H "Authorization: Bearer ..." header is the proper mechanism for GitHub API authentication.

5. Input validation before GITHUB_OUTPUT (Lines 43-46)
The SHA256 format validation (^[0-9a-fA-F]{64}$) is an excellent defense-in-depth measure against the documented issue where transient HTML error pages (e.g., GitHub's "Server Error" HTML) could otherwise be written to $GITHUB_OUTPUT, corrupting the workflow state.

6. Error handling improvements (Line 15)
The addition of set -euo pipefail is critical for CI reliability:

  • -e: Prevents silent failures on individual command errors
  • -u: Catches undefined variable references (e.g., typos)
  • -o pipefail: Ensures curl | cut fails if either component fails

7. Retry logic
Adding --retry 5 (and the noted --retry-all-errors) appropriately addresses transient network failures when downloading large fixture archives.

Minor Suggestions

**8. Consider jq over Python (Lines 28-34)


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR hardens the Get leanSpec fixtures release info step in the composite action to prevent transient HTML error pages from being silently captured and corrupting $GITHUB_OUTPUT. The root cause — unauthenticated curl without -f, where command-substitution assignments bypass set -e — is correctly identified and addressed.

  • Adds GH_TOKEN authentication to the GitHub API call, moving requests off the shared unauthenticated rate-limit pool, and adds -f plus --retry 5 --retry-all-errors to both the metadata fetch and asset download.
  • Introduces explicit || { echo "::error::..."; exit 1; } guards after every command-substitution assignment, and validates the checksum is a 64-char hex string before writing to $GITHUB_OUTPUT.
  • Also applies --retry 5 --retry-all-errors to the file download step for consistent resilience.

Confidence Score: 5/5

The change is a focused CI hardening of a single composite action step, with no production code affected. All error paths are explicit and the validation is conservative.

Each failure mode introduced by the original code (silent HTML capture, unchecked assignment exit codes, missing SHA validation) is addressed with a matching guard. The retry + auth combination directly targets the known failure trigger (shared-runner rate-limits), and the hex regex catches any remaining data corruption before it reaches GITHUB_OUTPUT. No functional logic changes to Rust builds or test orchestration.

No files require special attention.

Important Files Changed

Filename Overview
.github/actions/run-fixture-tests/action.yml Hardens the release-info step: adds GitHub token auth, curl -f + retries, explicit error traps after each assignment, and SHA256 format validation before writing to GITHUB_OUTPUT. Logic is sound and directly addresses the root cause.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Get leanSpec fixtures release info] -->|curl with GH_TOKEN auth, retry 5| B[Fetch release JSON from GitHub API]
    B -->|HTTP error| C[Error: Failed to fetch metadata, exit 1]
    B -->|success| D[asset_url: extract fixtures_url]
    D -->|asset missing| E[Error: Missing .tar.gz asset, exit 1]
    D -->|success| F[asset_url: extract sha_url]
    F -->|asset missing| G[Error: Missing .sha256 asset, exit 1]
    F -->|success| H[curl sha_url, pipe to cut]
    H -->|HTTP error| I[Error: Failed to download checksum, exit 1]
    H -->|success| J{sha matches hex64 regex?}
    J -->|no - HTML or garbage| K[Error: Bad SHA format, exit 1]
    J -->|yes| L[Write url, sha_url, sha to GITHUB_OUTPUT]
    L --> M[Cache restore by sha key]
    M -->|cache hit| N[Skip download step]
    M -->|cache miss| O[Download .tar.gz and .sha256 with retry 5]
    O --> P[sha256sum verify]
    P -->|mismatch| Q[exit 1]
    P -->|match| R[Extract fixtures and save cache]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Get leanSpec fixtures release info] -->|curl with GH_TOKEN auth, retry 5| B[Fetch release JSON from GitHub API]
    B -->|HTTP error| C[Error: Failed to fetch metadata, exit 1]
    B -->|success| D[asset_url: extract fixtures_url]
    D -->|asset missing| E[Error: Missing .tar.gz asset, exit 1]
    D -->|success| F[asset_url: extract sha_url]
    F -->|asset missing| G[Error: Missing .sha256 asset, exit 1]
    F -->|success| H[curl sha_url, pipe to cut]
    H -->|HTTP error| I[Error: Failed to download checksum, exit 1]
    H -->|success| J{sha matches hex64 regex?}
    J -->|no - HTML or garbage| K[Error: Bad SHA format, exit 1]
    J -->|yes| L[Write url, sha_url, sha to GITHUB_OUTPUT]
    L --> M[Cache restore by sha key]
    M -->|cache hit| N[Skip download step]
    M -->|cache miss| O[Download .tar.gz and .sha256 with retry 5]
    O --> P[sha256sum verify]
    P -->|mismatch| Q[exit 1]
    P -->|match| R[Extract fixtures and save cache]
Loading

Reviews (1): Last reviewed commit: "ci: retry fixtures download on transient..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant