fix(deploy): auto-install flyctl on Linux - #86
Conversation
… on a hand-install ensureFlyctl only auto-installed via brew on macOS; a fresh Linux machine (CI containers included — insta-e2e run 31284364163) got a hint and a failed build. Official installer into ~/.fly, and the current process extends its own PATH since the installer's shell-profile edit can't reach a running process. Still best-effort: on failure the same manual hint prints. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPyX5GsxHvWqLpPYiUCfb5
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
jwfing
left a comment
There was a problem hiding this comment.
Summary
This PR fixes the reported Linux flyctl bootstrap gap for source deploys, and I did not find any blocking issues, but there are a couple of non-blocking robustness/security follow-ups worth addressing.
Requirements context
I based intent on the PR description plus the local deploy flow in src/commands/deploy.ts, which requires ensureFlyctl() before source builds, and on Fly.io’s current install docs, which document Linux installation via curl -L https://fly.io/install.sh | sh (https://fly.io/docs/flyctl/install/). I did not find repo-local requirements beyond the PR description for how Linux auto-install must be implemented.
Findings
Critical
(none)
Suggestion
- src/flyctl-build.ts: The
HOMEfallback is ineffective. Whenprocess.env.HOMEis unset,flyHomebecomes the literal string~/.fly, but the installer command wrapsFLYCTL_INSTALLin double quotes, so~will not expand in the shell. In that case the install/verification path becomes incorrect and the new Linux auto-install still dead-ends in exactly the kind of minimal environment this PR is targeting. Usingos.homedir()or handling the unset-HOMEcase without relying on quoted tilde expansion would make this reliable. - src/flyctl-build.ts: The new installer path interpolates
flyHomeinto ash -ccommand string. SinceflyHomeis derived fromprocess.env.HOME, any shell metacharacters in that environment variable are interpreted by the shell. This is a local/CI hardening issue rather than a blocker, but it would be safer to passFLYCTL_INSTALLthrough the child process environment instead of embedding it in the shell command.
Information
- test/flyctl-build.test.ts: There is still no regression coverage for
ensureFlyctl()at all, so the new Linux branch is untested despite the repo’s general pattern of injecting side-effectful runners for unit tests. I would add coverage for the installer invocation and PATH update in a follow-up, but I would not block merge on that alone. - Performance: no performance-relevant issues found in this change.
- Security: no blocking security issues found, aside from the shell-hardening suggestion above.
Verdict
approved
jwfing
left a comment
There was a problem hiding this comment.
Review — fix(deploy): auto-install flyctl on Linux
Summary: Adds a Linux branch to ensureFlyctl that runs Fly's official installer into ~/.fly, extends the current process's PATH, and verifies with flyctl version — a clean, correctly-scoped fix for the fresh-Linux/CI dead-end the PR describes.
Requirements context: No docs/superpowers/ or docs/specs/ directory exists in this repo — assessing against the PR description alone (unblock insta deploy <dir> on fresh Linux/CI where flyctl isn't preinstalled and brew isn't available). The change matches that intent, and correctly mirrors the existing best-effort macOS/brew path. CI gates here are tsc --noEmit + vitest run (no lint gate).
Findings
Critical
(none) — the change is purely additive (a new process.platform === 'linux' branch inserted before the final fallback hint), so no existing behavior on macOS/other platforms changes, and the whole function stays wrapped in the best-effort try/catch.
Suggestion
-
Software engineering — no test for the new branch (
src/flyctl-build.ts:88-102).ensureFlyctlhas zero test coverage today because its internalokclosure (:75-80) spawns real processes and isn't injectable the wayBuildRunneris forflyctlBuildAndPush. The macOS branch is likewise untested, so this isn't a regression — but since this branch is the one now shelling out to a network installer, it's the strongest candidate for coverage. Consider liftingokto a default parameter (likerun: BuildRunner = defaultBuildRunner) so the platform branches can be exercised with a stubbed runner (assert the linux path issues the installer command and appends~/.fly/binto PATH). Non-blocking. -
Functionality —
HOME-unset fallback is non-functional (src/flyctl-build.ts:94-97).const flyHome = \${process.env.HOME ?? ''}/.fly`/.flyyields the literal stringwhenHOMEis unset. That literal is then passed asFLYCTL_INSTALL="/.fly"— tilde is **not** expanded inside double quotes, so the installer would create a directory literally namedin the cwd, and the subsequentprocess.env.PATH = ...:~/.fly/binentry won't resolve forspawneither (Node does no tilde expansion). Blast radius is low (HOME is essentially always set on Linux), but the fallback silently does the wrong thing rather than failing clean — preferable to bail to the manual hint whenHOME` is absent. Non-blocking.
Information
-
Security —
curl … | shfetch-and-execute (src/flyctl-build.ts:95). Piping a remote script intoshis the documented Fly install path and is consistent with the existing brew branch; it's over HTTPS (-fsSL), best-effort, and only triggered when the user is already runninginsta deploy. Note that Fly'sinstall.shalso silently appends to the user's shell rc — an expected side effect of the official installer, and the reason the code correctly extends the running process's own PATH. Nothing new here beyond what invoking flyctl already implies; flagged only for visibility. -
Security — pipeline exit status doesn't reflect curl (
src/flyctl-build.ts:95). Withoutpipefail,curl … | shreturnssh's exit code, so a failed download (bad network/404) still yields exit 0 and setsinstalled = true. This is not a bug: theflyctl versionrecheck at:98is the real success gate, so a failed download still falls through to the manual-install hint. Addingset -o pipefail(orsh -eo pipefail) would makeinstalledtruthful and is a minor robustness nicety, not a correctness issue. -
Security —
HOMEinterpolated into the shell string (src/flyctl-build.ts:94-95).flyHomeis substituted into a double-quotedsh -ccommand. The value comes from the user's ownHOME, so this is at most self-injection on the user's own machine — no untrusted input crosses a boundary. No action needed. -
Performance: no concerns — one-time install path, no hot-loop / N+1 / blocking-I/O implications.
Verdict
approved (informational — the human still gives the explicit GitHub approval). The fix is well-targeted, additive, and defensively written; the version-recheck gate makes it robust to installer failure. The suggestions above (add a regression test via an injectable runner; harden the HOME-unset fallback) are worth a follow-up but none are blocking.
Found by the newly wired insta-e2e fixture step (run 31284364163):
insta deploy <dir>on a fresh Linux machine prints "flyctl not found — install it to deploy from source" and fails —ensureFlyctlonly auto-installs via brew on macOS. That's a dead-end for exactly the agent/CI audience the CLI targets.Linux branch: official installer into
~/.fly(FLYCTL_INSTALL), extend the running process's PATH, verify withflyctl version. Best-effort like the brew path — failure falls back to the manual hint. 232/232 tests, typecheck clean.insta-e2e#15 installs flyctl in the CI container as the interim; this makes that unnecessary from the next release (rides v0.0.31, no release cut needed now).
🤖 Generated with Claude Code
https://claude.ai/code/session_01LPyX5GsxHvWqLpPYiUCfb5
Summary by cubic
Auto-install
flyctlon Linux to unblockinsta deployon fresh machines and CI. Best-effort: if install fails, we still show the manual install hint.ensureFlyctl: run the official installer withFLYCTL_INSTALL=~/.flyand append~/.fly/binto PATH for the current process.flyctl version; otherwise print the docs link.flyctlin e2e/CI containers.Written for commit 0a56bf7. Summary will update on new commits.