perf(install-cli): cache the binary per job, reuse across steps#28
Conversation
Each action ran its own install step with a fresh mktemp dir, so a multi-action job re-downloaded the binary every time (e.g. android-setup -> android-build -> 2x artifact-upload = 4 downloads). Cache it under $RUNNER_TEMP keyed by asset+version: the first action downloads, the rest reuse — one download per job. $RUNNER_TEMP is unique per job and not shared by concurrent jobs on a runner, so reuse is safe and can't collide (keeps the no-static-/tmp-path hardening; the non-Actions fallback still uses mktemp). Download is published atomically (mv into place only after checksum passes), so a partial/failed download is never mistaken for a cache hit. Verified locally: download-once then reuse (same path), fresh job re-downloads, tampered asset fails closed with no cache entry leaked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a job-scoped caching mechanism in scripts/install-cli.sh to reuse the downloaded flutter-tools binary across multiple actions within the same job, reducing redundant network downloads. The feedback recommends enhancing the script's robustness and security by restricting the cache directory permissions to 0700 immediately after creation, explicitly handling potential failures of mktemp when creating temporary files, and ensuring the script exits with an error if the atomic publish (mv) command fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![lightbridge-assistant[bot]](https://avatars.githubusercontent.com/u/128943481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🅵 Fast automated pass — SAST + a quick, diff-scoped look (no repo-wide retrieval). For a deeper, repo-aware review, mention @lightbridge-assistant on this PR.
The change correctly implements job-scoped caching: it skips the network when the binary already exists under $RUNNER_TEMP/<asset>/<version> and re-uses it across steps. Two minor observability notes: (1) $GITHUB_PATH now points one level up from the binary to an asset+version-keyed directory, which widens what is on PATH; (2) the cache-hit check ([ -x "$bin" ]) and the publish (mv -f "$tmp" "$bin") are not atomic, so a concurrent download completing as a step starts could be overwritten without causing a functional problem but would trigger a redundant download. Neither is a blocker.
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
Follow-up to #28 review. The mktemp fallback already created a 0700 dir; the RUNNER_TEMP path used mkdir -p (umask-dependent, typically 0755). Lock the cache dir to owner-only on both paths so the binary we exec can't be read/replaced by another user on a shared self-hosted host. (The other #28 review comments were false positives under set -euo pipefail / the mv+rm cleanup — rebutted on the threads.) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What
Make
install-cli.shcache the binary per job and reuse it across steps, instead of re-downloading in every action's install step.Why
A multi-action job downloaded the binary once per action — e.g.
android-setup→android-build→ 2×artifact-upload= 4 downloads. Now it's one download per job; later actions reuse the cached binary.How
asset+versionunder$RUNNER_TEMP, which is unique per job and not shared by concurrent jobs on a runner — so reuse is safe and can't collide (preserves the earlier "no static/tmppath" hardening). Outside Actions (no$RUNNER_TEMP) it falls back to a freshmktempdir (no reuse, still collision-safe).mv'd into place only on success — so a partial/failed download is never mistaken for a cache hit by a later step.Downloaded …vsReusing … from this job's cache …so the saving is visible.Verified locally
$RUNNER_TEMP) reuses — same path, no network.$RUNNER_TEMPchanges) re-downloads → job isolation intact.Release note
Script-only change (Flow B in docs/RELEASING.md) — no
cli-version.txtbump, norelease-cli. After merge: dispatchrelease-actionsto cut the next version and movev0.🤖 Generated with Claude Code