feat(bashinst): define spec for bash installer - #621
Conversation
📝 WalkthroughWalkthroughThis PR adds a new specification document for a POSIX ChangesInstaller Specification
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with 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.
Inline comments:
In `@docs/specs/07-spec-installer/07-spec-installer.md`:
- Around line 42-46: Update the installer portability contract in the spec to
list every runtime dependency the script actually uses. In the section that
describes the POSIX sh installer, revise the dependency list to include mktemp
and dirname, or explicitly document a fallback path if either is unavailable, so
the claims in the installer spec stay consistent with the tools referenced later
in the document.
- Around line 91-94: Make the architecture naming canonical across the installer
spec and examples. Update the manifest example and the field-matching rules that
still reference x86_64 so they use the same normalized arch value as R1.3, and
ensure the parser/spec logic in the installer sections matches on amd64 rather
than mixing aliases. Reference the R1.3 host-platform normalization rules and
the manifest matching rules in the spec so Linux x86_64 entries resolve
correctly end-to-end.
- Around line 202-218: The skip logic for installed components currently treats
a hash match as sufficient, which can leave `bin` artifacts non-executable on
reruns. Update the installer behavior in the component handling path so the
“already installed” check also verifies the executable mode for `bin`
destinations, or repairs the mode before skipping. Use the existing install flow
around the `R5.1`/`R5.4` behavior and the `bin` prefix token handling to locate
the logic, and ensure a hash-matching binary is not left in a broken state.
- Around line 131-135: The manifest example fence is missing a language tag,
which triggers markdownlint. Update the fenced code block in the installer spec
example to use a text-friendly language tag such as text or sh, keeping the
existing manifest content unchanged and preserving the formatting around the
example table.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ee287a9-c952-4eb0-8681-e7507c3ff52c
📒 Files selected for processing (1)
docs/specs/07-spec-installer/07-spec-installer.md
| **R1.3** — The host platform is derived from `uname -s` (→ `linux`/`darwin`) and | ||
| `uname -m`, with arch normalized so the same CPU has one name across platforms: | ||
| `amd64|x86_64 → amd64`, `arm64|aarch64 → arm64`. An unrecognized OS or arch | ||
| exits with a clear error. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the arch names canonical end-to-end.
R1.3 normalizes x86_64/amd64 to amd64, but the manifest example and field-matching rules still use x86_64. As written, Linux x86_64 rows will never match unless the manifest and parser agree on one canonical arch name.
Proposed fix
- minimald linux x86_64 1.4.2 3a7b… file bin/minimald minimald/1.4.2/minimald-linux-x86_64
+ minimald linux amd64 1.4.2 3a7b… file bin/minimald minimald/1.4.2/minimald-linux-amd64Also applies to: 131-145
🤖 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 `@docs/specs/07-spec-installer/07-spec-installer.md` around lines 91 - 94, Make
the architecture naming canonical across the installer spec and examples. Update
the manifest example and the field-matching rules that still reference x86_64 so
they use the same normalized arch value as R1.3, and ensure the parser/spec
logic in the installer sections matches on amd64 rather than mixing aliases.
Reference the R1.3 host-platform normalization rules and the manifest matching
rules in the spec so Linux x86_64 entries resolve correctly end-to-end.
| ``` | ||
| # component os arch version sha256 kind dest src | ||
| minimald linux x86_64 1.4.2 3a7b… file bin/minimald minimald/1.4.2/minimald-linux-x86_64 | ||
| minimald darwin arm64 1.4.2 2c26… file bin/minimald minimald/1.4.2/minimald-darwin-arm64 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the manifest example fence.
Markdownlint flags this fence, so text (or sh) will keep the spec lint-clean.
Proposed fix
- ```
+ ```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| # component os arch version sha256 kind dest src | |
| minimald linux x86_64 1.4.2 3a7b… file bin/minimald minimald/1.4.2/minimald-linux-x86_64 | |
| minimald darwin arm64 1.4.2 2c26… file bin/minimald minimald/1.4.2/minimald-darwin-arm64 | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 131-131: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/specs/07-spec-installer/07-spec-installer.md` around lines 131 - 135,
The manifest example fence is missing a language tag, which triggers
markdownlint. Update the fenced code block in the installer spec example to use
a text-friendly language tag such as text or sh, keeping the existing manifest
content unchanged and preserving the formatting around the example table.
Source: Linters/SAST tools
| **R5.1** — For each applicable component, if the resolved destination file | ||
| already exists **and** its on-disk SHA-256 equals the manifest `sha256`, the | ||
| component is skipped with no download. The on-disk file — not any recorded | ||
| state — is the source of truth for "already installed". | ||
|
|
||
| **R5.2** — Otherwise the artifact is downloaded from `<BUCKET>/<src>` to a | ||
| temp sibling **in the destination directory** (`<dest>.tmp.$$`), so the final | ||
| rename is same-filesystem and atomic. | ||
|
|
||
| **R5.3** — The downloaded temp file's SHA-256 is compared to the manifest | ||
| `sha256`; on mismatch the temp file is removed and the run exits non-zero | ||
| naming the component. The partially-written file is never installed. | ||
|
|
||
| **R5.4** — For files installed to a prefix token of `bin`, the temp file is | ||
| `chmod +x`ed **before** the rename, so it appears already-executable at its | ||
| final path. The install is completed with `mv -f <dest>.tmp.$$ <dest>` | ||
| (atomic within one filesystem). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don’t skip executable-bit repair on matching binaries.
A bin file can hash-match yet still be non-executable, so reruns would silently leave a broken install in place. Make the skip condition include mode, or normalize the existing file before continuing.
Proposed fix
- if [ -f "$target" ] && [ "$(sha256 "$target")" = "$want" ]; then
- continue # up to date, no download
- fi
+ if [ -f "$target" ] && [ "$(sha256 "$target")" = "$want" ]; then
+ if [ "$kind" != bin ] || [ -x "$target" ]; then
+ continue # up to date, no download
+ fi
+ fi🤖 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 `@docs/specs/07-spec-installer/07-spec-installer.md` around lines 202 - 218,
The skip logic for installed components currently treats a hash match as
sufficient, which can leave `bin` artifacts non-executable on reruns. Update the
installer behavior in the component handling path so the “already installed”
check also verifies the executable mode for `bin` destinations, or repairs the
mode before skipping. Use the existing install flow around the `R5.1`/`R5.4`
behavior and the `bin` prefix token handling to locate the logic, and ensure a
hash-matching binary is not left in a broken state.
| maximally portable and make as few assumptions about the host as possible. | ||
|
|
||
| Distribution artifacts are published to a public GCS bucket. Mutable pointer | ||
| files at the bucket root (`stable`, `nightly`, …) each contain a single version |
There was a problem hiding this comment.
do we want to use nightly vs the stable/main, next and unstable we are using for pkgs?
There was a problem hiding this comment.
Down for whatever, happy to do unstable.
There was a problem hiding this comment.
if you wouldn't mind, I think that will create less confusion. "next" in pkgs is for short-lived review cycle we can forego for this one.
0db1220 to
f686cb4
Compare
f686cb4 to
551518b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@docs/specs/07-spec-installer/07-spec-installer.md`:
- Around line 76-85: The wget path currently does not actually enforce the
no-downgrade guarantee claimed in the installer spec. Update the `R1.1`
downloader selection text and the `wget` wrapper so it either validates
redirects explicitly before downloading or removes the no-downgrade claim from
the wget case; keep the `curl` and `wget` behavior descriptions aligned with
what `curl --proto-redir` and `wget --https-only` באמת guarantee.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72a9b9db-bd18-48f3-8293-7830df5323d7
📒 Files selected for processing (1)
docs/specs/07-spec-installer/07-spec-installer.md
| **R1.1** — The script selects a downloader at runtime: prefer `curl`, else | ||
| `wget`, else exit with a clear error. Both wrappers enforce HTTPS and a TLS 1.2 | ||
| floor and refuse redirect downgrades: | ||
|
|
||
| ```sh | ||
| # curl | ||
| curl --proto '=https' --proto-redir '=https' --tlsv1.2 -fsSL "$url" -o "$out" | ||
| # wget | ||
| wget --https-only --secure-protocol=TLSv1_2 -qO "$out" "$url" | ||
| ``` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the spec around the cited lines.
sed -n '60,100p' docs/specs/07-spec-installer/07-spec-installer.md
# Search for any other mentions of wget/curl redirect handling in this spec.
rg -n "wget|curl|https-only|proto-redir|TLSv1_2|redirect" docs/specs/07-spec-installer/07-spec-installer.mdRepository: gominimal/minimal
Length of output: 2966
🌐 Web query:
GNU Wget manual --https-only recursive mode redirect downgrade protection
💡 Result:
In GNU Wget, the --https-only option is designed to restrict recursive downloads to HTTPS links only [1][2]. When this option is enabled, Wget will only follow links that use the HTTPS protocol during recursive retrieval [1][3]. Regarding redirect downgrade protection: The --https-only option specifically limits the links Wget follows recursively, but it does not inherently guarantee that Wget will abort or reject a redirect from an HTTPS URL to an HTTP URL if that redirection occurs for a request already in progress. While --https-only ensures that discovered links are HTTPS, it is primarily a filter for the recursion engine rather than a protocol-level enforcement mechanism for redirects [1][2]. If strict protection against protocol downgrades is required, relying solely on --https-only may be insufficient if a server issues a 3xx redirect from HTTPS to HTTP. Users seeking to prevent such behavior should be aware that Wget will typically follow redirects automatically (up to the --max-redirect limit) [4][5]. There is no native flag in the standard GNU Wget manual that explicitly disables "HTTPS to HTTP" redirects while allowing "HTTPS to HTTPS" redirects for a single request sequence. {https://www.gnu.org/software/wget/manual/html_node/HTTPS-_0028SSL_002fTLS_0029-Options.html, 1}, {https://www.gnu.org/software/wget/manual/html_node/HTTP-Options.html, 6}
Citations:
- 1: https://www.gnu.org/software/wget/manual/html_node/HTTPS-_0028SSL_002fTLS_0029-Options.html
- 2: https://man7.org/linux/man-pages/man1/wget.1.html
- 3: https://man.archlinux.org/man/extra/wget/wget.1.en
- 4: https://www.gnu.org/software/wget/manual/html_node/HTTP-Options.html
- 5: https://www.gnu.org/software/wget/manual/html_node/Wgetrc-Commands.html
Wget path doesn't satisfy the no-downgrade claim. --https-only only filters recursive link following, so this single-file installer fetch can still accept an HTTPS→HTTP redirect. Add explicit redirect validation for the wget path or limit the claim to the curl wrapper.
🤖 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 `@docs/specs/07-spec-installer/07-spec-installer.md` around lines 76 - 85, The
wget path currently does not actually enforce the no-downgrade guarantee claimed
in the installer spec. Update the `R1.1` downloader selection text and the
`wget` wrapper so it either validates redirects explicitly before downloading or
removes the no-downgrade claim from the wget case; keep the `curl` and `wget`
behavior descriptions aligned with what `curl --proto-redir` and `wget
--https-only` באמת guarantee.
Summary by CodeRabbit
shinstaller specification for a “curl … | sh” fallback flow.awk, cleanup behavior).