[PR #105] [MERGED] fix(cursor-review): download-then-execute the CLI installer + assert the binary (BE-5646) #115

New issue

Closed

opened 2026-10-26 22:36:23 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:23 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/105
Author: @mattmillerai
Created: 8/1/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-5646-cursor-review-install-hardening

📝 Commits (4)

a2e23e9 fix(cursor-review): download-then-execute the CLI installer + assert the binary (BE-5646)
951e093 fix(cursor-review): export the real install dir on PATH + bound the installer fetch (BE-5646)
5cdbeee fix(cursor-review): bound the installer run, reject an empty body, mktemp the download (BE-5646)
afb541f fix(cursor-review): SIGKILL escalation, file-not-directory assertion, accurate timeout math (BE-5646)

📊 Changes

1 file changed (+153 additions, -6 deletions)

View changed files

📝 .github/workflows/cursor-review.yml (+153 -6)

📄 Description

ELI-5

The workflow installed the Cursor CLI with curl … | bash. If the download failed, bash still ran (on nothing) and succeeded, so the install step went green — and the run only fell over a step later with cursor-agent: command not found, under an error message blaming the model pins. Now we download the installer to a file first, run it, and then check that the binary actually exists. If either half fails, the install step itself goes red with the real reason.

What changed

All three identical Install Cursor agent CLI steps in .github/workflows/cursor-review.yml (preflight, review matrix, consolidate) now:

curl -fsSL --retry 3 --retry-all-errors --retry-delay 2 … -o /tmp/cursor-install.sh — a failed download is now the step's own exit status, and up to 3 retries ride out a transient 5xx.
bash /tmp/cursor-install.sh — separate command, so its status stands on its own.
An in-step assertion that cursor-agent actually exists, emitting an explicit ::error:: if it does not.

The preflight step keeps its CURSOR_API_KEY skip guard verbatim as its first lines. The three bodies are otherwise byte-identical (verified by parsing the YAML and comparing the step bodies) — GitHub Actions has no YAML anchors and the preflight job has no checkout, so a local composite action is not an option; the triplication matches the rest of the file.

Not done, deliberately: no workflow-level defaults: run: shell: bash was added — it would flip every run step in the file to -eo pipefail, and this workflow deliberately relies on last-command-wins pipelines and && … || … exit-capture idioms elsewhere. The Log Cursor agent version step, the model pins, and every caller repo are untouched. Vendor-outage soft-skip is out of scope: hard-red stays, now correctly attributed to the install step.

Deviation from the ticket — the assertion path (please read)

The ticket prescribed asserting on $HOME/.cursor/bin/cursor-agent. That assertion fires on a successful install, which would have hard-red'd every cursor-review run across every pinned caller repo. The current cursor.com/install script installs to ~/.local/bin:

ln -s ~/.local/share/cursor-agent/versions/<ver>/cursor-agent ~/.local/bin/cursor-agent

There is no .cursor/bin path anywhere in the installer. I ran the real installer against an isolated HOME and confirmed it creates ~/.local/bin/{agent,cursor-agent} and no ~/.cursor directory at all, and a recent successful CI run of this repo's own review panel shows the same in its install log (✓ Detected linux/x64 → ✓ Symlink created → Start using Cursor Agent: agent), followed by cursor-agent --version → 2026.07.23-e383d2b.

So the assertion here targets the real location, and is written to err toward not failing — it only errors when both the PATH lookup and the explicit path come up empty:

if ! command -v cursor-agent >/dev/null 2>&1 \
  && [ ! -x "$HOME/.local/bin/cursor-agent" ]; then

A false positive on this check would break the review panel in every consumer repo at once, so the lenient direction is the right one.

Related finding, left alone as out of scope: the pre-existing echo "$HOME/.cursor/bin" >> "$GITHUB_PATH" line is vestigial — that directory is never created, and cursor-agent resolves today only because ~/.local/bin is already on the GitHub-hosted runner's default PATH. The ticket says the success path and the PATH line must be unchanged, so I left it (and said so in a code comment). Worth a follow-up to either point it at ~/.local/bin or drop it; today it is a harmless no-op, but it is also the reason the ticket's assertion path looked plausible.

Verification

actionlint (with shellcheck integration) clean on the whole repo and on the changed file; shellcheck -s bash clean on the extracted step body.
python3 -m unittest discover -s .github/cursor-review/tests — 40 tests, OK (untouched, sanity only). AGENTS.md integrity checker passes (one pre-existing CODEOWNERS warning, unrelated).
Three step bodies confirmed byte-identical apart from the preflight guard, by loading the YAML and diffing the parsed run: strings.
Success path, executed for real: ran the new step body against the live installer with an isolated HOME whose .local/bin was deliberately not on PATH (so the explicit-path fallback was the branch under test) → exit 0, symlinks created.
Failure path, executed for real: pointed the URL at a 500 endpoint → curl: (56) The requested URL returned error: 500 four times (initial + 3 retries), step exits 56. Pointed it at a URL serving a 200 HTML page → bash fails parsing it, step exits 2. Both go red at the install step, which is the whole point.
Not run: an end-to-end panel run on a fork PR. The preflight guard is unchanged, so fork behavior is untouched.

Notes

/tmp/cursor-install.sh is a predictable path, which would matter on a self-hosted runner; every job in this workflow is runs-on: ubuntu-latest (GitHub-hosted, single-tenant per job), so there is no shared-runner race here.
--retry-all-errors needs curl ≥ 7.71; the ubuntu-latest images ship 8.x.
Merging this triggers bump-cursor-review-callers.yml, which opens SHA-bump PRs in the pinned caller repos automatically — no caller-side edits in this PR.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/105 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/1/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-5646-cursor-review-install-hardening` --- ### 📝 Commits (4) - [`a2e23e9`](https://github.com/Comfy-Org/github-workflows/commit/a2e23e90a0b647939d3e1134be936372607188f8) fix(cursor-review): download-then-execute the CLI installer + assert the binary (BE-5646) - [`951e093`](https://github.com/Comfy-Org/github-workflows/commit/951e093fe0d70d6857be111b73b9cc1e8a243a7c) fix(cursor-review): export the real install dir on PATH + bound the installer fetch (BE-5646) - [`5cdbeee`](https://github.com/Comfy-Org/github-workflows/commit/5cdbeee6daac3a0fd9d2b49c383c6c351510500d) fix(cursor-review): bound the installer run, reject an empty body, mktemp the download (BE-5646) - [`afb541f`](https://github.com/Comfy-Org/github-workflows/commit/afb541f86e81e18d58e42c16eaa235750f14211a) fix(cursor-review): SIGKILL escalation, file-not-directory assertion, accurate timeout math (BE-5646) ### 📊 Changes **1 file changed** (+153 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/cursor-review.yml` (+153 -6) </details> ### 📄 Description ## ELI-5 The workflow installed the Cursor CLI with `curl … | bash`. If the download failed, `bash` still ran (on nothing) and *succeeded*, so the install step went green — and the run only fell over a step later with `cursor-agent: command not found`, under an error message blaming the model pins. Now we download the installer to a file first, run it, and then check that the binary actually exists. If either half fails, the install step itself goes red with the real reason. ## What changed All three identical `Install Cursor agent CLI` steps in `.github/workflows/cursor-review.yml` (preflight, review matrix, consolidate) now: 1. `curl -fsSL --retry 3 --retry-all-errors --retry-delay 2 … -o /tmp/cursor-install.sh` — a failed download is now the step's own exit status, and up to 3 retries ride out a transient 5xx. 2. `bash /tmp/cursor-install.sh` — separate command, so its status stands on its own. 3. An in-step assertion that `cursor-agent` actually exists, emitting an explicit `::error::` if it does not. The preflight step keeps its `CURSOR_API_KEY` skip guard verbatim as its first lines. The three bodies are otherwise byte-identical (verified by parsing the YAML and comparing the step bodies) — GitHub Actions has no YAML anchors and the preflight job has no checkout, so a local composite action is not an option; the triplication matches the rest of the file. **Not done, deliberately:** no workflow-level `defaults: run: shell: bash` was added — it would flip every run step in the file to `-eo pipefail`, and this workflow deliberately relies on last-command-wins pipelines and `&& … || …` exit-capture idioms elsewhere. The `Log Cursor agent version` step, the model pins, and every caller repo are untouched. Vendor-outage soft-skip is out of scope: hard-red stays, now correctly attributed to the install step. ## Deviation from the ticket — the assertion path (please read) The ticket prescribed asserting on `$HOME/.cursor/bin/cursor-agent`. **That assertion fires on a *successful* install**, which would have hard-red'd every cursor-review run across every pinned caller repo. The current `cursor.com/install` script installs to `~/.local/bin`: ``` ln -s ~/.local/share/cursor-agent/versions/<ver>/cursor-agent ~/.local/bin/cursor-agent ``` There is no `.cursor/bin` path anywhere in the installer. I ran the real installer against an isolated `HOME` and confirmed it creates `~/.local/bin/{agent,cursor-agent}` and no `~/.cursor` directory at all, and a recent successful CI run of this repo's own review panel shows the same in its install log (`✓ Detected linux/x64` → `✓ Symlink created` → `Start using Cursor Agent: agent`), followed by `cursor-agent --version` → `2026.07.23-e383d2b`. So the assertion here targets the real location, and is written to err toward *not* failing — it only errors when both the PATH lookup and the explicit path come up empty: ```bash if ! command -v cursor-agent >/dev/null 2>&1 \ && [ ! -x "$HOME/.local/bin/cursor-agent" ]; then ``` A false positive on this check would break the review panel in every consumer repo at once, so the lenient direction is the right one. **Related finding, left alone as out of scope:** the pre-existing `echo "$HOME/.cursor/bin" >> "$GITHUB_PATH"` line is **vestigial** — that directory is never created, and `cursor-agent` resolves today only because `~/.local/bin` is already on the GitHub-hosted runner's default PATH. The ticket says the success path and the PATH line must be unchanged, so I left it (and said so in a code comment). Worth a follow-up to either point it at `~/.local/bin` or drop it; today it is a harmless no-op, but it is also the reason the ticket's assertion path looked plausible. ## Verification - `actionlint` (with `shellcheck` integration) clean on the whole repo and on the changed file; `shellcheck -s bash` clean on the extracted step body. - `python3 -m unittest discover -s .github/cursor-review/tests` — 40 tests, OK (untouched, sanity only). AGENTS.md integrity checker passes (one pre-existing CODEOWNERS warning, unrelated). - Three step bodies confirmed byte-identical apart from the preflight guard, by loading the YAML and diffing the parsed `run:` strings. - **Success path, executed for real:** ran the new step body against the live installer with an isolated `HOME` whose `.local/bin` was deliberately *not* on `PATH` (so the explicit-path fallback was the branch under test) → exit 0, symlinks created. - **Failure path, executed for real:** pointed the URL at a 500 endpoint → `curl: (56) The requested URL returned error: 500` four times (initial + 3 retries), step exits 56. Pointed it at a URL serving a 200 HTML page → `bash` fails parsing it, step exits 2. Both go red *at the install step*, which is the whole point. - Not run: an end-to-end panel run on a fork PR. The preflight guard is unchanged, so fork behavior is untouched. ## Notes - `/tmp/cursor-install.sh` is a predictable path, which would matter on a self-hosted runner; every job in this workflow is `runs-on: ubuntu-latest` (GitHub-hosted, single-tenant per job), so there is no shared-runner race here. - `--retry-all-errors` needs curl ≥ 7.71; the ubuntu-latest images ship 8.x. - Merging this triggers `bump-cursor-review-callers.yml`, which opens SHA-bump PRs in the pinned caller repos automatically — no caller-side edits in this PR. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:23 +00:00