fix(cursor-review): download-then-execute the CLI installer + assert the binary (BE-5646) - #105
fix(cursor-review): download-then-execute the CLI installer + assert the binary (BE-5646)#105mattmillerai wants to merge 4 commits into
Conversation
…the binary (BE-5646) `curl https://cursor.com/install -fsSL | bash` exits 0 when curl fails: this workflow sets no `shell:`/`defaults:`, so run steps get `bash -e {0}` with no pipefail, and a pipeline's status is its last command's. When the install endpoint returned HTTP 500 the install step went green and the failure resurfaced one step later as `cursor-agent: command not found` under a misleading annotation about the model pins. Replace all three identical `Install Cursor agent CLI` steps (preflight, review matrix, consolidate) with download-then-execute plus an in-step binary assertion, and add `--retry 3 --retry-all-errors --retry-delay 2` so a transient 5xx is ridden out. GitHub Actions has no YAML anchors and the preflight job has no checkout (so no local composite action), so the three copies stay triplicated to match the rest of the file. The assertion targets `$HOME/.local/bin/cursor-agent`, which is where the current installer symlinks the binary — verified by running the live installer against an isolated HOME and by a successful CI run's own install log. The pre-existing `$HOME/.cursor/bin` GITHUB_PATH line is left untouched (it is vestigial: `~/.local/bin` is already on the runner PATH, which is why the version step resolves today). Success path is unchanged. Failure path now goes red at the install step with curl's own error or the explicit missing-binary annotation.
mattmillerai
added
cursor-review
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…nstaller fetch (BE-5646)
Review-panel follow-ups on the install hardening, applied to all three
identical `Install Cursor agent CLI` steps:
- Export `$HOME/.local/bin` to `$GITHUB_PATH` instead of the vestigial
`$HOME/.cursor/bin`. The assertion passes when the binary exists at the
explicit path but is not on PATH; in that case later steps still died with
the misleading `cursor-agent: command not found` the change set out to
remove. `.cursor/bin` is provably dead here — the live installer only ever
creates `~/.local/bin/{agent,cursor-agent}`, and any run reaching that line
has already proven cursor-agent is on PATH or in `~/.local/bin`.
- Download to `"${RUNNER_TEMP:-/tmp}"` rather than a fixed world-writable
`/tmp` path.
- Add `--connect-timeout 10 --max-time 60` so a stalled cursor.com fails at
the install step instead of hanging each of the 8 matrix cells to the
job timeout.
actionlint + shellcheck clean; cursor-review script tests 40/40 OK.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 3 |
| ⚪ Nit | 3 |
Panel: 8/8 reviewers contributed findings.
…temp the download (BE-5646) Second review-panel round, applied to all three `Install Cursor agent CLI` steps: - `timeout 300 bash "$installer"`. The previous timeouts bounded only the ~6 KB bootstrap; the bootstrap itself downloads the cursor-agent payload, which was unbounded — so a stalled cursor.com could still burn the whole job budget and surface as an opaque cancelled job. - `[ -s "$installer" ]` before executing. A 200 with an empty body made the installer a silent no-op, which the post-install assertion could not distinguish from a real install when cursor-agent was already on PATH. - `mktemp` instead of a fixed filename, so nothing can pre-place a symlink at a predictable path for `curl -o` to write through. - `--retry-max-time 120`: `--max-time` is per attempt, so curl had no overall bound. Worst case is now ~2 min of curl + 300s of installer, inside the tightest job cap (preflight, 10 min). - `</dev/null` keeps stdin closed as it effectively was under `curl | bash`. - Corrected the timeout comment, which was copy-pasted from the review job and described a "30-minute cap (x8 matrix cells)" in the 10-minute non-matrix preflight job and the 15-minute consolidate job. actionlint + shellcheck clean; cursor-review script tests 40/40 OK. Step body executed against the live installer with an isolated HOME whose .local/bin was off PATH (exercising the fallback branch): exit 0, symlinks created, $GITHUB_PATH written, `cursor-agent --version` -> 2026.07.23-e383d2b. Empty-body and installer-nonzero paths both exit red at this step with the explicit error.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 5 |
| ⚪ Nit | 3 |
Panel: 8/8 reviewers contributed findings.
… accurate timeout math (BE-5646) Third review-panel round, applied to all three `Install Cursor agent CLI` steps: - `timeout -k 30 240` instead of `timeout 300`. Plain `timeout` sends only SIGTERM, so an installer that traps or ignores it outlives the cap the line exists to guarantee; `-k` escalates to SIGKILL. - The post-install assertion now requires `-f` as well as `-x`. `-x` alone is true for a *searchable directory*, so a stray directory at `$HOME/.local/bin/cursor-agent` satisfied the fallback and let the step pass with no binary. Confirmed both ways against a directory at that path. - Corrected the timeout arithmetic in the comment: `--retry-max-time` bounds only when a new attempt may *start*, so an attempt beginning just under 120s still gets its full `--max-time 60` and curl's worst case is ~3 min, not the ~2 min claimed. 240s + a 30s grace keeps the total inside preflight's 10-minute cap. - The exit-124 message no longer asserts a timeout unconditionally, since an installer can natively exit 124. actionlint + shellcheck clean; cursor-review script tests 40/40 OK.
Review resolution — three panel rounds, all threads resolved25 findings across three cursor-review panel rounds; all threads replied to and resolved. Base is Fixed (13). The one High: the assertion could pass via the explicit-path fallback while only the vestigial Declined with reasoning (10). Mostly premises that aren't reachable here — Deferred to follow-ups (2), recorded for filing rather than dropped:
Verification. One caveat for whoever merges: the panel is label-triggered, so it ran against |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
2 participants
ELI-5
The workflow installed the Cursor CLI with
curl … | bash. If the download failed,bashstill ran (on nothing) and succeeded, so the install step went green — and the run only fell over a step later withcursor-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 CLIsteps 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.cursor-agentactually exists, emitting an explicit::error::if it does not.The preflight step keeps its
CURSOR_API_KEYskip 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: bashwas 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. TheLog Cursor agent versionstep, 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 currentcursor.com/installscript installs to~/.local/bin:There is no
.cursor/binpath anywhere in the installer. I ran the real installer against an isolatedHOMEand confirmed it creates~/.local/bin/{agent,cursor-agent}and no~/.cursordirectory 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 bycursor-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:
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, andcursor-agentresolves today only because~/.local/binis 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/binor drop it; today it is a harmless no-op, but it is also the reason the ticket's assertion path looked plausible.Verification
actionlint(withshellcheckintegration) clean on the whole repo and on the changed file;shellcheck -s bashclean 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).run:strings.HOMEwhose.local/binwas deliberately not onPATH(so the explicit-path fallback was the branch under test) → exit 0, symlinks created.curl: (56) The requested URL returned error: 500four times (initial + 3 retries), step exits 56. Pointed it at a URL serving a 200 HTML page →bashfails parsing it, step exits 2. Both go red at the install step, which is the whole point.Notes
/tmp/cursor-install.shis a predictable path, which would matter on a self-hosted runner; every job in this workflow isruns-on: ubuntu-latest(GitHub-hosted, single-tenant per job), so there is no shared-runner race here.--retry-all-errorsneeds curl ≥ 7.71; the ubuntu-latest images ship 8.x.bump-cursor-review-callers.yml, which opens SHA-bump PRs in the pinned caller repos automatically — no caller-side edits in this PR.