Skip to content

Document binary and source installation#78

Merged
cosentinode merged 22 commits into
developfrom
issue-64-installation-docs
Jul 17, 2026
Merged

Document binary and source installation#78
cosentinode merged 22 commits into
developfrom
issue-64-installation-docs

Conversation

@cosentinode

Copy link
Copy Markdown
Owner

Summary

  • document checksum-verified installation for all four release artifacts
  • document runtime prerequisites, supported targets, PATH setup, and source builds
  • link installation guidance from README without claiming package-manager availability

Verification

  • cargo build --locked --release -p bitbygit
  • cargo run -p bitbygit -- --version (bitbygit 0.1.0)
  • git diff --check

Fixes #64

@cosentinode

Copy link
Copy Markdown
Owner Author

Independent review findings:

  1. High - failed checksum verification does not stop Unix installation (docs/installation.md:45-53, docs/installation.md:74-82). These are plain interactive command sequences with no set -e/pipefail or && gating. If sha256sum --check or shasum --check reports a mismatch, the shell still runs tar and install, so the documented checksum protection can install the unverified archive. Make extraction and installation conditional on successful downloads and checksum verification (ideally in a fail-fast subshell).

  2. Medium - source-build prerequisites are incomplete (docs/installation.md:121-128). Rust/Cargo and Git alone are not sufficient on clean systems: GNU targets need a native linker/toolchain, macOS needs Xcode Command Line Tools, and x86_64-pc-windows-msvc needs the Visual Studio MSVC build tools. The linked Rust page itself calls out the Windows requirement, but this document currently states the smaller list as sufficient. Document the platform build prerequisites or explicitly defer to a prerequisite page that covers them.

  3. Medium - the source path and final check do not verify the requested release version (docs/installation.md:121-128, docs/installation.md:156-162). git clone checks out the moving default develop branch, while bitbygit --version only prints whichever binary wins PATH lookup and is not compared with $VERSION. Once a release exists, the source path can therefore build unreleased code rather than the selected tag, and the final check cannot detect an old/wrong installation. Pin source builds to v$VERSION when building a release and state or assert the expected bitbygit $VERSION output.

  4. Medium - CI does not validate the installation documentation contract. The four archive names/layouts are duplicated between docs/installation.md and .github/workflows/release.yml, but .github/workflows/ci.yml and scripts/test-release-workflow.sh never reference the docs; neither the Unix/PowerShell install snippets nor Markdown links are checked. The PR checks only compile/test Rust on Linux, and the Release workflow has never run, so there is no automated evidence for the issue criterion that every artifact is installable with these commands. Add a lightweight docs contract/snippet/link check, with PowerShell/platform validation where needed, to prevent silent drift.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the posted installation review findings in 43b057f:

  • made Linux and macOS snippets fail fast with set -euo pipefail, and added terminating PowerShell behavior so checksum failures cannot reach extraction or installation
  • documented the native Linux, macOS, and Windows/MSVC source-build prerequisites
  • pinned source builds to v$VERSION / v$Version and validate both built and installed executables against the selected bitbygit <version> output using explicit paths
  • added scripts/test-installation-docs.sh to CI to derive artifact names/layout from the release workflow, syntax-check Bash and PowerShell snippets, exercise checksum-failure gating, verify source/version contracts, and check Markdown links and anchors
  • retained the actual four release artifact names and explicitly states that no tagged release or package-manager publication exists yet

Verification:

  • bash scripts/test-installation-docs.sh in mcr.microsoft.com/powershell:7.4-ubuntu-22.04 (pass)
  • bash scripts/test-release-workflow.sh (pass)
  • actionlint 1.7.12 (pass)
  • cargo fmt --all --check (pass)
  • cargo clippy --locked --workspace --all-targets -- -D warnings (pass)
  • cargo test --locked --workspace (pass)
  • cargo build --locked --workspace (pass)
  • cargo build --locked --release -p bitbygit (pass)
  • cargo run --locked -p bitbygit -- --version (bitbygit 0.1.0)
  • git diff --check (pass)

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review of current head 43b057f:

  1. Medium - the fail-fast setup permanently changes the callers interactive shell (docs/installation.md:40, docs/installation.md:74, docs/installation.md:109, docs/installation.md:162, docs/installation.md:187). The blocks are presented as commands to run in Bash/PowerShell, but top-level set -euo pipefail and $ErrorActionPreference = "Stop" remain enabled after a successful install. In Bash this can make a later unrelated command terminate the users shell, and in PowerShell it changes subsequent error handling. Scope the install body in a subshell/script block (while applying the intended PATH update explicitly) or preserve and restore the prior settings. The validator executes extracted snippets in child processes, so it cannot catch this session-level side effect.

  2. Medium - the validator still does not establish that any documented installation command succeeds (scripts/test-installation-docs.sh:38-51, scripts/test-installation-docs.sh:91-143). Artifact/layout/version checks are substring assertions, and the only executions force checksum failure before extraction. The PowerShell case also runs under Ubuntu and stops before Windows-only install/PATH behavior. The release workflow smoke tests archive contents, not these documentation commands, and that workflow has no runs yet. Consequently regressions in extraction, copy/install, version verification, or PATH updates can pass CI despite the issue criterion that every artifact be installable with the documented commands. Exercise valid synthetic archives/checksums through successful installation and version verification on applicable platform jobs, including Windows.

  3. Medium - the new required validator has an undeclared local PowerShell prerequisite (scripts/test-installation-docs.sh:57, scripts/test-installation-docs.sh:124, README.md:53-71, scripts/setup-dev.sh:4-7). It invokes pwsh unconditionally, but development prerequisites mention only Rust, Git, and optional gh, and the advertised setup script neither checks for PowerShell nor runs this validator. On the provided Linux development environment, bash scripts/test-installation-docs.sh fails at line 57 with pwsh: command not found; CI passes only because ubuntu-latest currently supplies it. Declare/provision a pinned prerequisite or split the checks into platform-native jobs, and include the required validator in the documented local verification path.

  4. Low - the stated Linux installer prerequisites are incomplete (docs/installation.md:36-54, docs/installation.md:159-175). The archive section says the commands require curl, sha256sum, and tar, but they also require external grep and install commands (and the source path uses install too). Minimal Linux installations can lack them. List the complete command prerequisites or state that standard coreutils/grep equivalents are required.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the fresh review concerns in 19860ca:

  • replaced top-level Bash options with isolated bash -euo pipefail child scripts and scoped PowerShell preferences in & { ... }, while retaining only successful PATH updates in the caller session
  • changed Unix validation to execute valid synthetic archives through checksum, extraction, installation, explicit-path version verification, and caller PATH checks; it also retains corrupt-checksum gates and verifies caller shell options remain unchanged
  • added a native Windows validator that builds and packages the pinned workspace binary, runs the extracted PowerShell archive command through successful and failed checksum cases, verifies install/version/PATH behavior, and restores the runner user PATH
  • split validation into Linux, macOS, and Windows CI jobs, removing the undeclared Linux pwsh dependency; the documented local/setup path now runs release and native Bash documentation validators
  • documented Linux installer dependencies on grep and GNU coreutils (sha256sum, mkdir, and install) and retained native linker/toolchain prerequisites
  • preserved release-version pinning, artifact names/layouts, checksum fail-fast behavior, and the statement that no tagged release or package-manager publication exists yet

Verification:

  • ./scripts/setup-dev.sh (pass: release validator, Linux docs validator, fmt, clippy, tests, workspace build)
  • INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh (pass for Intel and Apple silicon archives plus checksum failure)
  • PowerShell 7.4 parser validation for the validator and all documented PowerShell snippets (pass)
  • actionlint 1.7.12 (pass)
  • cargo build --locked --release -p bitbygit (pass)
  • cargo run --locked -p bitbygit -- --version (bitbygit 0.1.0)
  • git diff --check (pass)
  • PR CI: Rust, macOS installation docs, Windows installation docs, and GitGuardian all pass: https://github.com/cosentinode/bitbygit/actions/runs/29436668122

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh review of current head 19860ca found three concerns:

  1. High - the documented Rust 1.85 source-build minimum is false (docs/installation.md:148-149, .github/workflows/ci.yml:22-24,76-78). I built this exact head in rust:1.85-bookworm with cargo build --locked --release -p bitbygit; Rust 1.85.1 fails at crates/bitbygit-core/src/config.rs:40-41 because this let-chain is still unstable there. CI uses moving stable (currently much newer), so it cannot detect the unsupported advertised minimum. Either make the locked tree compile on 1.85 and add a pinned MSRV build, or raise the prerequisite/manifest consistently and test that exact minimum.

  2. Medium - the Windows source snippet still changes the caller's working directory (docs/installation.md:191-218). & { ... } isolates preference variables, but Set-Location bitbygit changes the runspace location and is not undone when the block succeeds or fails. PowerShell 7.4 reproduction leaves Get-Location at the child location after & { Set-Location ... }, unlike the Unix child-shell snippet and the stated intent to retain only PATH changes. Use Push-Location/Pop-Location in try/finally (or explicit paths), and execute this source block in validation; current source checks are only substring assertions (scripts/test-installation-docs.sh:181-184).

  3. Medium - the advertised local Windows validator writes the developer's real persistent user PATH (README.md:71-72, scripts/test-installation-docs.ps1:53,86,139-142, via docs/installation.md:136-140). The success test dot-sources the installation block, which calls SetEnvironmentVariable(..., "User") with a temporary directory, then restores a startup snapshot. Process termination can leave the temporary entry persisted, and the unconditional restore can overwrite a concurrent legitimate PATH update. CI's disposable runner does not make the documented local command safe. Validate this behavior without mutating the invoking user's persistent environment, or run it under an explicitly disposable user/environment.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the review concerns in b1f072d and 39e2ed3:

  • kept the repository policy and issue contract at Rust 1.85, replaced the unstable let-chain, and added an exact Rust 1.85.0 locked release build to CI
  • changed the Windows source instructions to use an absolute source directory and --manifest-path, so they never change the caller working directory
  • extended native Windows validation to execute the source block through success and post-clone build failure, asserting the caller location, error preference, install/version result, and failure gating
  • changed the validator to execute process-scope copies of both Windows blocks; it no longer reads, writes, snapshots, or restores persistent user PATH
  • preserved the documented tag/version checks, checksums, four artifact layouts, archive success/failure behavior, and cross-platform validators

Verification:

  • docker run ... rust:1.85-bookworm cargo build --locked --release -p bitbygit (pass)
  • ./scripts/setup-dev.sh (pass: release/docs validators, fmt, clippy, tests, workspace build)
  • INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh (pass for both macOS artifacts and checksum failure)
  • PowerShell 7.4 parser checks for the validator and documented snippets (pass)
  • actionlint 1.7.12 (pass)
  • cargo build --locked --release -p bitbygit and cargo run --locked -p bitbygit -- --version (bitbygit 0.1.0)
  • final PR CI (pass): Rust, Rust 1.85 source build, macOS installation docs, Windows installation docs, and GitGuardian: https://github.com/cosentinode/bitbygit/actions/runs/29438322974

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh independent review of current head 39e2ed3 found three concerns:

  1. Medium - source installs are not actually pinned to the published tag (docs/installation.md:168, docs/installation.md:196). git clone --branch "v$VERSION" accepts either a branch or a tag, and a same-named branch wins. I reproduced this with distinct refs/heads/v0.1.0 and refs/tags/v0.1.0: both documented clone forms checked out the branch as heads/v0.1.0. That permits moving/unreleased source with the same package version to pass the later --version check, contradicting lines 159-161. Use an unambiguous refs/tags/v... fetch/checkout and validate the resulting ref; the validators currently assert the ambiguous command and the Windows git mock ignores its arguments (scripts/test-installation-docs.sh:181-182, scripts/test-installation-docs.ps1:62-63,153-159).

  2. Medium - Rust 1.85 source-build support is only established for Linux (docs/installation.md:148-157, .github/workflows/ci.yml:15-29,73-98). The exact 1.85 job builds only the Linux host target. The Windows validator builds with moving stable, while the macOS validator does not compile the workspace at all. Since the documentation applies the 1.85 minimum to Linux, macOS, and Windows source builds, target-specific code/dependencies can exceed the advertised MSRV without PR CI noticing. Pin 1.85 in native platform build/check jobs (or otherwise check every documented source target at that minimum).

  3. Low - an absent Windows user PATH is persisted with an empty entry (docs/installation.md:136-140, docs/installation.md:214-218). When GetEnvironmentVariable(..., "User") returns null/empty, "$UserPath;$InstallDir" becomes ";$InstallDir". Both install methods therefore write a malformed leading empty PATH component for a clean user environment. Build the new value conditionally when no user PATH exists. The validator rewrites the operation to the always-populated process PATH (scripts/test-installation-docs.ps1:19-31,67-68), so this case is not covered.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed review concerns in f364f44:

  • source installs now create a fresh source directory, fetch only the full refs/tags/v<version> ref into the matching local tag, peel it to a commit, check it out detached, and verify HEAD; the Unix validator executes this against distinct same-named branch and annotated-tag objects, while the Windows mock requires the exact fetch/checkout/verification sequence
  • Rust 1.85.0 source-build CI now runs natively for all four supported targets: Linux x86-64, macOS Intel, macOS Apple silicon, and Windows x86-64
  • both Windows installation paths conditionally persist only $InstallDir when user PATH is null/empty; validation captures the would-be persistent value without touching user state and covers both empty and populated PATH behavior
  • retained fail-fast checksum handling, fresh-directory and caller-session isolation, explicit installed-path version checks, artifact/layout contracts, and link validation

Verification:

  • TMPDIR=/home/adrian/.cache/bitbygit-test-tmp bash scripts/test-installation-docs.sh (Linux archive success/failure and colliding branch/tag source install pass)
  • TMPDIR=/home/adrian/.cache/bitbygit-test-tmp INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh (both macOS archives, checksum failure, and source tag checkout pass)
  • TMPDIR=/home/adrian/.cache/bitbygit-test-tmp bash scripts/test-release-workflow.sh (pass)
  • actionlint 1.7.12 (pass)
  • PowerShell 7.4 parser checks for the validator and documented snippets (pass)
  • cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, cargo test --locked --workspace, and cargo build --locked --workspace (pass)
  • isolated rust:1.85-bookworm locked Linux release build (pass)
  • PR CI passes all eight checks, including all four native Rust 1.85 targets and the native Windows validator: https://github.com/cosentinode/bitbygit/actions/runs/29439964067

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review of current head f364f44 found two concerns:

  1. Medium - PATH installation is never validated through the command users are told to run (docs/installation.md:55-61, docs/installation.md:93-99, docs/installation.md:132-141, docs/installation.md:186-192, docs/installation.md:227-236, docs/installation.md:246-250). Every block runs the binary by explicit path before changing PATH; the final operation is the PATH mutation, despite the issue requiring bitbygit --version as the final check and the document claiming each block finishes with version validation. The validators likewise only check that the PATH string starts with the install directory and then invoke the explicit file (scripts/test-installation-docs.sh:125-127, scripts/test-installation-docs.ps1:114-123,222-233). A broken command-resolution/PATH integration can therefore pass all checks. Retain the explicit-path identity check, then execute bitbygit --version after the PATH update and assert the selected version on each platform.

  2. Low - the Windows process PATH update is malformed for an empty PATH and duplicates existing entries (docs/installation.md:141, docs/installation.md:236). Both blocks unconditionally assign "$InstallDir;$env:Path". An empty/null process PATH becomes a trailing-empty-entry value, while rerunning either installer when $InstallDir is already present prepends another duplicate each time. The Windows validator always starts from the runner's populated PATH and only checks entry zero (scripts/test-installation-docs.ps1:95-118,204-227), so neither edge is covered. Build the process value conditionally/idempotently, as is already done for user PATH, and test empty and already-present process PATH states.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed both review concerns in 05d3247 (plus updated-base MSRV compatibility in e5c7f32):

  • every archive and source installation retains the explicit installed-file identity check, then updates PATH, resolves bitbygit through that documented setup, requires the selected bitbygit <version>, and finishes by reporting bitbygit --version
  • Unix validation puts a wrong-version command earlier on PATH and proves all archive/source snippets resolve the newly installed binary
  • Windows process PATH now follows the same conditional/idempotent handling as user PATH; null/empty values become exactly the install directory and existing entries are not duplicated
  • native Windows validation exercises null/empty process and user PATH state for archive installation, already-present process and user entries for source installation, installed-command resolution, and selected-version output without touching persistent user state
  • after develop advanced, merged it in 53f6f1b and replaced its new let-chain with equivalent Rust 1.85-compatible control flow in e5c7f32, preserving the required native four-target MSRV matrix
  • exact release tags, four artifact names/layouts, checksum failure gates, caller-session isolation, and source tag collision checks remain intact

Verification:

  • TMPDIR=/home/adrian/.cache/bitbygit-test-tmp ./scripts/setup-dev.sh (pass: release/Linux docs validators, fmt, clippy, tests, workspace build)
  • TMPDIR=/home/adrian/.cache/bitbygit-test-tmp INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh (pass: Intel and Apple silicon archives, checksum failure, exact source tag, PATH resolution)
  • PowerShell 7.4 parser validation for the validator and documented snippets (pass)
  • actionlint 1.7.12 (pass)
  • isolated rust:1.85-bookworm locked Linux release build (pass)
  • git diff --check (pass)
  • final PR CI passes all eight checks, including native Windows installation validation and all four Rust 1.85 targets: https://github.com/cosentinode/bitbygit/actions/runs/29442098350

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review of current head e5c7f32 found one concern:

  1. Medium - selecting a different release version makes every Unix installer fail its final PATH check (docs/installation.md:31-32,42,62-65,82,106-109,187,212-215). The document tells users to set VERSION to the desired release, but that variable exists only inside the bash -euo pipefail child created by the heredoc. After the child exits, each caller-side check compares against the literal bitbygit 0.1.0. When v0.2.0 (or any later version) is selected by changing the advertised VERSION=... assignment, checksum, extraction, build, and explicit-path validation can all succeed, then the documented final check incorrectly rejects the installed binary. Keep the selected version available to the caller-side check, or perform the PATH mutation/check in a scoped design that uses the same version value. Add validation with a selected version distinct from the workspace/example version; the current validator only builds assets and assertions around workspace_version (scripts/test-installation-docs.sh:32-38,67-69,131-133,209-242), so it preserves rather than detects this defect.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the Unix selected-version PATH concern in 09a4d25.

  • The Linux, macOS, and Unix source snippets now retain VERSION in the caller and pass it into the scoped installer shell, so explicit-path and final PATH-resolved checks use the same selected release dynamically.
  • The Unix validator now changes the documented selection to 2.3.4 (distinct from workspace/example 0.1.0) and builds archive, checksum, exact-tag, colliding-branch, explicit-path, and PATH-resolution fixtures around that selected value.

Verification:

  • TMPDIR="$PWD/target" bash scripts/test-installation-docs.sh (passed on Linux)
  • TMPDIR="$PWD/target" INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh (passed for Intel and Apple silicon archive paths)
  • bash -n scripts/test-installation-docs.sh
  • cargo fmt --all --check
  • git diff --check

The PowerShell validator was not run locally because pwsh is unavailable; Windows documentation and validation code are unchanged, and the PR CI Windows job will provide that platform check.

@cosentinode

Copy link
Copy Markdown
Owner Author

CI follow-up for 09a4d25: all 8 PR checks pass, including Linux/macOS installation validation, Windows installation docs, all four Rust 1.85 platform builds, and GitGuardian.

@cosentinode

Copy link
Copy Markdown
Owner Author

Concern: Windows selected-version behavior is not actually tested. scripts/test-installation-docs.ps1:34-101 derives the validator version from Cargo.toml, executes the PowerShell blocks unchanged, and builds the same workspace-version binary. Since both the docs and binary are currently 0.1.0, a regression that hardcodes 0.1.0 in the archive/source version checks would still pass even though selecting another published version would fail. The Bash validator avoids this by rewriting the snippets to synthetic 2.3.4; the Windows archive and source paths need an equivalent version-decoupled fixture/check.

@cosentinode

Copy link
Copy Markdown
Owner Author

Resolved the Windows selected-version validation concern in f106e93.

The PowerShell validator now rewrites both extracted Windows snippets to synthetic version 2.3.4, packages a native fixture executable that reports that version, and uses the same fixture for the mocked source build. A hardcoded 0.1.0 check therefore fails. The real workspace Windows binary is still built and executed separately against the Cargo workspace version.

Verification:

  • bash scripts/test-installation-docs.sh
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • git diff --check
  • PR CI: all checks passed, including Windows installation docs

@cosentinode

Copy link
Copy Markdown
Owner Author

Concern: persistent Windows PATH order is not actually validated (docs/installation.md:148-160, repeated at 256-268; validator coverage at scripts/test-installation-docs.ps1:129-163). Both Windows snippets append $InstallDir to a non-empty user PATH, but prepend it only to the current process PATH. If an older bitbygit.exe is already in an earlier user-PATH directory, the immediate final check passes because of the temporary prepend while a new shell still resolves the old binary. If $InstallDir already exists later in PATH, the snippets do not move it and may instead fail the current check. The archive validator starts with empty process/user PATH, and the source case starts with the install directory already first, so neither exercises this upgrade/shadowing case. Please make the persisted ordering match the verified process ordering and add a conflicting-command/new-shell-path fixture so bitbygit --version proves the selected installed version remains resolved after restart.

@cosentinode

Copy link
Copy Markdown
Owner Author

Resolved the persistent Windows PATH ordering concern in e565b4b and 3f0d40c.

  • Both archive and source snippets now remove empty entries and all existing occurrences of the selected install directory, then prepend exactly one install entry to both persistent user PATH and the current process PATH. This makes reruns idempotent and ensures restart behavior matches the immediate verification.
  • The native Windows validator starts with an older workspace-version bitbygit.exe first on PATH and duplicated selected-install entries later, proves the old version initially resolves, runs the synthetic selected-version 2.3.4 install, then proves both the current process and a simulated new shell built from captured persistent user PATH resolve the selected installed binary and version.
  • Existing empty process/user PATH coverage remains in the archive case, and checksum failure, source tag pinning, caller state, selected-version, and explicit-path guarantees remain covered.

Verification:

  • TMPDIR="$PWD/target" bash scripts/test-installation-docs.sh
  • TMPDIR="$PWD/target" INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh
  • TMPDIR="$PWD/target" bash scripts/test-release-workflow.sh
  • PowerShell 7.4 parser validation for the validator and all documented PowerShell snippets
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • git diff --check
  • PR CI: all eight checks pass, including native Windows installation docs: https://github.com/cosentinode/bitbygit/actions/runs/29446044454

@cosentinode

Copy link
Copy Markdown
Owner Author

Concern: persistent Windows PATH precedence is not actually validated

  • docs/installation.md:148-166 (and the source equivalent at :258-272) prepends the install directory to the user PATH, which makes it first only among user entries. Windows constructs a normal new user process with machine PATH entries ahead of user PATH entries, so an older bitbygit.exe in a machine-level directory will still shadow this installation despite the claim that new shells will use it. The current process check passes only because the snippet separately prepends $InstallDir to $env:Path.
  • scripts/test-installation-docs.ps1:253-300 misses that case: it places the old executable in both simulated process/user PATH data, then simulates a new shell with $env:Path = $CapturedUserPath, omitting a machine-PATH prefix entirely. This will certify persistence even when real new shells resolve the old machine-level binary.

Please make the persistent behavior/claim safe for machine-level shadowing (for example, detect and report it) and have the validator compose machine + user PATH in Windows order with an older machine-level fixture.

@cosentinode

Copy link
Copy Markdown
Owner Author

Resolved the machine-level Windows PATH precedence concern in 5f17c6b.

  • Both Windows installation blocks now inspect machine PATH before persisting user PATH. If a machine directory contains bitbygit.exe, they stop before any persistent PATH write and explain that user PATH cannot override it in new shells; the selected installed binary remains available through the documented explicit-path invocation.
  • Successful PATH updates remain idempotent and continue to prioritize one install-directory entry in both user and current-process PATH.
  • The Windows validator now intercepts machine and user PATH reads through process-scoped fixtures, composes simulated new-shell PATH as machine entries followed by user entries, and verifies archive/source resolution.
  • A distinct older machine-level bitbygit.exe fixture proves Windows would resolve the old version first, that installation rejects the unsafe persistent setup, that process/user PATH remain unchanged, and that the selected binary is still usable explicitly. The validator never reads or writes persistent validator PATH state.

Verification:

  • TMPDIR="$PWD/target" ./scripts/setup-dev.sh (release/Linux docs validator, format, clippy; its first workspace test run hit the existing intermittent upstream_invalid_repo_is_error test)
  • cargo test --locked -p bitbygit-git upstream_invalid_repo_is_error (pass on immediate isolated rerun)
  • cargo test --locked --workspace (pass on full rerun)
  • cargo build --locked --workspace
  • TMPDIR="$PWD/target" INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh
  • PowerShell 7.4 parser validation for scripts/test-installation-docs.ps1 and all three documented PowerShell blocks
  • git diff --check
  • PR CI: all eight checks pass, including native Windows installation docs: https://github.com/cosentinode/bitbygit/actions/runs/29447360510

@cosentinode

Copy link
Copy Markdown
Owner Author

Medium - fixed working-directory state makes installation unsafe and breaks the documented retry path. The archive snippets download SHA256SUMS and the archive into the callers current directory and extract there (docs/installation.md:48-51,92-95,131-138), so they reuse generic names and leave package trees that can collide with unrelated files. More concretely, both Windows blocks detect a machine-PATH conflict only after extraction/build and installation, then instruct the user to remove the conflict and rerun (docs/installation.md:155-156,286-287). That rerun from the same directory fails: Expand-Archive encounters the package files left by the first archive attempt, while the source block fails when New-Item encounters the existing bitbygit directory (docs/installation.md:252). The Unix source block has the same failed-build/retry problem at mkdir bitbygit (docs/installation.md:212). The validators always execute in fresh one-shot directories and do not cover a second invocation (scripts/test-installation-docs.sh:118-120,230-245; scripts/test-installation-docs.ps1:143-162,300-330). Stage downloads/builds in a unique temporary directory with cleanup, or otherwise make retries non-destructive and test the instructed rerun.

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed the fixed-working-directory/retry concern from #78 (comment) in commit 41e1a75.

  • All Bash archive/source snippets now stage in a per-invocation mktemp -d directory and clean it with an EXIT trap that preserves the command status.
  • Both PowerShell snippets now stage in a cryptographically random temporary directory and remove it in finally.
  • Archive checksums are still validated before extraction; caller shell options, PATH failure behavior, and working directory remain unchanged.
  • Validators now protect pre-existing fixed-name archive/checksum/package/source symlinks and their targets, execute successful installs twice, retry failed source builds, retry the documented Windows machine-PATH conflict flow, and assert temporary work is cleaned on success and failure.

Verification:

  • bash scripts/test-installation-docs.sh
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • cargo build --locked --workspace
  • git diff --check
  • CI run https://github.com/cosentinode/bitbygit/actions/runs/29449011131: all jobs passed, including Linux, macOS, and Windows installation documentation validators.

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh independent review of current head 41e1a75 found three concerns:

  1. Medium - the final version checks do not reliably validate PATH command resolution (docs/installation.md:70-76,123-129,189-194,268-274,335-340). Bash functions and PowerShell functions/aliases take precedence over executables found through PATH. A pre-existing bitbygit function that prints bitbygit <selected-version> therefore lets these blocks pass even though the bare command never resolved the installed file; a conflicting function instead makes the Windows blocks fail only after persistent user PATH and process PATH were already changed. The document consequently overstates the guarantee at docs/installation.md:355-358. Resolve and compare the application path itself before the final bare invocation, and test higher-precedence command shadowing plus late-failure state preservation; current fixtures cover only an older executable earlier in PATH (scripts/test-installation-docs.sh:108-111,149-153; scripts/test-installation-docs.ps1:204-221,414-433).

  2. Low - cleanup failures mask the primary installation result on every platform (docs/installation.md:48-53,101-106,239-244,195-197,341-343). In each Bash EXIT trap, rm -rf runs under set -e, so if cleanup fails the following exit "${status}" is never reached and the original failure status is lost. In PowerShell, a terminating Remove-Item error from finally similarly replaces the exception raised by download, checksum, build, or validation. This makes the actual failure difficult to diagnose, and after a successful Windows PATH mutation it reports failure without distinguishing that installation state was already committed. Preserve the original status/exception while reporting cleanup failure separately, and add forced-cleanup-failure coverage; the validators currently check only successful removal (scripts/test-installation-docs.sh:154-156,219-220,282-293; scripts/test-installation-docs.ps1:186-188,257,315,403-405,466,481).

  3. Low - successful Unix retries are not PATH-idempotent (docs/installation.md:70,123,268). Each rerun prepends another identical $HOME/.local/bin entry. The validator executes archive and source blocks twice but asserts only that the first entry is correct, so this accumulation passes (scripts/test-installation-docs.sh:142-149,286-293). Deduplicate or avoid prepending when the entry is already present, and assert exactly one entry after retries.

@cosentinode

Copy link
Copy Markdown
Owner Author

Resolved all concerns from #78 (review) in b6348df with the Windows assertion follow-up in af7ad5d.

  • Unix snippets now prioritize and deduplicate the install directory, resolve applications with type -P, compare file identity with -ef, and invoke through command so aliases/functions cannot satisfy the check.
  • PowerShell snippets resolve only Application commands, compare canonical installed/resolved paths case-insensitively, invoke that resolved path, validate before persisting user PATH, and restore process PATH on validation failure.
  • Bash and PowerShell cleanup now report cleanup errors separately while preserving the original installation status/exception; cleanup failure after success is a warning rather than a false installation failure.
  • Validators exercise alias and function shadowing, forced cleanup failure with primary-error preservation, repeated installs with exactly one Unix PATH entry, and the existing archive/source retry guarantees.

Verification:

  • TMPDIR="$PWD/target" bash scripts/test-installation-docs.sh
  • TMPDIR="$PWD/target" INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh
  • TMPDIR="$PWD/target" bash scripts/test-release-workflow.sh
  • PowerShell 7.4 parser validation for the native validator and all documented PowerShell snippets
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked -p bitbygit-git upstream_invalid_repo_is_error
  • cargo test --locked --workspace
  • cargo build --locked --workspace
  • git diff --check
  • All eight PR checks pass, including native Windows installation validation: https://github.com/cosentinode/bitbygit/actions/runs/29452143962

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review of current head af7ad5d found two concerns:

  1. Medium - the Windows new-shell guard misses non-.exe applications that can still shadow the install (docs/installation.md:196-205, repeated at :371-380; coverage at scripts/test-installation-docs.ps1:249-303). PowerShell's Application resolution considers executable extensions from PATHEXT, but $MachineCommand checks only bitbygit.exe. If an earlier machine-PATH directory contains bitbygit.cmd, .bat, or .com, the installer accepts and persists the user PATH because no .exe was found; the immediate check passes only because $InstallDir is prepended to the current process, while a normal new shell (machine PATH before user PATH) resolves the machine-level wrapper instead. Inspect applicable PATHEXT candidates (or resolve against the composed new-shell PATH) and add a non-.exe machine fixture; the validator currently covers only an older bitbygit.exe.

  2. Low - Unix PATH deduplication drops a valid trailing empty entry (docs/installation.md:73-79, :139-145, :313-319; validator assertions at scripts/test-installation-docs.sh:164-170,357-362). In Bash, IFS=: read -r -a path_entries <<< "$PATH" does not retain a trailing empty field, so a valid PATH such as /usr/bin: becomes $HOME/.local/bin:/usr/bin, silently removing the caller's current-directory search entry. The retry tests use only an ordinary populated PATH and count the install directory, so this side effect passes. Preserve all non-install entries, including leading/trailing empty components, and add an edge-PATH assertion.

@cosentinode

Copy link
Copy Markdown
Owner Author

Resolved both concerns from #78 (comment) in bb23248.

  • Both Windows installation guards now derive the standard application candidates enabled by PATHEXT case-insensitively and inspect .exe, .com, .bat, and .cmd in every machine PATH directory before persisting user PATH. The native validator uses mixed-case PATHEXT and independently proves rejection for all four machine-level extensions.
  • All three Unix PATH updates now preserve leading, trailing, and interior empty components exactly, remove every existing install-directory component, and prepend one selected install entry. Archive and source regressions start with all three empty-component positions plus duplicate install entries and assert the exact PATH after repeated installation.

Verification:

  • TMPDIR="$PWD/target" bash scripts/test-installation-docs.sh
  • TMPDIR="$PWD/target" INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh
  • TMPDIR="$PWD/target" bash scripts/test-release-workflow.sh
  • PowerShell 7.4 parser validation for the native validator and all documented PowerShell snippets
  • actionlint
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked -p bitbygit-git upstream_invalid_repo_is_error (pass after the known intermittent failure in the first full setup run)
  • cargo test --locked --workspace
  • cargo build --locked --workspace
  • git diff --check
  • All eight PR checks pass, including native Windows installation validation: https://github.com/cosentinode/bitbygit/actions/runs/29453045728

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review of current head bb232487 found one concern:

  1. Medium - the Windows machine-PATH guard ignores executable extensions that are actually enabled in PATHEXT (docs/installation.md:200-214, repeated at :384-398). The blocks parse $env:PATHEXT but then retain only .exe, .com, .bat, and .cmd. PowerShell treats every extension listed in PATHEXT as executable, so an earlier machine directory containing (for example) an enabled/associated bitbygit.vbs or a custom executable extension can still shadow the user-level bitbygit.exe in a new shell. The immediate verification passes because $InstallDir is separately prepended to the current process PATH, after which the blocks persist the user PATH and claim new shells will use it (docs/installation.md:253-265). The validator masks this case by replacing PATHEXT with exactly those four extensions and iterating only them (scripts/test-installation-docs.ps1:151, :278). Inspect every non-empty extension enabled in PATHEXT (or resolve against a composed machine-plus-user new-shell PATH), and add a candidate outside the four-value subset to the native Windows test.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed the PATHEXT machine-PATH shadow concern in commit 281d4d2.

  • Both Windows installation snippets now collect every non-empty PATHEXT entry into an ordinal-ignore-case HashSet, rather than filtering to .exe, .com, .bat, and .cmd.
  • Candidate checks remain literal-path checks, so custom entries are handled without wildcard expansion.
  • The Windows validator now exercises mixed-case custom .VbS and .BiTbYgIt-Test entries, a case-duplicate entry, and blank entries.
  • Each rejected custom-extension conflict asserts no captured user PATH, process PATH, or process PATHEXT mutation, while preserving explicit access to the installed binary.

Verification:

  • bash scripts/test-release-workflow.sh
  • bash scripts/test-installation-docs.sh
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • cargo build --locked --workspace
  • git diff --check
  • All PR checks passed, including native Windows installation docs, macOS installation docs, Rust, all Rust 1.85 target builds, and GitGuardian.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review of current head 281d4d2 found two concerns:

  1. Medium - the Unix checksum lookup can validate a different file than the archive being installed (docs/installation.md:61, docs/installation.md:129). grep -F " ${ARCHIVE}" SHA256SUMS does not require the manifest filename to end after ${ARCHIVE}. If the exact archive entry is absent but a line for (for example) ${ARCHIVE}.sig exists, sha256sum --check - validates that sidecar and returns success, after which the block extracts the unverified archive. I reproduced this with different archive and archive.sig contents: the documented pipeline returned success while printing only archive.sig: OK. The current release workflow emits exact entries, but a future sidecar or malformed manifest silently defeats the documented integrity guarantee. Match/parse the exact filename and add a missing-exact-entry plus prefix-collision fixture; the current failure test only supplies an exact entry with a bad digest (scripts/test-installation-docs.sh:207-247).

  2. Low - an unset Unix PATH loses Bash default command-resolution semantics (docs/installation.md:75-81, repeated at :143-149 and :329-335). ${PATH-} intentionally maps both unset and empty PATH to the same value. Bash can resolve standard commands through its implementation default when PATH is unset, but after any of these blocks it exports only $HOME/.local/bin:; standard commands such as ls, git, and curl then stop resolving unless present in the current directory. The validators always begin with a populated PATH (scripts/test-installation-docs.sh:139-141, :327-330), so this side effect is not covered. Distinguish unset from explicitly empty state and retain the default search path when adding the install directory, with an unset-PATH regression test.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fixed both concerns in 98c0c3e:

  • Unix/macOS checksum selection now parses the manifest and only emits a record whose complete filename field exactly equals the selected archive. The validator includes a valid checksum for an ambient similarly named .sig file while omitting the exact archive entry and confirms extraction/installation never runs.
  • Unix/macOS PATH setup now distinguishes unset from explicitly empty/set PATH. An unset PATH is initialized from command -p getconf PATH before prepending the install directory; existing set values and empty entries retain the prior preservation/deduplication behavior. Archive and source validator paths now cover originally unset PATH and standard command lookup.

Verification:

  • bash scripts/test-installation-docs.sh
  • INSTALLATION_DOCS_TEST_OS=Darwin bash scripts/test-installation-docs.sh
  • ./scripts/setup-dev.sh
  • cargo build --locked --release -p bitbygit
  • cargo run -p bitbygit -- --version
  • git diff --check

No unresolved risks identified. Not merged.

@cosentinode cosentinode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh independent review of current head 98c0c3e found two concerns:

  1. Medium - source installs fail when Cargo output is configured outside the default target directory (docs/installation.md:319-327, repeated for Windows at :382-394). Both snippets run Cargo and then unconditionally execute/copy $SourceDir/target/release/bitbygit[.exe]. CARGO_TARGET_DIR and the standard Cargo build.target-dir / build.target settings legitimately move that artifact. I reproduced this head with CARGO_TARGET_DIR=/tmp/custom cargo build --locked --release -p bitbygit: the build succeeded, /tmp/custom/release/bitbygit existed, and the documented target/release/bitbygit path did not. Pin a private target directory (and native target, if needed) in the snippets or derive the actual Cargo output path. The validators mask this because they pre-place a fixture at the assumed path and mock cargo without validating arguments or output (scripts/test-installation-docs.sh:343-377; scripts/test-installation-docs.ps1:391-430); the separate CI builds do not execute these source snippets with real Cargo.

  2. Low - Unix PATH deduplication is still not lossless for newline-containing entries (docs/installation.md:79-84, repeated at :151-156 and :340-345). Unix environment values and path components may legally contain newlines, but read -r -a consumes only the first line. For PATH=$'/one:/two\ncontinued:/three:', the documented loop produces only <install>:/one, dropping the rest of the caller PATH. Parse by the colon delimiter without a line-oriented read, and add this case alongside the current empty-entry assertions (scripts/test-installation-docs.sh:146-179,383-423).

@cosentinode

Copy link
Copy Markdown
Owner Author

Addressed both concerns in fb82aa5:

  • Source builds now use an isolated cargo-target directory, explicitly override configured Cargo targets with the native rustc host tuple, and verify/install the exact resulting binary on Unix and Windows. The regressions set conflicting CARGO_TARGET_DIR and CARGO_BUILD_TARGET values and require mocked Cargo to create output only at the requested path.
  • Unix PATH deduplication now parses directly on : with shell parameter expansion, preserving newline-containing entries and every leading, interior, and trailing empty entry. Linux/macOS Bash regressions include a newline-bearing entry and multiple empty entries.

Verification:

  • bash scripts/test-installation-docs.sh
  • real release build with conflicting Cargo output settings and exact binary-path/version verification
  • cargo fmt --all --check
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • cargo test --locked --workspace
  • bash scripts/test-release-workflow.sh
  • git diff --check
  • PowerShell validator and documented snippet parser validation via PowerShell LTS container

The pushed PR checks will execute the full PowerShell regression on Windows and the Bash regression on macOS.

@cosentinode

Copy link
Copy Markdown
Owner Author

Follow-up 89b6bc5 fixes the macOS validator-only exit 141: host-target parsing now consumes the complete rustc -vV stream instead of closing the pipeline early under pipefail. bash scripts/test-installation-docs.sh, bash -n, and git diff --check pass after the fix.

@cosentinode

Copy link
Copy Markdown
Owner Author

Fresh independent review: zero concerns.

Reviewed issue #64, the final diff, release workflow contracts, installation snippets and validators, and the effective merge with current develop. Local verification passed: Linux installation docs validation, release workflow validation, full workspace tests, Rust 1.85 release build, final bitbygit --version, and git diff --check. GitHub platform checks are also green, including macOS and Windows installation validation and all four Rust 1.85 target builds.

@cosentinode
cosentinode merged commit 32c0cb8 into develop Jul 17, 2026
8 checks passed
@cosentinode
cosentinode deleted the issue-64-installation-docs branch July 17, 2026 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Phase 14] Document binary and source installation

1 participant