feat(installer): implement install.sh per 07-spec-installer - #645

.github/workflows/release.yml (1)

520-530: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider staging install.sh via scripts/stage-release.sh instead of a standalone gcloud storage cp.

The step immediately above (Line 516-519) already stages release artifacts through scripts/stage-release.sh, which presumably centralizes the immutable cache-control header and bucket-path conventions. This new step duplicates that header inline instead of extending the shared script, so the two staging paths can drift (e.g. if the cache-control policy changes later, only one call site gets updated).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release.yml around lines 520 - 530, The installer staging
step duplicates release-upload logic and cache-control settings instead of using
the shared staging flow. Update the release workflow to stage install.sh through
scripts/stage-release.sh, reusing the same bucket/path conventions and immutable
header handling already used for the other release artifacts. Keep the change
aligned with the existing release artifact staging steps so the logic stays
centralized and consistent.

scripts/install_test.sh (2)

241-250: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Traversal test uses ../evil, which is rejected only because of the /, not because .. is blocked.

This test doesn't cover a bare ".." target (no slash), which — per the companion finding on scripts/install.sh Lines 124-127 — currently passes the charset validation unmodified since every character in .. is individually allowed. Once that validation gap is fixed, please add a case asserting run traversal2 "$H3" ".." also exits non-zero, to lock in the fix.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/install_test.sh` around lines 241 - 250, The traversal coverage in
install_test.sh only exercises "../evil", which can fail for the slash rather
than for dot-dot traversal. Update the Unit 2 validation block to also test the
bare ".." target using the existing run traversal2 helper, and assert it exits
non-zero like the other invalid target cases. Keep the new assertion alongside
run traversal and run emptytarget so the install.sh charset validation fix is
locked in by a direct check on the traversal2 path.

---

102-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fallback downloader/hasher branches (wget, shasum, openssl) are never exercised.

The stub wget always exits 1 (Line 128-132), so install.sh's DL_TOOL=wget branch and the wget case in fetch() (install.sh Line 58) are dead code as far as this suite goes — curl is always found first. Likewise, whichever SHA_TOOL the CI host happens to have wins, so the shasum/openssl branches of install.sh's sha256() are only covered opportunistically, not deliberately, depending on the runner.

Consider adding a scenario that hides the curl stub from PATH (functional wget stub) to force and assert the wget path, and one that forces SHA_TOOL selection order via a similar PATH trick, so all of Unit 1's fallback branches get direct coverage rather than relying on host happenstance.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/install_test.sh` around lines 102 - 134, The test setup currently
never exercises the fallback downloader/hasher paths because the stubbed curl is
always preferred and the stubbed wget always fails, so the install.sh fetch()
wget branch and sha256() shasum/openssl branches are not directly covered.
Update the install_test.sh scenario around the curl/wget stubs to add a case
that removes curl from PATH and provides a working wget stub so install.sh is
forced through the DL_TOOL=wget path, and add a separate PATH-controlled
scenario that forces SHA_TOOL selection to verify both shasum and openssl
branches deterministically. Use the existing install.sh fetch() and sha256()
logic, plus the stubbin curl/wget setup, to locate and adjust the test coverage.

scripts/install.sh (1)

167-169: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Install record write is not same-filesystem atomic, unlike per-component installs.

Per-component installs are deliberately made atomic by writing the temp file as a sibling of target_file (Lines 226-228 comment) before mv -f. The install record, however, is built at $tmpdir/installed (under /tmp typically) and then mv -f'd to $prev_record under $state_dir (typically $HOME/.local/state/...) — usually a different filesystem, so mv falls back to copy+unlink and is not atomic. An interruption mid-write could leave a truncated/corrupt record.

Impact is bounded (a corrupted line just fails the hash match on the next run, causing an extra download/re-sign — not silent corruption of installed binaries), but this undermines the "reruns are cheap" goal (Goal 2) referenced by the macOS signing tests.

♻️ Proposed fix: build the record in-place under `state_dir`

+mkdir -p "$state_dir"
-records="$tmpdir/installed"
+records="$state_dir/installed.tmp.$$"
 : >"$records"

And drop the later mkdir -p "$state_dir" before the final mv -f "$records" "$prev_record", which now becomes a same-directory atomic rename.

Also applies to: 263-269

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/install.sh` around lines 167 - 169, The install record is being
written to a temp file under $tmpdir and later moved into $state_dir, which
makes the final rename non-atomic across filesystems. Update the install record
flow in scripts/install.sh so the record is created in-place under state_dir as
a sibling of the final $prev_record target, then keep the final mv -f as a
same-directory atomic rename. Use the existing records/$prev_record handling in
the install logic and remove the extra mkdir -p step that was only needed for
the cross-filesystem move.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/install_test.sh (1)

312-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated invocation logic instead of extending run().

This block re-implements run()'s env -i ... "$SH" "$installer" invocation just to add $H1/bin to PATH, and unlike run() it doesn't capture rc. Consider adding an optional PATH-prefix parameter to run() to cover this case and keep a single source of truth for the invocation.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/install_test.sh` around lines 312 - 320, The PATH advisory test block
duplicates the shell invocation logic already centralized in run(), instead of
extending that helper. Update run() to accept an optional PATH-prefix argument
so it can prepend $H1/bin when needed, keep the env -i and "$SH" "$installer"
invocation in one place, and preserve rc capture there for callers like this
PATH advisory check.

.github/workflows/release.yml (1)

520-530: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Correctly ordered before the channel update; consider env: indirection for the interpolated output.

zizmor flags the direct ${{ steps.release_info.outputs.short_sha }} expansion inside the run: block as a template-injection pattern. The value here is a deterministic hex git rev-parse --short output, so it isn't attacker-controllable — but routing GitHub Action outputs through an env: var before use in shell avoids the general anti-pattern (and matches the same opportunity at line 518, already pre-existing).

♻️ Optional hardening

             - name: Stage the installer script into the versioned folder
               run: |
+                  SHORT_SHA="$SHORT_SHA"
                   gcloud storage cp \
                     --cache-control="public, max-age=31536000, immutable" \
                     scripts/install.sh \
-                    "gs://minimal-one/versions/${{ steps.release_info.outputs.short_sha }}/install.sh"
+                    "gs://minimal-one/versions/${SHORT_SHA}/install.sh"
+              env:
+                  SHORT_SHA: ${{ steps.release_info.outputs.short_sha }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release.yml around lines 520 - 530, The installer staging
step uses a direct GitHub Actions expression inside the shell command, which
zizmor flags as a template-injection pattern. Update the release workflow job
that stages the installer script by moving the
`steps.release_info.outputs.short_sha` value into an `env:` variable and
reference that variable inside the `run:` block, keeping the `gcloud storage cp`
step and its ordering unchanged. Use the existing `release_info` output and the
installer staging step as the anchors when making the change.

Source: Linters/SAST tools

scripts/install_test.sh (1)

179-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated run() body for the PATH-suppression case.

The advise_on block re-implements run()'s env -i invocation just to prepend $H1/bin to PATH. Consider adding an optional PATH-prefix parameter to run() to avoid the duplication.

Also applies to: 312-320

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/install_test.sh` around lines 179 - 196, The PATH-suppression case
duplicates the same env -i launcher logic inside run(), so update run() to
accept an optional PATH-prefix argument and build PATH from it instead of
re-implementing the invocation in the advise_on path. Keep the existing setup in
run() (env -i, HOME, MINIMAL_* variables, STUB_UNAME_* and "$SH" "$installer")
and only parameterize the PATH value so both callers use the same function body.