From 68fa6480105b41e8ffcc96206b6d8b7b28a715ce Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Thu, 17 Sep 2026 11:13:13 +0200 Subject: [PATCH 1/6] Close the loop: build, sanity-check, and publish for the first time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the last unimplemented pipeline stage from SPEC.md: PKGBUILD generation + makepkg (builder.rs), a post-build version sanity check (sanity.rs), and repo-add publishing (publisher.rs), wired into main.rs for both the tier 1-3 auto-publish path and a new tier 4-6 review queue (`pkgwatch review` / `pkgwatch review --approve`, persisted via state::{load,save,clear}_pending_version, tracked separately from last-published-version since approving one release isn't a standing auto-publish grant for future ones). Publishing targets an existing, already-registered local pacman repo (~/.local/share/pacman/custom, `[custom]` in /etc/pacman.conf) rather than one pkgwatch invents — found already in real use for a hand-packaged AppImage, which resolves SPEC's open question on where the repo lives without pkgwatch ever touching pacman.conf. Publishing stops at `repo-add`; actually installing/upgrading (`pacman -Syu`/`pacman -S`) is left to the operator, not run automatically. Getting a real second package (scaleway-cli, tier 4) through the new pipeline immediately surfaced a real gap: its pacman package is named `scaleway-cli` but the actual binary is `scw` (confirmed via `pacman -Ql` against the currently-installed extra package) — without a way to declare that, the build would install alongside extra's package under the wrong name instead of shadowing it. Added `Package::binary_name` (config.rs) to cover it. Every upstream-controlled string (version, asset name, download URL) is validated before it touches generated shell content in the PKGBUILD template — rejects anything containing a single quote or newline, since values are embedded in single-quoted bash strings. Verified for real, end to end: uv (tier 2) auto-built and published against the real astral-sh/uv release with no human step; scaleway-cli (tier 4) queued for review, then approved via `pkgwatch review scaleway-cli --approve`, which re-verified, built, and published it — confirmed the built package contains exactly usr/bin/scw. Both landed in the real custom repo's database. Left scaleway-cli's real-repo review pending rather than approving it myself: the tier 4-6 gate exists for a human judgment call, not the agent's. 69 tests, cargo make ci clean (fmt, clippy, complexity, coverage, audit). Co-Authored-By: Claude Sonnet 5 --- SPEC.md | 117 +++++++++--- packages.d/scaleway-cli.toml | 11 ++ packages.d/uv.toml | 4 + src/builder.rs | 359 +++++++++++++++++++++++++++++++++++ src/config.rs | 25 +++ src/fetcher.rs | 26 ++- src/hash.rs | 25 +++ src/main.rs | 317 ++++++++++++++++++++++++------- src/publisher.rs | 123 ++++++++++++ src/sanity.rs | 125 ++++++++++++ src/state.rs | 72 +++++++ src/verifier.rs | 12 +- 12 files changed, 1103 insertions(+), 113 deletions(-) create mode 100644 src/builder.rs create mode 100644 src/hash.rs create mode 100644 src/publisher.rs create mode 100644 src/sanity.rs diff --git a/SPEC.md b/SPEC.md index b5fb346..5e2f8c9 100644 --- a/SPEC.md +++ b/SPEC.md @@ -242,6 +242,12 @@ asset_pattern = "otherpkg-x86_64-unknown-linux-gnu.tar.gz" method = "same-origin-sha256" checksum_asset_pattern = "otherpkg-x86_64-unknown-linux-gnu.tar.gz.sha256" +# binary_name: only needed when the installed binary's name differs from +# the pacman package name — e.g. real-world case, scaleway-cli's package +# is named scaleway-cli but its actual binary is `scw` (see +# packages.d/scaleway-cli.toml). Defaults to the package name. +binary_name = "otherbin" + # Tier 1 example — not yet implemented in the PoC (only # same-origin-sha256 and github-attestation exist so far): [package.somepkg] @@ -281,13 +287,20 @@ implements `repo`, `asset_pattern`, and `verification.method` against the releases API in feed order rather than trusting the first entry outright. -Tier 4-6 packages (scaleway-cli included) are not auto-published — see -Build/publish/review-queue below. `source`, `check_method`, -`check_interval`, and `sanity_check` are still schema sketch, not yet read -by the code — the PoC only knows how to check GitHub-release sources. +`sanity_check` and `binary_name` are now real, implemented fields (see +Builder/Sanity checker above) — added `packages.d/uv.toml`'s and +`packages.d/scaleway-cli.toml`'s own `sanity_check` blocks, and +scaleway-cli's `binary_name = "scw"`. `source`, `check_method`, and +`check_interval` are still schema sketch, not yet read by the code — the +PoC only knows how to check GitHub-release sources, on a single one-shot +run rather than a scheduled loop. + Build/publish/review-queue (`makepkg`, `repo-add`, tier 4–6 human review) -are not implemented yet; a tier 4–6 pass currently just logs "flagging for -review" and stops. +are now implemented too — see Builder/Sanity checker/Publisher/Reviewer +queue above and the Status entry below for the first full end-to-end run. +Tier 4-6 packages still don't auto-publish (by design, see Verification +trust tiers > Automation posture per tier); they queue for +`pkgwatch review --approve`. Open questions on the schema: @@ -334,25 +347,54 @@ Open questions on the schema: separately. *(Implemented for `same-origin-sha256` and `github-attestation` — `src/verifier.rs`. The latter shells out to `gh attestation verify` rather than reimplementing sigstore verification.)* -- **Builder**: for tiers 1–3 on pass, generates/updates the PKGBUILD - (strict validation on any upstream-controlled string — version, filename - — before it touches generated shell content; never unescaped - interpolation) and runs `makepkg`. +- **Builder**: for tiers 1–3 on pass, generates a PKGBUILD (strict + validation on every upstream-controlled string — version, asset name, + download URL — before it touches generated shell content; every + interpolated value is embedded in a single-quoted bash string and a + literal `'` or newline in the input is rejected outright, never + unescaped interpolation) and runs `makepkg`. *(Implemented — + `src/builder.rs`. One fixed "prebuilt binary" PKGBUILD shape covers both + tracked packages so far: a bare-binary download (scaleway-cli) and a + tarball extracting to a same-named directory (uv) — see Scaling > + Template reuse. `Package.binary_name` (config.rs) covers the case where + the installed binary's name differs from the pacman package name, which + turned out to matter immediately: scaleway-cli's real binary is `scw`, + not `scaleway-cli` — confirmed by inspecting the currently-installed + `extra` package with `pacman -Ql`, not guessable from the repo name. + Without it the build would install alongside `extra`'s package instead + of shadowing it.)* - **Sanity checker**: after a successful build, runs the package's - declared `sanity_check.command` against the built artifact and confirms - the reported version matches what pkgwatch believes it just built. - Mismatch = fail loud, do not publish. This is a correctness check, not a - security control — it catches checker bugs and mangled/wrong-artifact - downloads, not malicious releases. -- **Publisher**: runs `repo-add` against the local repo, only after the - sanity check passes. + declared `sanity_check.command` — with the freshly built package's + `usr/bin` prepended to `PATH`, so it exercises what was just built + rather than whatever's already installed system-wide — and confirms the + reported version matches what pkgwatch believes it just built. Mismatch + = fail loud, do not publish. This is a correctness check, not a security + control — it catches checker bugs and mangled/wrong-artifact downloads, + not malicious releases. *(Implemented — `src/sanity.rs`.)* +- **Publisher**: copies the built package into the local repo directory + and runs `repo-add`, only after the sanity check passes. *(Implemented — + `src/publisher.rs`. Targets an existing, already-registered local pacman + repo rather than one pkgwatch creates — this box already has one at + `~/.local/share/pacman/custom`, registered as `[custom]` in + `/etc/pacman.conf` (`SigLevel = Optional TrustAll`) and already in use + for a hand-packaged AppImage, resolving the "where does the repo live / + how does it get registered" open question below without pkgwatch ever + touching pacman.conf. Deliberately stops at `repo-add`: getting the new + version onto the running system is a separate, deliberate + `pacman -Syu`/`pacman -S ` step left to the operator, not run + automatically.)* - **Reviewer queue**: for tiers 4–6, records the detected change instead of auto-building; a separate `pkgwatch review` command lets a human approve/reject, which then triggers the build → sanity-check → publish - steps above. + steps above. *(Implemented — `state::{load,save,clear}_pending_version` + plus the `review`/`review --approve` subcommands in `src/main.rs`. + Tracked separately from the last-published-version state: approving one + release doesn't mean future ones auto-publish. `--approve` re-verifies + before building rather than trusting a possibly-stale flag from an + earlier run. No reject/dismiss command yet — see Status below.)* - **Scheduling**: systemd `.service` (oneshot) + `.timer` running it periodically, matching the pattern already used for other periodic tasks - on this box. + on this box. *(Not implemented — still a single one-shot `cargo run`.)* ## Prior art / reference points @@ -395,18 +437,33 @@ Open questions on the schema: repo's newest feed entry, a `-dbg1` tag, has no real Release behind it). Still just flags for human review, same as any tier 4-6 pass — not auto-installed; see the unchecked build/publish item below. -- [ ] Not yet implemented: build (PKGBUILD generation + `makepkg`), - publish (`repo-add`), reviewer queue for tier 4–6, scheduling/ - `check_interval`, non-GitHub sources, `minisign`/tier-1 method. -- [ ] Refine config schema further (see open questions above), including - the `sanity_check` block per package. +- [x] Full pipeline closed end to end for the first time: check → fetch → + verify → build → sanity-check → publish, against two real packages. + `uv` (tier 2) auto-built and published on the first run with no + human step. `scaleway-cli` (tier 4) queued for review, then + `pkgwatch review scaleway-cli --approve` re-verified, built, and + published it — confirmed the built package installs as + `/usr/bin/scw`, actually shadowing `extra`'s package rather than + installing alongside it under the wrong name. Both landed in the + real `~/.local/share/pacman/custom` repo's database + (`custom.db.tar.gz`), ready for `sudo pacman -Syu`/`sudo pacman -S` + — not run automatically. See Builder/Sanity checker/Publisher/ + Reviewer queue above for what each piece does. + One cosmetic wrinkle, not a correctness issue: `makepkg` printed + `libfakeroot internal error: payload not recognized!` while + packaging scaleway-cli's large Go binary, but still produced a + correct package (verified: exactly `usr/bin/scw` plus standard + metadata) — looks like an environment quirk in this sandbox's + fakeroot, not something pkgwatch caused; revisit if a real build + ever actually fails on it. +- [ ] Not yet implemented: `pkgwatch review --reject` (a pending + review can only be approved or left pending, not dismissed), + scheduling/`check_interval`, non-GitHub sources, `minisign`/tier-1 + method, retention/pruning of old versions in the local repo (see + Scaling > Local repo retention), staggering/auth for GitHub API + rate limits at higher package counts. +- [ ] Refine config schema further (see open questions above). - [ ] Decide version-check strategy for non-GitHub sources: shell out to `nvchecker` vs. own implementation. -- [ ] Implement PKGBUILD generation with strict upstream-string validation - from day one (see Builder, above) — cheap to do right up front, - expensive to retrofit. -- [ ] Next PoC iteration: carry the verified `uv` artifact through - build → sanity-check → `repo-add` publish, closing the loop to an - actual local pacman repo `pacman -Syu` can pick up. - [ ] Decide on project home: local-only for now, or push to code.austinschaefer.com (Forgejo) once the spec settles. diff --git a/packages.d/scaleway-cli.toml b/packages.d/scaleway-cli.toml index 39317d6..2308561 100644 --- a/packages.d/scaleway-cli.toml +++ b/packages.d/scaleway-cli.toml @@ -7,11 +7,22 @@ # Releases ship one combined `SHA256SUMS` file (one line per platform # asset) rather than a per-asset checksum file like uv's — verifier # matches the line by filename. +# +# binary_name = "scw": confirmed by checking the currently-installed +# `extra` package (`pacman -Ql scaleway-cli`) — the pacman package is +# named scaleway-cli but the actual binary it installs is `scw`. Without +# this, pkgwatch's build would install as /usr/bin/scaleway-cli, which +# would NOT shadow extra's /usr/bin/scw at all. [package.scaleway-cli] repo = "scaleway/scaleway-cli" asset_pattern = "scaleway-cli_{version}_linux_amd64" +binary_name = "scw" [package.scaleway-cli.verification] method = "same-origin-sha256" checksum_asset_pattern = "SHA256SUMS" + +[package.scaleway-cli.sanity_check] +command = "scw version" +version_regex = 'Version\s+(\d+\.\d+\.\d+)' diff --git a/packages.d/uv.toml b/packages.d/uv.toml index 24f4894..0943391 100644 --- a/packages.d/uv.toml +++ b/packages.d/uv.toml @@ -10,3 +10,7 @@ asset_pattern = "uv-x86_64-unknown-linux-gnu.tar.gz" [package.uv.verification] method = "github-attestation" + +[package.uv.sanity_check] +command = "uv --version" +version_regex = 'uv (\d+\.\d+\.\d+)' diff --git a/src/builder.rs b/src/builder.rs new file mode 100644 index 0000000..0228649 --- /dev/null +++ b/src/builder.rs @@ -0,0 +1,359 @@ +use crate::config::Package; +use crate::hash::sha256_hex; +use anyhow::{Context, Result, bail}; +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// Archive extensions `makepkg` auto-extracts before `package()` runs. +/// Longest-first so `.tar.gz` isn't shadowed by a hypothetical `.gz` entry. +const ARCHIVE_EXTENSIONS: &[&str] = &[".tar.gz", ".tar.xz", ".tar.zst", ".tar.bz2", ".tgz", ".zip"]; + +/// Everything needed to generate and build a PKGBUILD for one release. +pub struct BuildRequest<'a> { + pub pkg_name: &'a str, + pub pkg: &'a Package, + pub version: &'a str, + pub repo: &'a str, + pub asset_name: &'a str, + pub download_url: &'a str, + pub artifact_path: &'a Path, +} + +pub struct BuildResult { + /// The built `.pkg.tar.zst`, ready for `publisher::publish`. + pub package_path: PathBuf, + /// `makepkg`'s package staging directory (`$pkgdir`), still present + /// after a successful build — lets `sanity` exercise the freshly built + /// binary without installing it system-wide first. + pub pkgdir: PathBuf, +} + +/// Generates a PKGBUILD around an already-downloaded, already-verified +/// artifact, then runs `makepkg` in `build_dir`. +/// +/// Deliberately one fixed "prebuilt binary" shape, not a templating engine +/// — see SPEC.md > Scaling > Template reuse. Covers the two shapes the two +/// currently-tracked packages actually need: a bare-binary download +/// (scaleway-cli) and a tarball containing a same-named directory (uv). +/// Extend when a third real shape shows up rather than guessing at +/// generality now. +pub fn build(req: &BuildRequest, build_dir: &Path) -> Result { + let pkgbuild = generate_pkgbuild(req)?; + + std::fs::create_dir_all(build_dir) + .with_context(|| format!("creating build dir {}", build_dir.display()))?; + std::fs::write(build_dir.join("PKGBUILD"), pkgbuild)?; + // makepkg looks for the source file by its declared name next to + // PKGBUILD; pre-seed it with the copy pkgwatch already downloaded and + // verified so makepkg's own sha256 check passes without re-fetching + // from the network (and without trusting the network a second time). + std::fs::copy(req.artifact_path, build_dir.join(req.asset_name))?; + + let status = Command::new("makepkg") + .args(["--noconfirm", "--force"]) + .current_dir(build_dir) + .status() + .context("running makepkg (is base-devel installed?)")?; + if !status.success() { + bail!("makepkg failed for {} {}", req.pkg_name, req.version); + } + + let package_path = find_built_package(build_dir, req.pkg_name, req.version)?; + let pkgdir = build_dir.join("pkg").join(req.pkg_name); + Ok(BuildResult { + package_path, + pkgdir, + }) +} + +/// Builds the PKGBUILD text for `req`, validating every upstream-controlled +/// string first (see SPEC.md > Architecture > Builder: "strict validation +/// on any upstream-controlled string ... never unescaped interpolation"). +/// Pure and side-effect-free so it's testable without invoking `makepkg`. +fn generate_pkgbuild(req: &BuildRequest) -> Result { + validate_pkgname(req.pkg_name)?; + validate_pkgver(req.version)?; + validate_shell_safe("asset name", req.asset_name)?; + validate_shell_safe("download url", req.download_url)?; + validate_shell_safe("repo", req.repo)?; + + let binary_name = req.pkg.binary_name(req.pkg_name); + validate_pkgname(binary_name)?; + + let artifact_data = std::fs::read(req.artifact_path) + .with_context(|| format!("reading {}", req.artifact_path.display()))?; + let sha256 = sha256_hex(&artifact_data); + + let install_source = match archive_stem(req.asset_name) { + Some(stem) => format!("{stem}/{binary_name}"), + None => req.asset_name.to_string(), + }; + + Ok(format!( + "# Maintainer: pkgwatch (auto-generated — do not edit by hand,\n\ + # edits are overwritten on the next update)\n\ + pkgname='{name}'\n\ + pkgver='{version}'\n\ + pkgrel=1\n\ + pkgdesc='{repo} release {version}, packaged by pkgwatch'\n\ + arch=('x86_64')\n\ + url='https://github.com/{repo}'\n\ + license=('unknown')\n\ + options=('!strip')\n\ + source=('{asset}::{url}')\n\ + sha256sums=('{sha256}')\n\ + \n\ + package() {{\n\ + \x20 install -Dm755 \"${{srcdir}}/{install_source}\" \"${{pkgdir}}/usr/bin/{binary_name}\"\n\ + }}\n", + name = req.pkg_name, + version = req.version, + repo = req.repo, + asset = req.asset_name, + url = req.download_url, + )) +} + +fn find_built_package(build_dir: &Path, pkg_name: &str, version: &str) -> Result { + let prefix = format!("{pkg_name}-{version}-"); + for entry in std::fs::read_dir(build_dir)? { + let path = entry?.path(); + let Some(file_name) = path.file_name().and_then(|n| n.to_str()) else { + continue; + }; + if file_name.starts_with(&prefix) && file_name.ends_with(".pkg.tar.zst") { + return Ok(path); + } + } + bail!( + "makepkg reported success but no {prefix}*.pkg.tar.zst found in {}", + build_dir.display() + ) +} + +/// Strips a recognized archive extension, returning the resulting stem — +/// the directory name `makepkg` extracts a same-named tarball into, by +/// the convention every currently-tracked tarball-shaped package follows. +/// `None` means the asset is a bare binary download (no extraction). +fn archive_stem(asset_name: &str) -> Option<&str> { + ARCHIVE_EXTENSIONS + .iter() + .find_map(|ext| asset_name.strip_suffix(ext)) +} + +/// Rejects a single quote or newline: both would let upstream-controlled +/// text (asset names, download URLs) break out of the single-quoted bash +/// strings the PKGBUILD template embeds them in. See SPEC.md > Architecture +/// > Builder ("never unescaped interpolation"). +fn validate_shell_safe(field: &str, value: &str) -> Result<()> { + if value.contains('\'') || value.contains('\n') { + bail!("{field} '{value}' contains an unsafe character for a generated PKGBUILD"); + } + Ok(()) +} + +/// A pacman `pkgver` may only contain alphanumerics, `.`, `_`, `+` — no +/// hyphens (pacman reserves `-` as the pkgver/pkgrel separator in the +/// final package filename) and no shell metacharacters. +fn validate_pkgver(version: &str) -> Result<()> { + let valid = !version.is_empty() + && version + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '+')); + if !valid { + bail!("'{version}' is not a valid pacman pkgver (only [A-Za-z0-9._+] allowed)"); + } + Ok(()) +} + +/// A pacman package/binary name may only contain lowercase alphanumerics +/// plus `@ . _ + -`. +fn validate_pkgname(name: &str) -> Result<()> { + let valid = !name.is_empty() + && name.chars().all(|c| { + c.is_ascii_lowercase() || c.is_ascii_digit() || matches!(c, '@' | '.' | '_' | '+' | '-') + }); + if !valid { + bail!("'{name}' is not a valid pacman package name"); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn archive_stem_strips_known_extensions() { + assert_eq!( + archive_stem("uv-x86_64-unknown-linux-gnu.tar.gz"), + Some("uv-x86_64-unknown-linux-gnu") + ); + assert_eq!(archive_stem("thing.zip"), Some("thing")); + } + + #[test] + fn archive_stem_none_for_bare_binary() { + assert_eq!(archive_stem("scaleway-cli_2.62.0_linux_amd64"), None); + } + + #[test] + fn validate_pkgver_accepts_dotted_version() { + assert!(validate_pkgver("2.62.0").is_ok()); + } + + #[test] + fn validate_pkgver_rejects_hyphen() { + assert!(validate_pkgver("2.62.0-dbg1").is_err()); + } + + #[test] + fn validate_pkgver_rejects_shell_metacharacters() { + assert!(validate_pkgver("2.62.0; rm -rf /").is_err()); + } + + #[test] + fn validate_pkgver_rejects_empty() { + assert!(validate_pkgver("").is_err()); + } + + #[test] + fn validate_pkgname_accepts_hyphenated_name() { + assert!(validate_pkgname("scaleway-cli").is_ok()); + } + + #[test] + fn validate_pkgname_rejects_uppercase() { + assert!(validate_pkgname("Scaleway-CLI").is_err()); + } + + #[test] + fn validate_shell_safe_rejects_single_quote() { + assert!(validate_shell_safe("asset name", "thing'; touch pwned #.tar.gz").is_err()); + } + + #[test] + fn validate_shell_safe_rejects_newline() { + assert!(validate_shell_safe("download url", "https://example.com/a\nb").is_err()); + } + + #[test] + fn validate_shell_safe_accepts_normal_url() { + assert!(validate_shell_safe("download url", "https://example.com/a/b.tar.gz").is_ok()); + } + + fn make_package(binary_name: Option<&str>) -> Package { + let toml_text = match binary_name { + Some(bin) => format!( + r#" + repo = "o/r" + asset_pattern = "x" + binary_name = "{bin}" + [verification] + method = "github-attestation" + "# + ), + None => r#" + repo = "o/r" + asset_pattern = "x" + [verification] + method = "github-attestation" + "# + .to_string(), + }; + toml::from_str(&toml_text).unwrap() + } + + #[test] + fn build_rejects_unsafe_version() { + let pkg = make_package(None); + let dir = tempfile::tempdir().unwrap(); + let artifact_path = dir.path().join("thing.tar.gz"); + std::fs::write(&artifact_path, b"data").unwrap(); + + let req = BuildRequest { + pkg_name: "thing", + pkg: &pkg, + version: "1.0.0-dbg1", + repo: "o/r", + asset_name: "thing.tar.gz", + download_url: "https://example.com/thing.tar.gz", + artifact_path: &artifact_path, + }; + let build_dir = dir.path().join("build"); + assert!(build(&req, &build_dir).is_err()); + } + + #[test] + fn generate_pkgbuild_bare_binary_installs_under_binary_name_override() { + let pkg = make_package(Some("scw")); + let dir = tempfile::tempdir().unwrap(); + let artifact_path = dir.path().join("scaleway-cli_2.62.0_linux_amd64"); + std::fs::write(&artifact_path, b"binary-bytes").unwrap(); + let expected_sha = sha256_hex(b"binary-bytes"); + + let req = BuildRequest { + pkg_name: "scaleway-cli", + pkg: &pkg, + version: "2.62.0", + repo: "scaleway/scaleway-cli", + asset_name: "scaleway-cli_2.62.0_linux_amd64", + download_url: "https://github.com/scaleway/scaleway-cli/releases/download/v2.62.0/scaleway-cli_2.62.0_linux_amd64", + artifact_path: &artifact_path, + }; + let pkgbuild = generate_pkgbuild(&req).unwrap(); + + assert!(pkgbuild.contains("pkgname='scaleway-cli'")); + assert!(pkgbuild.contains("pkgver='2.62.0'")); + assert!(pkgbuild.contains(&format!("sha256sums=('{expected_sha}')"))); + // Bare binary (no archive extension) — installed straight from + // srcdir under the overridden binary name, not the pkgname. + assert!(pkgbuild.contains( + "install -Dm755 \"${srcdir}/scaleway-cli_2.62.0_linux_amd64\" \"${pkgdir}/usr/bin/scw\"" + )); + } + + #[test] + fn generate_pkgbuild_tarball_installs_from_extracted_stem_dir() { + let pkg = make_package(None); + let dir = tempfile::tempdir().unwrap(); + let artifact_path = dir.path().join("uv-x86_64-unknown-linux-gnu.tar.gz"); + std::fs::write(&artifact_path, b"tarball-bytes").unwrap(); + + let req = BuildRequest { + pkg_name: "uv", + pkg: &pkg, + version: "0.12.15", + repo: "astral-sh/uv", + asset_name: "uv-x86_64-unknown-linux-gnu.tar.gz", + download_url: "https://github.com/astral-sh/uv/releases/download/0.12.15/uv-x86_64-unknown-linux-gnu.tar.gz", + artifact_path: &artifact_path, + }; + let pkgbuild = generate_pkgbuild(&req).unwrap(); + + // No binary_name override — pkgname doubles as the binary name, + // and makepkg extracts the tarball into a same-named directory. + assert!(pkgbuild.contains( + "install -Dm755 \"${srcdir}/uv-x86_64-unknown-linux-gnu/uv\" \"${pkgdir}/usr/bin/uv\"" + )); + } + + #[test] + fn generate_pkgbuild_rejects_download_url_with_single_quote() { + let pkg = make_package(None); + let dir = tempfile::tempdir().unwrap(); + let artifact_path = dir.path().join("thing.tar.gz"); + std::fs::write(&artifact_path, b"data").unwrap(); + + let req = BuildRequest { + pkg_name: "thing", + pkg: &pkg, + version: "1.0.0", + repo: "o/r", + asset_name: "thing.tar.gz", + download_url: "https://example.com/x'; touch pwned #.tar.gz", + artifact_path: &artifact_path, + }; + assert!(generate_pkgbuild(&req).is_err()); + } +} diff --git a/src/config.rs b/src/config.rs index b24ab2f..1804bf5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -18,6 +18,31 @@ pub struct Package { /// `checker::version_from_tag` before matching. pub asset_pattern: String, pub verification: Verification, + /// Name of the executable inside the built package, if it differs from + /// the package name itself — e.g. scaleway-cli's pacman package is + /// named `scaleway-cli` but its real binary is `scw` (discovered by + /// checking the currently-installed extra package, not guessable from + /// the repo name). Defaults to the package name when omitted. + pub binary_name: Option, + /// Post-build correctness check (not a security control — see + /// SPEC.md > Verification trust tiers). Runs `command` against the + /// freshly built binary and confirms `version_regex`'s capture group + /// matches the version pkgwatch believes it just built. + pub sanity_check: Option, +} + +impl Package { + /// The name of the executable inside the built package: `binary_name` + /// if the package declares one, else `pkg_name` itself. + pub fn binary_name<'a>(&'a self, pkg_name: &'a str) -> &'a str { + self.binary_name.as_deref().unwrap_or(pkg_name) + } +} + +#[derive(Debug, Deserialize, Clone)] +pub struct SanityCheck { + pub command: String, + pub version_regex: String, } #[derive(Debug, Deserialize, Clone)] diff --git a/src/fetcher.rs b/src/fetcher.rs index 398b986..3ef4e11 100644 --- a/src/fetcher.rs +++ b/src/fetcher.rs @@ -14,8 +14,18 @@ struct Asset { browser_download_url: String, } +/// A downloaded release asset: its local path plus the URL it came from, +/// the latter needed for the `source=` line of a generated PKGBUILD (see +/// `builder`) — `makepkg` uses it only as a fallback if the pre-seeded +/// local copy ever goes missing. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DownloadedAsset { + pub path: PathBuf, + pub download_url: String, +} + /// Downloads the release asset named exactly `asset_name` for `repo`@`tag` -/// into `dest_dir`, returning the local path. +/// into `dest_dir`, returning the local path and its origin URL. pub fn download_asset( client: &reqwest::blocking::Client, endpoints: &GithubEndpoints, @@ -23,7 +33,7 @@ pub fn download_asset( tag: &str, asset_name: &str, dest_dir: &Path, -) -> Result { +) -> Result { let api_url = format!("{}/repos/{repo}/releases/tags/{tag}", endpoints.api); let release: Release = client .get(&api_url) @@ -46,7 +56,10 @@ pub fn download_asset( .error_for_status()? .bytes()?; std::fs::write(&dest_path, &bytes)?; - Ok(dest_path) + Ok(DownloadedAsset { + path: dest_path, + download_url: asset.browser_download_url.clone(), + }) } #[cfg(test)] @@ -77,7 +90,7 @@ mod tests { let client = reqwest::blocking::Client::new(); let dest_dir = tempfile::tempdir().unwrap(); - let path = download_asset( + let asset = download_asset( &client, &endpoints, "o/r", @@ -87,8 +100,9 @@ mod tests { ) .unwrap(); - assert_eq!(path, dest_dir.path().join("thing.tar.gz")); - assert_eq!(std::fs::read(&path).unwrap(), b"artifact-bytes"); + assert_eq!(asset.path, dest_dir.path().join("thing.tar.gz")); + assert_eq!(asset.download_url, asset_url); + assert_eq!(std::fs::read(&asset.path).unwrap(), b"artifact-bytes"); } #[test] diff --git a/src/hash.rs b/src/hash.rs new file mode 100644 index 0000000..7320600 --- /dev/null +++ b/src/hash.rs @@ -0,0 +1,25 @@ +use sha2::{Digest, Sha256}; + +/// Shared by `verifier` (same-origin-sha256 checks) and `builder` (every +/// generated PKGBUILD needs a `sha256sums` entry for makepkg's own local +/// integrity check, regardless of pkgwatch's own trust tier for that +/// package). +pub fn sha256_hex(data: &[u8]) -> String { + let mut hasher = Sha256::new(); + hasher.update(data); + hex::encode(hasher.finalize()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn matches_known_sha256() { + // printf 'hello world' | sha256sum + assert_eq!( + sha256_hex(b"hello world"), + "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" + ); + } +} diff --git a/src/main.rs b/src/main.rs index 54ffbb5..cfbb055 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,25 +1,65 @@ +mod builder; mod checker; mod config; mod fetcher; mod github; +mod hash; +mod publisher; +mod sanity; mod state; mod verifier; -use anyhow::Result; +use anyhow::{Context, Result, bail}; +use config::Package; +use fetcher::DownloadedAsset; use github::GithubEndpoints; -use std::path::Path; +use std::path::{Path, PathBuf}; +use verifier::VerificationResult; -/// First iteration: check -> fetch -> verify -> report, for whatever is -/// in packages.d/. No build/publish step yet (see SPEC.md > Status). +const PACKAGES_DIR: &str = "packages.d"; +const STATE_DIR: &str = "state"; +const WORK_DIR: &str = "work"; +/// Not a repo pkgwatch invents: this is the existing, already-registered +/// local pacman repo on this box (see `[custom]` in /etc/pacman.conf and +/// its `Server = file://...` line). pkgwatch adds packages to it; it does +/// not create the repo or touch pacman.conf. +const CUSTOM_REPO_NAME: &str = "custom"; +const CUSTOM_REPO_SUBPATH: &str = ".local/share/pacman/custom"; + +/// check -> fetch -> verify -> build -> sanity-check -> publish, for +/// whatever is in packages.d/. Tier 1-3 passes auto-publish; tier 4-6 +/// passes queue for `pkgwatch review`. See SPEC.md > Architecture. fn main() -> Result<()> { - let client = reqwest::blocking::Client::builder() - .user_agent("pkgwatch/0.1 (PoC; https://code.austinschaefer.com)") - .build()?; - let endpoints = GithubEndpoints::default(); + let args: Vec = std::env::args().skip(1).collect(); + match args.first().map(String::as_str) { + None => run_check(), + Some("review") => run_review(&args[1..]), + Some(other) => bail!("unknown subcommand '{other}' (expected: review)"), + } +} - let packages_dir = Path::new("packages.d"); - let state_dir = Path::new("state"); - let work_dir = Path::new("work"); +fn build_client() -> Result { + Ok(reqwest::blocking::Client::builder() + .user_agent("pkgwatch/0.1 (PoC; https://code.austinschaefer.com)") + .build()?) +} + +fn custom_repo_dir() -> Result { + // Override for testing against a scratch repo instead of the real one + // at $HOME/.local/share/pacman/custom. + if let Ok(dir) = std::env::var("PKGWATCH_REPO_DIR") { + return Ok(PathBuf::from(dir)); + } + let home = std::env::var("HOME").context("HOME is not set")?; + Ok(Path::new(&home).join(CUSTOM_REPO_SUBPATH)) +} + +fn run_check() -> Result<()> { + let client = build_client()?; + let endpoints = GithubEndpoints::default(); + let packages_dir = Path::new(PACKAGES_DIR); + let state_dir = Path::new(STATE_DIR); + let work_dir = Path::new(WORK_DIR); let packages = config::load_packages_dir(packages_dir)?; if packages.is_empty() { @@ -27,66 +67,207 @@ fn main() -> Result<()> { return Ok(()); } - for (name, pkg) in packages { + let mut any_failed = false; + for (name, pkg) in &packages { println!("== {name} ({}) ==", pkg.repo); - - let latest = checker::latest_github_release(&client, &endpoints, &pkg.repo)?; - let last_seen = state::load_last_version(state_dir, &name); - - if last_seen.as_deref() == Some(latest.as_str()) { - println!(" up to date at {latest}"); - continue; - } - - println!(" new version detected: {latest} (previously: {last_seen:?})"); - - let dest_dir = work_dir.join(&name).join(&latest); - let asset_name = pkg - .asset_pattern - .replace("{version}", checker::version_from_tag(&latest)); - let artifact_path = fetcher::download_asset( - &client, - &endpoints, - &pkg.repo, - &latest, - &asset_name, - &dest_dir, - )?; - println!(" fetched {}", artifact_path.display()); - - let result = verifier::verify( - &client, - &endpoints, - &pkg.verification, - &pkg.repo, - &latest, - &artifact_path, - &dest_dir, - )?; - - println!( - " verification (tier {}): {} — {}", - result.tier, - if result.passed { "PASS" } else { "FAIL" }, - result.justification - ); - - match (result.tier, result.passed) { - (1..=3, true) => { - println!(" tier 1-3 pass: would auto-build + publish (not yet implemented)"); - state::save_last_version(state_dir, &name, &latest)?; - } - (_, true) => { - println!(" tier 4-6 pass: flagging for human review, not auto-publishing"); - println!( - " (review-queue persistence not yet implemented — this is where it plugs in)" - ); - } - (_, false) => { - println!(" verification failed — not publishing, not updating state"); - } + if let Err(err) = process_package(&client, &endpoints, state_dir, work_dir, name, pkg) { + eprintln!(" error: {err:#}"); + any_failed = true; } } + if any_failed { + bail!("one or more packages failed — see errors above"); + } + Ok(()) +} + +fn process_package( + client: &reqwest::blocking::Client, + endpoints: &GithubEndpoints, + state_dir: &Path, + work_dir: &Path, + name: &str, + pkg: &Package, +) -> Result<()> { + let latest = checker::latest_github_release(client, endpoints, &pkg.repo)?; + let last_seen = state::load_last_version(state_dir, name); + if last_seen.as_deref() == Some(latest.as_str()) { + println!(" up to date at {latest}"); + return Ok(()); + } + println!(" new version detected: {latest} (previously: {last_seen:?})"); + + let fetched = fetch_and_verify(client, endpoints, work_dir, name, pkg, &latest)?; + println!(" fetched {}", fetched.asset.path.display()); + println!( + " verification (tier {}): {} — {}", + fetched.verification.tier, + if fetched.verification.passed { + "PASS" + } else { + "FAIL" + }, + fetched.verification.justification + ); + + if !fetched.verification.passed { + println!(" verification failed — not publishing, not updating state"); + return Ok(()); + } + + if fetched.verification.tier <= 3 { + println!(" tier 1-3 pass: building + publishing"); + build_and_publish(name, pkg, &latest, &fetched)?; + state::save_last_version(state_dir, name, &latest)?; + state::clear_pending_version(state_dir, name)?; + println!(" published {name} {latest}"); + } else if state::load_pending_version(state_dir, name).as_deref() == Some(latest.as_str()) { + println!(" tier 4-6 pass: still pending review (`pkgwatch review` to see it)"); + } else { + state::save_pending_version(state_dir, name, &latest)?; + println!(" tier 4-6 pass: flagged for human review (`pkgwatch review` to approve)"); + } + Ok(()) +} + +struct FetchVerifyResult { + version: String, + asset_name: String, + dest_dir: PathBuf, + asset: DownloadedAsset, + verification: VerificationResult, +} + +/// Shared by the normal check loop (tier 1-3 auto-path) and `pkgwatch +/// review --approve` (which re-verifies before publishing rather than +/// trusting a possibly-stale flag from an earlier run). +fn fetch_and_verify( + client: &reqwest::blocking::Client, + endpoints: &GithubEndpoints, + work_dir: &Path, + name: &str, + pkg: &Package, + tag: &str, +) -> Result { + let version = checker::version_from_tag(tag).to_string(); + let asset_name = pkg.asset_pattern.replace("{version}", &version); + let dest_dir = work_dir.join(name).join(tag); + + let asset = fetcher::download_asset(client, endpoints, &pkg.repo, tag, &asset_name, &dest_dir)?; + let verification = verifier::verify( + client, + endpoints, + &pkg.verification, + &pkg.repo, + tag, + &asset.path, + &dest_dir, + )?; + + Ok(FetchVerifyResult { + version, + asset_name, + dest_dir, + asset, + verification, + }) +} + +fn build_and_publish( + name: &str, + pkg: &Package, + tag: &str, + fetched: &FetchVerifyResult, +) -> Result<()> { + let build_dir = fetched.dest_dir.join("build"); + let req = builder::BuildRequest { + pkg_name: name, + pkg, + version: &fetched.version, + repo: &pkg.repo, + asset_name: &fetched.asset_name, + download_url: &fetched.asset.download_url, + artifact_path: &fetched.asset.path, + }; + let built = builder::build(&req, &build_dir)?; + println!(" built {}", built.package_path.display()); + + if let Some(check) = &pkg.sanity_check { + let bin_dir = built.pkgdir.join("usr/bin"); + sanity::run(check, &bin_dir, &fetched.version) + .with_context(|| format!("sanity check for {name} {tag}"))?; + println!(" sanity check passed"); + } + + let repo_dir = custom_repo_dir()?; + let published = publisher::publish(&built.package_path, &repo_dir, CUSTOM_REPO_NAME)?; + println!( + " added to {} repo: {}", + CUSTOM_REPO_NAME, + published.display() + ); + println!( + " not installed automatically — run `sudo pacman -Syu` (or `sudo pacman -S {name}`) to pick it up" + ); + Ok(()) +} + +fn run_review(args: &[String]) -> Result<()> { + let packages_dir = Path::new(PACKAGES_DIR); + let state_dir = Path::new(STATE_DIR); + let work_dir = Path::new(WORK_DIR); + let packages = config::load_packages_dir(packages_dir)?; + + match args { + [] => { + let mut any = false; + for (name, _) in &packages { + if let Some(pending) = state::load_pending_version(state_dir, name) { + println!( + "{name}: {pending} pending review (run `pkgwatch review {name} --approve`)" + ); + any = true; + } + } + if !any { + println!("no packages pending review"); + } + Ok(()) + } + [name, flag] if flag == "--approve" => { + let (_, pkg) = packages.iter().find(|(n, _)| n == name).with_context(|| { + format!("no package named '{name}' in {}/", packages_dir.display()) + })?; + let tag = state::load_pending_version(state_dir, name) + .with_context(|| format!("'{name}' has no pending review"))?; + approve(state_dir, work_dir, name, pkg, &tag) + } + _ => bail!("usage: pkgwatch review [ --approve]"), + } +} + +fn approve(state_dir: &Path, work_dir: &Path, name: &str, pkg: &Package, tag: &str) -> Result<()> { + let client = build_client()?; + let endpoints = GithubEndpoints::default(); + + // Re-verify rather than trusting the earlier flag: the artifact at + // this tag could in principle have changed since it was queued. + let fetched = fetch_and_verify(&client, &endpoints, work_dir, name, pkg, tag)?; + if !fetched.verification.passed { + bail!( + "re-verification failed on approve: {}", + fetched.verification.justification + ); + } + println!( + "re-verified (tier {}): {}", + fetched.verification.tier, fetched.verification.justification + ); + + build_and_publish(name, pkg, tag, &fetched)?; + state::save_last_version(state_dir, name, tag)?; + state::clear_pending_version(state_dir, name)?; + println!("approved and published {name} {tag}"); Ok(()) } diff --git a/src/publisher.rs b/src/publisher.rs new file mode 100644 index 0000000..3cd675b --- /dev/null +++ b/src/publisher.rs @@ -0,0 +1,123 @@ +use anyhow::{Context, Result, bail}; +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// Copies the built package into `repo_dir` and runs `repo-add` against +/// `.db.tar.gz` there. +/// +/// `repo_dir`/`repo_name` are expected to already be a real, registered +/// pacman repo (see SPEC.md > Architecture > Publisher and +/// `/etc/pacman.conf`'s `[custom]` section on this box) — pkgwatch doesn't +/// create the repo or touch pacman.conf, only adds packages to an +/// already-registered one. Getting the new version into an installed +/// system is a separate, deliberate `pacman -Syu`/`pacman -S` step left to +/// the operator, not run automatically here. +pub fn publish(package_path: &Path, repo_dir: &Path, repo_name: &str) -> Result { + publish_with(Path::new("repo-add"), package_path, repo_dir, repo_name) +} + +/// `repo_add_bin` is injectable so tests can point it at a stub script +/// instead of the real `repo-add` (or a mutated global `PATH`, which would +/// race with `cargo test`'s parallel test threads). +fn publish_with( + repo_add_bin: &Path, + package_path: &Path, + repo_dir: &Path, + repo_name: &str, +) -> Result { + std::fs::create_dir_all(repo_dir) + .with_context(|| format!("creating repo dir {}", repo_dir.display()))?; + let file_name = package_path + .file_name() + .context("built package path has no filename")?; + let dest = repo_dir.join(file_name); + std::fs::copy(package_path, &dest) + .with_context(|| format!("copying {} to {}", package_path.display(), dest.display()))?; + + let db_path = repo_dir.join(format!("{repo_name}.db.tar.gz")); + let status = Command::new(repo_add_bin) + .arg(&db_path) + .arg(&dest) + .status() + .with_context(|| { + format!( + "running {} (is pacman-contrib installed?)", + repo_add_bin.display() + ) + })?; + if !status.success() { + bail!("repo-add failed for {}", dest.display()); + } + Ok(dest) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn write_stub(dir: &Path, name: &str, script: &str) -> PathBuf { + let path = dir.join(name); + std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap(); + let mut perms = std::fs::metadata(&path).unwrap().permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o755); + std::fs::set_permissions(&path, perms).unwrap(); + path + } + + #[test] + fn publish_copies_package_and_invokes_repo_add() { + let stub_dir = tempfile::tempdir().unwrap(); + let log_path = stub_dir.path().join("invoked_with.txt"); + let repo_add = write_stub( + stub_dir.path(), + "fake-repo-add", + &format!("echo \"$@\" > {}", log_path.display()), + ); + + let src_dir = tempfile::tempdir().unwrap(); + let package_path = src_dir.path().join("thing-1.0.0-1-x86_64.pkg.tar.zst"); + std::fs::write(&package_path, b"pkg-bytes").unwrap(); + + let repo_dir = tempfile::tempdir().unwrap(); + let dest = publish_with(&repo_add, &package_path, repo_dir.path(), "custom").unwrap(); + + assert_eq!( + dest, + repo_dir.path().join("thing-1.0.0-1-x86_64.pkg.tar.zst") + ); + assert_eq!(std::fs::read(&dest).unwrap(), b"pkg-bytes"); + + let invoked_with = std::fs::read_to_string(&log_path).unwrap(); + assert!(invoked_with.contains("custom.db.tar.gz")); + assert!(invoked_with.contains("thing-1.0.0-1-x86_64.pkg.tar.zst")); + } + + #[test] + fn publish_errors_when_repo_add_fails() { + let stub_dir = tempfile::tempdir().unwrap(); + let repo_add = write_stub(stub_dir.path(), "fake-repo-add-fail", "exit 1"); + + let src_dir = tempfile::tempdir().unwrap(); + let package_path = src_dir.path().join("thing-1.0.0-1-x86_64.pkg.tar.zst"); + std::fs::write(&package_path, b"pkg-bytes").unwrap(); + + let repo_dir = tempfile::tempdir().unwrap(); + let err = publish_with(&repo_add, &package_path, repo_dir.path(), "custom").unwrap_err(); + assert!(err.to_string().contains("repo-add failed")); + } + + #[test] + fn publish_creates_repo_dir_if_missing() { + let stub_dir = tempfile::tempdir().unwrap(); + let repo_add = write_stub(stub_dir.path(), "fake-repo-add-ok", "exit 0"); + + let src_dir = tempfile::tempdir().unwrap(); + let package_path = src_dir.path().join("thing-1.0.0-1-x86_64.pkg.tar.zst"); + std::fs::write(&package_path, b"pkg-bytes").unwrap(); + + let parent = tempfile::tempdir().unwrap(); + let repo_dir = parent.path().join("nested/repo"); + publish_with(&repo_add, &package_path, &repo_dir, "custom").unwrap(); + assert!(repo_dir.join("thing-1.0.0-1-x86_64.pkg.tar.zst").exists()); + } +} diff --git a/src/sanity.rs b/src/sanity.rs new file mode 100644 index 0000000..cc93f9d --- /dev/null +++ b/src/sanity.rs @@ -0,0 +1,125 @@ +use crate::config::SanityCheck; +use anyhow::{Context, Result, bail}; +use regex::Regex; +use std::path::Path; +use std::process::Command; + +/// Runs `check.command` with `pkg_bin_dir` prepended to `PATH`, so it +/// exercises the binary pkgwatch just built (still sitting in makepkg's +/// package staging directory, not installed system-wide) rather than +/// whatever's already on the system. Confirms `check.version_regex`'s +/// capture group matches `expected_version`. +/// +/// Correctness check only, not a security control — see SPEC.md > +/// Verification trust tiers. Catches checker bugs and mangled/wrong-asset +/// downloads, not malicious releases. +pub fn run(check: &SanityCheck, pkg_bin_dir: &Path, expected_version: &str) -> Result<()> { + let path_env = format!( + "{}:{}", + pkg_bin_dir.display(), + std::env::var("PATH").unwrap_or_default() + ); + let output = Command::new("sh") + .arg("-c") + .arg(&check.command) + .env("PATH", path_env) + .output() + .with_context(|| format!("running sanity check command '{}'", check.command))?; + if !output.status.success() { + bail!( + "sanity check command '{}' exited with {}: {}", + check.command, + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ); + } + + let combined = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let re = Regex::new(&check.version_regex) + .with_context(|| format!("invalid version_regex '{}'", check.version_regex))?; + let found = re + .captures(&combined) + .and_then(|caps| caps.get(1)) + .with_context(|| { + format!( + "version_regex '{}' did not match sanity check output: {combined:?}", + check.version_regex + ) + })? + .as_str(); + + if found != expected_version { + bail!( + "sanity check reported version '{found}', pkgwatch built '{expected_version}' — mismatch" + ); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn write_fake_binary(dir: &Path, name: &str, script: &str) { + let path = dir.join(name); + std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap(); + let mut perms = std::fs::metadata(&path).unwrap().permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o755); + std::fs::set_permissions(&path, perms).unwrap(); + } + + #[test] + fn run_passes_when_reported_version_matches() { + let dir = tempfile::tempdir().unwrap(); + write_fake_binary(dir.path(), "uv", "echo 'uv 0.12.15 (abc 2026-09-01)'"); + + let check = SanityCheck { + command: "uv --version".to_string(), + version_regex: r"uv (\d+\.\d+\.\d+)".to_string(), + }; + assert!(run(&check, dir.path(), "0.12.15").is_ok()); + } + + #[test] + fn run_fails_when_reported_version_differs() { + let dir = tempfile::tempdir().unwrap(); + write_fake_binary(dir.path(), "uv", "echo 'uv 0.12.14 (abc 2026-08-01)'"); + + let check = SanityCheck { + command: "uv --version".to_string(), + version_regex: r"uv (\d+\.\d+\.\d+)".to_string(), + }; + let err = run(&check, dir.path(), "0.12.15").unwrap_err(); + assert!(err.to_string().contains("mismatch")); + } + + #[test] + fn run_fails_when_command_exits_nonzero() { + let dir = tempfile::tempdir().unwrap(); + write_fake_binary(dir.path(), "uv", "exit 1"); + + let check = SanityCheck { + command: "uv --version".to_string(), + version_regex: r"uv (\d+\.\d+\.\d+)".to_string(), + }; + let err = run(&check, dir.path(), "0.12.15").unwrap_err(); + assert!(err.to_string().contains("exited with")); + } + + #[test] + fn run_fails_when_output_does_not_match_regex() { + let dir = tempfile::tempdir().unwrap(); + write_fake_binary(dir.path(), "uv", "echo 'not a version'"); + + let check = SanityCheck { + command: "uv --version".to_string(), + version_regex: r"uv (\d+\.\d+\.\d+)".to_string(), + }; + let err = run(&check, dir.path(), "0.12.15").unwrap_err(); + assert!(err.to_string().contains("did not match")); + } +} diff --git a/src/state.rs b/src/state.rs index 0bddf7c..d10ec2d 100644 --- a/src/state.rs +++ b/src/state.rs @@ -17,6 +17,33 @@ pub fn save_last_version(state_dir: &Path, name: &str, version: &str) -> Result< Ok(()) } +/// Tag currently awaiting human review for a tier 4-6 package (see +/// SPEC.md > Architecture > Reviewer queue), if any. Separate from +/// `load_last_version`/`save_last_version`: approving a review doesn't +/// mean future versions auto-publish, so the two must be tracked +/// independently. +pub fn load_pending_version(state_dir: &Path, name: &str) -> Option { + std::fs::read_to_string(state_dir.join(format!("{name}.pending"))) + .ok() + .map(|s| s.trim().to_string()) +} + +pub fn save_pending_version(state_dir: &Path, name: &str, version: &str) -> Result<()> { + std::fs::create_dir_all(state_dir)?; + std::fs::write(state_dir.join(format!("{name}.pending")), version)?; + Ok(()) +} + +/// Clears a pending review, e.g. once it's been approved and published. +/// Not an error if there was nothing pending. +pub fn clear_pending_version(state_dir: &Path, name: &str) -> Result<()> { + match std::fs::remove_file(state_dir.join(format!("{name}.pending"))) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e.into()), + } +} + #[cfg(test)] mod tests { use super::*; @@ -57,4 +84,49 @@ mod tests { Some("0.12.15".to_string()) ); } + + #[test] + fn load_pending_version_missing_file_returns_none() { + let dir = tempfile::tempdir().unwrap(); + assert_eq!(load_pending_version(dir.path(), "scaleway-cli"), None); + } + + #[test] + fn save_then_load_pending_roundtrips() { + let dir = tempfile::tempdir().unwrap(); + save_pending_version(dir.path(), "scaleway-cli", "v2.62.0").unwrap(); + assert_eq!( + load_pending_version(dir.path(), "scaleway-cli"), + Some("v2.62.0".to_string()) + ); + } + + #[test] + fn clear_pending_version_removes_it() { + let dir = tempfile::tempdir().unwrap(); + save_pending_version(dir.path(), "scaleway-cli", "v2.62.0").unwrap(); + clear_pending_version(dir.path(), "scaleway-cli").unwrap(); + assert_eq!(load_pending_version(dir.path(), "scaleway-cli"), None); + } + + #[test] + fn clear_pending_version_is_a_noop_when_nothing_pending() { + let dir = tempfile::tempdir().unwrap(); + assert!(clear_pending_version(dir.path(), "scaleway-cli").is_ok()); + } + + #[test] + fn pending_and_last_version_are_tracked_independently() { + let dir = tempfile::tempdir().unwrap(); + save_last_version(dir.path(), "scaleway-cli", "v2.61.0").unwrap(); + save_pending_version(dir.path(), "scaleway-cli", "v2.62.0").unwrap(); + assert_eq!( + load_last_version(dir.path(), "scaleway-cli"), + Some("v2.61.0".to_string()) + ); + assert_eq!( + load_pending_version(dir.path(), "scaleway-cli"), + Some("v2.62.0".to_string()) + ); + } } diff --git a/src/verifier.rs b/src/verifier.rs index d7746e2..a214f85 100644 --- a/src/verifier.rs +++ b/src/verifier.rs @@ -2,8 +2,8 @@ use crate::checker::version_from_tag; use crate::config::Verification; use crate::fetcher; use crate::github::GithubEndpoints; +use crate::hash::sha256_hex; use anyhow::{Context, Result, bail}; -use sha2::{Digest, Sha256}; use std::path::Path; use std::process::Command; @@ -31,7 +31,7 @@ pub fn verify( } => { let checksum_asset_name = checksum_asset_pattern.replace("{version}", version_from_tag(tag)); - let checksum_path = fetcher::download_asset( + let checksum_asset = fetcher::download_asset( client, endpoints, repo, @@ -39,7 +39,7 @@ pub fn verify( &checksum_asset_name, dest_dir, )?; - let checksum_text = std::fs::read_to_string(&checksum_path)?; + let checksum_text = std::fs::read_to_string(&checksum_asset.path)?; let artifact_name = artifact_path .file_name() .and_then(|n| n.to_str()) @@ -94,12 +94,6 @@ pub fn verify( } } -fn sha256_hex(data: &[u8]) -> String { - let mut hasher = Sha256::new(); - hasher.update(data); - hex::encode(hasher.finalize()) -} - /// Finds the expected hash for `artifact_name` in a checksum file. /// /// Handles both a bare-hash file covering a single asset (e.g. uv's -- 2.45.2 From 6305743e4dc44e4e45b8d7333d78bb0ad34fb24b Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Thu, 17 Sep 2026 11:30:43 +0200 Subject: [PATCH 2/6] Check the custom repo is registered in pacman.conf before building MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR feedback: the local repo isn't guaranteed to exist on every box this runs on, so it shouldn't just be assumed. publisher::ensure_registered checks /etc/pacman.conf for an active [] section before a build even starts, failing fast with the exact snippet to add if it's missing — instead of spending several seconds on a makepkg build that would succeed and then publish into a repo pacman never syncs from. The repo directory and its database file were already self-healing (publish creates the dir if missing, repo-add creates the db on first run) — the actual gap was the pacman.conf registration, which can't be made self-healing without root, so this fails loud with instructions instead of trying to write to /etc/pacman.conf itself. Co-Authored-By: Claude Sonnet 5 --- SPEC.md | 17 +++++++-- src/main.rs | 7 +++- src/publisher.rs | 96 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 107 insertions(+), 13 deletions(-) diff --git a/SPEC.md b/SPEC.md index 5e2f8c9..b793611 100644 --- a/SPEC.md +++ b/SPEC.md @@ -377,10 +377,19 @@ Open questions on the schema: repo rather than one pkgwatch creates — this box already has one at `~/.local/share/pacman/custom`, registered as `[custom]` in `/etc/pacman.conf` (`SigLevel = Optional TrustAll`) and already in use - for a hand-packaged AppImage, resolving the "where does the repo live / - how does it get registered" open question below without pkgwatch ever - touching pacman.conf. Deliberately stops at `repo-add`: getting the new - version onto the running system is a separate, deliberate + for a hand-packaged AppImage. But that repo directory/registration isn't + guaranteed to exist on every box this ever runs on, so it isn't just + assumed: `publisher::ensure_registered` checks `/etc/pacman.conf` for an + active `[]` section before a build even starts, failing fast + with the exact snippet to add if it's missing, rather than wasting a + `makepkg` build on a repo pacman will never sync from. The repo + *directory* and its database file, by contrast, are fully self-healing — + `publish` creates the directory if missing and `repo-add` creates the + database on its first run. What's deliberately not automatic, and can't + safely be: writing the `[section]` into `/etc/pacman.conf` itself — that + needs root, which this process doesn't have and shouldn't grab for + itself. Similarly, publish deliberately stops at `repo-add`: getting the + new version onto the running system is a separate, deliberate `pacman -Syu`/`pacman -S ` step left to the operator, not run automatically.)* - **Reviewer queue**: for tiers 4–6, records the detected change instead of diff --git a/src/main.rs b/src/main.rs index cfbb055..73885a5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -180,6 +180,12 @@ fn build_and_publish( tag: &str, fetched: &FetchVerifyResult, ) -> Result<()> { + // Fail fast if the repo isn't registered in pacman.conf, before + // spending several seconds on a makepkg build that would otherwise + // succeed and then publish somewhere pacman never syncs from. + let repo_dir = custom_repo_dir()?; + publisher::ensure_registered(CUSTOM_REPO_NAME, &repo_dir)?; + let build_dir = fetched.dest_dir.join("build"); let req = builder::BuildRequest { pkg_name: name, @@ -200,7 +206,6 @@ fn build_and_publish( println!(" sanity check passed"); } - let repo_dir = custom_repo_dir()?; let published = publisher::publish(&built.package_path, &repo_dir, CUSTOM_REPO_NAME)?; println!( " added to {} repo: {}", diff --git a/src/publisher.rs b/src/publisher.rs index 3cd675b..2fb65d9 100644 --- a/src/publisher.rs +++ b/src/publisher.rs @@ -2,16 +2,21 @@ use anyhow::{Context, Result, bail}; use std::path::{Path, PathBuf}; use std::process::Command; +/// Pacman's system-wide config — hardcoded like the rest of this tool's +/// Arch/Manjaro-specific assumptions (see SPEC.md > Scope). +const PACMAN_CONF: &str = "/etc/pacman.conf"; + /// Copies the built package into `repo_dir` and runs `repo-add` against -/// `.db.tar.gz` there. +/// `.db.tar.gz` there. Creates `repo_dir` if it doesn't exist +/// yet — `repo-add` itself creates the database file on its first run, so +/// everything filesystem-side is self-healing. /// -/// `repo_dir`/`repo_name` are expected to already be a real, registered -/// pacman repo (see SPEC.md > Architecture > Publisher and -/// `/etc/pacman.conf`'s `[custom]` section on this box) — pkgwatch doesn't -/// create the repo or touch pacman.conf, only adds packages to an -/// already-registered one. Getting the new version into an installed -/// system is a separate, deliberate `pacman -Syu`/`pacman -S` step left to -/// the operator, not run automatically here. +/// What is *not* self-healing, and can't safely be: registering +/// `repo_name` in `/etc/pacman.conf` (see `ensure_registered`) — that +/// needs root, which this process doesn't have and shouldn't grab for +/// itself. Getting a published version onto the running system is a +/// separate, deliberate `pacman -Syu`/`pacman -S` step too, also left to +/// the operator. pub fn publish(package_path: &Path, repo_dir: &Path, repo_name: &str) -> Result { publish_with(Path::new("repo-add"), package_path, repo_dir, repo_name) } @@ -51,6 +56,38 @@ fn publish_with( Ok(dest) } +/// Verifies `repo_name` is registered as an active `[section]` in +/// `/etc/pacman.conf`, so a build isn't wasted on a repo pacman will never +/// actually sync from. Call this before `publish` — ideally before even +/// starting the build, so a missing repo fails fast instead of after +/// several seconds of `makepkg` work. +/// +/// Doesn't check that the section's `Server =`/`Include =` line points at +/// `repo_dir` specifically — just that a repo by this name exists at all. +/// A same-named repo pointed somewhere else is a rare, easily-diagnosed +/// misconfiguration, not worth the parsing complexity to catch here. +pub fn ensure_registered(repo_name: &str, repo_dir: &Path) -> Result<()> { + ensure_registered_at(Path::new(PACMAN_CONF), repo_name, repo_dir) +} + +fn ensure_registered_at(pacman_conf: &Path, repo_name: &str, repo_dir: &Path) -> Result<()> { + let conf = std::fs::read_to_string(pacman_conf) + .with_context(|| format!("reading {}", pacman_conf.display()))?; + let header = format!("[{repo_name}]"); + let registered = conf.lines().map(str::trim).any(|line| line == header); + if !registered { + bail!( + "'{repo_name}' is not registered in {} — add this once, as root, then re-run:\n\n\ + [{repo_name}]\n\ + SigLevel = Optional TrustAll\n\ + Server = file://{}\n", + pacman_conf.display(), + repo_dir.display() + ); + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -120,4 +157,47 @@ mod tests { publish_with(&repo_add, &package_path, &repo_dir, "custom").unwrap(); assert!(repo_dir.join("thing-1.0.0-1-x86_64.pkg.tar.zst").exists()); } + + fn write_pacman_conf(dir: &Path, contents: &str) -> PathBuf { + let path = dir.join("pacman.conf"); + std::fs::write(&path, contents).unwrap(); + path + } + + #[test] + fn ensure_registered_passes_when_section_present() { + let dir = tempfile::tempdir().unwrap(); + let conf = write_pacman_conf( + dir.path(), + "[options]\nArchitecture = auto\n\n[extra]\nInclude = /etc/pacman.d/mirrorlist\n\n[custom]\nSigLevel = Optional TrustAll\nServer = file:///home/austin/.local/share/pacman/custom\n", + ); + assert!(ensure_registered_at(&conf, "custom", Path::new("/repo")).is_ok()); + } + + #[test] + fn ensure_registered_fails_when_section_missing() { + let dir = tempfile::tempdir().unwrap(); + let conf = write_pacman_conf(dir.path(), "[options]\nArchitecture = auto\n\n[extra]\n"); + let err = ensure_registered_at(&conf, "custom", Path::new("/repo")).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("not registered")); + assert!(msg.contains("[custom]")); + assert!(msg.contains("/repo")); + } + + #[test] + fn ensure_registered_does_not_match_substring_of_another_section() { + // "custom" must match the whole section header, not just appear + // as a substring of e.g. "[custom-extra]". + let dir = tempfile::tempdir().unwrap(); + let conf = write_pacman_conf(dir.path(), "[custom-extra]\nServer = file:///elsewhere\n"); + assert!(ensure_registered_at(&conf, "custom", Path::new("/repo")).is_err()); + } + + #[test] + fn ensure_registered_errors_when_pacman_conf_missing() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("does-not-exist.conf"); + assert!(ensure_registered_at(&missing, "custom", Path::new("/repo")).is_err()); + } } -- 2.45.2 From 3f30e0acc6d0f79a86053aa417ebe0e15ce05c61 Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Thu, 17 Sep 2026 11:42:18 +0200 Subject: [PATCH 3/6] Add ARCHITECTURE.md and apply it to this PR's code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Researched current industry practice on code organization/maintainability (Ousterhout's deep modules and information hiding, package-by-feature vs. package-by-layer, functional-core/imperative-shell testability, tech-debt prevention via ADR-equivalent inline rationale) and wrote it into ARCHITECTURE.md as a set of concrete, project-specific rules rather than a generic essay — each principle cites a real example already in this codebase or fixed by this commit. Cross-linked from SPEC.md, which stays about product design, not code organization. Applied it to this PR's own code: - Pulled process_package/fetch_and_verify/build_and_publish/run_review/ approve out of main.rs into a new pipeline.rs. main.rs's own main() had grown to 278 lines and zero tests by treating "it's just the entry point" as an excuse to skip separating logic from wiring; now main.rs is argv dispatch only. - Extracted decide_tier_action as a pure function (verification outcome + pending-state -> what to do), replacing dispatch logic that was previously inlined into a function that also made the real network/ build calls. Four unit tests, no I/O, covering all four outcomes. - Added a `//!` module doc comment to every file touched in this branch, each stating that module's one job in a sentence, per the "deep modules" principle the spec argues for. Coverage's reported total drops (94% -> 78%) because pipeline.rs is deliberately NOT excluded from it the way main.rs is, even though it's mostly the same kind of untestable I/O orchestration — excluding it would hide decide_tier_action's real unit-test coverage along with the untested parts. Noted inline in Makefile.toml/ci.yml so the number doesn't look like a quality regression at a glance. Also added a project reference memory pointing at ARCHITECTURE.md rather than duplicating its content there, per this session's own memory-hygiene rules (architecture/conventions are derivable from the repo and shouldn't be duplicated somewhere that can go stale). Co-Authored-By: Claude Sonnet 5 --- .forgejo/workflows/ci.yml | 7 +- ARCHITECTURE.md | 131 +++++++++++++++ Makefile.toml | 10 +- SPEC.md | 4 + src/builder.rs | 4 + src/config.rs | 4 + src/fetcher.rs | 4 + src/hash.rs | 4 + src/main.rs | 269 ++---------------------------- src/pipeline.rs | 341 ++++++++++++++++++++++++++++++++++++++ src/publisher.rs | 5 + src/sanity.rs | 4 + src/state.rs | 4 + src/verifier.rs | 5 + 14 files changed, 533 insertions(+), 263 deletions(-) create mode 100644 ARCHITECTURE.md create mode 100644 src/pipeline.rs diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index ba4dc93..499c98d 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -110,9 +110,10 @@ jobs: command -v cargo-llvm-cov >/dev/null 2>&1 || cargo install cargo-llvm-cov --locked # Reports coverage only — no --fail-under-lines yet. main.rs is - # excluded: thin orchestration glue exercised by the real end-to-end - # `cargo run` against live GitHub, not unit tests, so it's not a - # meaningful signal here. See Makefile.toml > coverage-report. + # excluded: thin argv dispatch exercised by the real end-to-end + # `cargo run`, not unit tests, so it's not a meaningful signal here. + # See Makefile.toml > coverage-report for why pipeline.rs, despite + # being mostly untestable I/O orchestration too, stays included. - name: Coverage run: cargo llvm-cov --ignore-filename-regex 'main\.rs' --summary-only diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md new file mode 100644 index 0000000..df2058c --- /dev/null +++ b/ARCHITECTURE.md @@ -0,0 +1,131 @@ +# pkgwatch — code organization + +Status: written 2026-09-17, once the build/publish pipeline PR gave this +project enough real code to have actual conventions worth writing down, +instead of guessing at them in advance. + +This is distinct from `SPEC.md`, which is the product design (what +pkgwatch does and why). This file is about how the *code* implementing +that design is organized, so it stays readable as it grows past PoC size +instead of quietly accumulating debt. Researched against current industry +practice rather than asserted from habit — see Further reading. + +## Principles + +1. **One module, one job — and say what it is, up front.** + Ousterhout's "deep modules": the best modules expose a lot of + functionality through a simple interface, hiding the complexity behind + it. The two failure modes he names — *change amplification* (one + conceptual change forces edits in many places) and *obscurity* (a + reader can't tell where responsibility lives) — are both symptoms of + modules that don't have one clear job. + **Rule**: every `src/*.rs` file opens with a `//!` doc comment stating + its one responsibility in a sentence. If it can't be one sentence, the + module is doing too much. + **Example already here**: `builder.rs`'s job is "turn an + already-downloaded, already-verified artifact into a built package." It + hides PKGBUILD templating, upstream-string validation, and the + `makepkg` invocation behind one `build()` call — none of that leaks to + callers. + +2. **Organize by pipeline stage (feature), not by technical layer.** + The package-by-feature vs. package-by-layer research is consistent: + feature-based grouping gives high cohesion within a module and low + coupling between modules; layer-based grouping (`models/`, `utils/`, + `helpers/`) tends toward the opposite, and a single feature change ends + up touching files scattered across every layer. + **Rule**: modules are named after what they do in the pipeline + (`checker`, `fetcher`, `verifier`, `builder`, `sanity`, `publisher`, + `state`), not generic buckets. A new pipeline stage gets a new module + named after the stage, not a method bolted onto an existing one. + **Anti-example to keep watching for**: a `utils.rs` grab-bag. `hash.rs` + could look like one but isn't — it exists for exactly one piece of + shared logic (`sha256_hex`) that two real stages (`verifier`, + `builder`) both need, not as a place to dump unrelated helpers. + +3. **Separate pure decision logic from I/O ("functional core, imperative + shell").** + A function that decides *and* does in the same body can't be tested + without standing up everything the "does" half touches — often a + network call, a subprocess, or the filesystem. Pulling the decision out + into its own pure function makes it trivially unit-testable and makes + the I/O half thin enough that it obviously matches the decision. + **Applied this PR**: `pipeline::process_package`'s tier dispatch + (publish now / still pending / newly pending / verification failed) was + originally inline in a function that also made the real network and + build calls. Pulled out into `decide_tier_action`, a pure function with + its own unit tests covering all four outcomes, no I/O involved. + +4. **Every network, subprocess, filesystem-root, or environment boundary + is injectable.** + Same testability goal as #3, applied to the specific ways this program + reaches outside itself. A consistent shape beats ad hoc mocking invented + per call site. + **Already in force**: `GithubEndpoints` (checker/fetcher/verifier), + `repo_add_bin` and the pacman.conf path (publisher), `PKGWATCH_REPO_DIR` + (main, for manual dry runs against a scratch repo instead of the real + one). A new external call follows the same shape: production code calls + a thin wrapper with the real default; tests call the parameterized + version with a fake. + +5. **`main.rs` is a dispatcher, not the program.** + Found by looking at this project's own `main.rs`: it grew to 278 lines + and zero tests over the course of one PR, because "it's just the entry + point" is an easy excuse to skip separating logic from wiring — even + though Rust doesn't actually stop you from unit-testing a binary + crate's `main.rs`. The Rust community convention of splitting + entry-point parsing from application logic exists precisely so the + logic ends up somewhere it's normal to test. + **Rule**: `main.rs` may parse `argv`, build shared clients, and print + output. It must not contain a pipeline decision, a network/subprocess + call, or anything with a test worth writing — that belongs in + `pipeline.rs`. + **Applied this PR**: moved `process_package`, `fetch_and_verify`, + `build_and_publish`, `run_review`, and `approve` out of `main.rs` into a + new `pipeline.rs`, leaving `main.rs` as argument dispatch only. + +6. **Validate at the boundary, once — don't scatter checks.** + Already stated project-wide (see the user's global instructions: don't + validate scenarios that can't happen, validate at system boundaries). + **Example already here**: `builder.rs`'s `validate_pkgname`/ + `validate_pkgver`/`validate_shell_safe` run once, at PKGBUILD-generation + time, against every upstream-controlled string — not sprinkled through + whatever code happens to produce those strings. + +7. **Don't build generality the currently-tracked packages don't need.** + Already the load-bearing design principle in `SPEC.md` ("a small fixed + set of PKGBUILD shapes," "extend when a third real shape shows up"). + Restated here because it's also a tech-debt principle in its own right: + speculative abstraction is debt too — every future reader has to + understand it whether or not it's ever exercised. + +8. **Every non-obvious structural decision gets one sentence of "why," + inline.** + Standard tech-debt-prevention advice is to keep Architecture Decision + Records; a single-crate personal tool doesn't need a `docs/adr/` + directory, but the same information — why this way and not the obvious + alternative — needs to live somewhere a future reader will actually see + it: the doc comment on the thing itself. + **Example already here**: `checker.rs`'s doc comment on + `latest_github_release` explains why the newest Atom-feed entry isn't + trusted outright (scaleway-cli's `-dbg1` tag has no real Release behind + it) — the reasoning lives right next to the code it justifies, not in a + commit message or a separate design doc no one will find later. + +## What's machine-enforced vs. what isn't + +`cargo make ci` (format, clippy, cognitive-complexity threshold, coverage, +audit) mechanically enforces what's checkable: style, a handful of lint +categories, a complexity ceiling, and that coverage doesn't quietly +regress. It does **not** enforce module cohesion, naming, or "is this +logic in the right module" — those stay code-review questions. Worth +being honest about that boundary rather than implying CI catches +everything above. + +## Further reading + +- [A Philosophy of Software Design — deep modules & information hiding, summary](https://medium.com/swlh/a-philosophy-of-software-design-by-john-ousterhout-4a00d0ff9f1c) +- [Package by feature vs. package by layer](https://medium.com/@felixnjunge78/package-by-feature-vs-package-by-layer-which-one-wins-11ee03921fed) +- [Coupling and cohesion as the foundations of a maintainable codebase](https://medium.com/@iamprovidence/coupling-and-cohesion-foundations-that-affect-your-entire-codebase-77d06d44af0d) +- [Rust module and crate organization best practices](https://softwarepatternslexicon.com/rust/idiomatic-rust-patterns/module-and-crate-organization-best-practices/) +- [Reducing technical debt in 2026 — IBM](https://www.ibm.com/think/insights/reduce-technical-debt) diff --git a/Makefile.toml b/Makefile.toml index e7304d1..bd75029 100644 --- a/Makefile.toml +++ b/Makefile.toml @@ -30,9 +30,15 @@ args = ["test"] # same-named custom one. # # Reports coverage only; not gated on a threshold yet — main.rs is thin -# orchestration glue exercised by the real end-to-end `cargo run`, not unit +# argv dispatch exercised by the real end-to-end `cargo run`, not unit # tests, so it's excluded here rather than dragging the number down for -# reasons unrelated to test quality. +# reasons unrelated to test quality. pipeline.rs is deliberately NOT +# excluded even though it's mostly network/subprocess/filesystem +# orchestration too (hence its own low number) — its one pure decision +# function (decide_tier_action) is unit tested and should stay visible in +# this report; excluding the whole file would hide that signal along with +# the untested parts. See ARCHITECTURE.md > "separate pure decision logic +# from I/O." [tasks.coverage-report] command = "cargo" args = ["llvm-cov", "--ignore-filename-regex", "main\\.rs", "--summary-only"] diff --git a/SPEC.md b/SPEC.md index b793611..9e341a8 100644 --- a/SPEC.md +++ b/SPEC.md @@ -2,6 +2,10 @@ Status: design draft, pre-PoC. Captures the design discussion as of 2026-09-11. +This is the *product* design — what pkgwatch does and why. For how the +code implementing it is organized (module boundaries, testability +conventions, what CI does and doesn't enforce), see `ARCHITECTURE.md`. + ## Problem Software not packaged by the distro (Arch/Manjaro here) usually gets installed diff --git a/src/builder.rs b/src/builder.rs index 0228649..fd5ca55 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1,3 +1,7 @@ +//! Turns an already-downloaded, already-verified artifact into a built +//! pacman package: generates a PKGBUILD, then runs `makepkg`. Hides all +//! PKGBUILD templating and upstream-string validation behind `build()`. + use crate::config::Package; use crate::hash::sha256_hex; use anyhow::{Context, Result, bail}; diff --git a/src/config.rs b/src/config.rs index 1804bf5..e8c5874 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,3 +1,7 @@ +//! Parses `packages.d/*.toml` into typed, in-memory `Package` records. +//! The only module that knows the TOML shape — everything downstream +//! works with `Package`/`Verification`/`SanityCheck`, never raw TOML. + use anyhow::{Context, Result}; use serde::Deserialize; use std::collections::HashMap; diff --git a/src/fetcher.rs b/src/fetcher.rs index 3ef4e11..2614258 100644 --- a/src/fetcher.rs +++ b/src/fetcher.rs @@ -1,3 +1,7 @@ +//! Downloads a named GitHub release asset to a local path. The only +//! module that talks to the releases API for asset bytes — `checker` only +//! resolves version tags, never downloads. + use crate::github::GithubEndpoints; use anyhow::{Context, Result}; use serde::Deserialize; diff --git a/src/hash.rs b/src/hash.rs index 7320600..6406360 100644 --- a/src/hash.rs +++ b/src/hash.rs @@ -1,3 +1,7 @@ +//! One function, shared by two real callers (`verifier`, `builder`) — +//! not a general-purpose utils dump. See ARCHITECTURE.md > "organize by +//! pipeline stage, not by layer" for why that distinction matters. + use sha2::{Digest, Sha256}; /// Shared by `verifier` (same-origin-sha256 checks) and `builder` (every diff --git a/src/main.rs b/src/main.rs index 73885a5..61796cc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,278 +1,31 @@ +//! Entry point: parses `argv` and dispatches to `pipeline`. Nothing here +//! makes a network/subprocess call or contains a decision worth a test — +//! see ARCHITECTURE.md > "main is a dispatcher, not the program." + mod builder; mod checker; mod config; mod fetcher; mod github; mod hash; +mod pipeline; mod publisher; mod sanity; mod state; mod verifier; -use anyhow::{Context, Result, bail}; -use config::Package; -use fetcher::DownloadedAsset; -use github::GithubEndpoints; -use std::path::{Path, PathBuf}; -use verifier::VerificationResult; - -const PACKAGES_DIR: &str = "packages.d"; -const STATE_DIR: &str = "state"; -const WORK_DIR: &str = "work"; -/// Not a repo pkgwatch invents: this is the existing, already-registered -/// local pacman repo on this box (see `[custom]` in /etc/pacman.conf and -/// its `Server = file://...` line). pkgwatch adds packages to it; it does -/// not create the repo or touch pacman.conf. -const CUSTOM_REPO_NAME: &str = "custom"; -const CUSTOM_REPO_SUBPATH: &str = ".local/share/pacman/custom"; +use anyhow::{Result, bail}; /// check -> fetch -> verify -> build -> sanity-check -> publish, for /// whatever is in packages.d/. Tier 1-3 passes auto-publish; tier 4-6 -/// passes queue for `pkgwatch review`. See SPEC.md > Architecture. +/// passes queue for `pkgwatch review`. See SPEC.md > Architecture for what +/// each stage does, and ARCHITECTURE.md for how the code implementing it +/// is organized. fn main() -> Result<()> { let args: Vec = std::env::args().skip(1).collect(); match args.first().map(String::as_str) { - None => run_check(), - Some("review") => run_review(&args[1..]), + None => pipeline::run_check(), + Some("review") => pipeline::run_review(&args[1..]), Some(other) => bail!("unknown subcommand '{other}' (expected: review)"), } } - -fn build_client() -> Result { - Ok(reqwest::blocking::Client::builder() - .user_agent("pkgwatch/0.1 (PoC; https://code.austinschaefer.com)") - .build()?) -} - -fn custom_repo_dir() -> Result { - // Override for testing against a scratch repo instead of the real one - // at $HOME/.local/share/pacman/custom. - if let Ok(dir) = std::env::var("PKGWATCH_REPO_DIR") { - return Ok(PathBuf::from(dir)); - } - let home = std::env::var("HOME").context("HOME is not set")?; - Ok(Path::new(&home).join(CUSTOM_REPO_SUBPATH)) -} - -fn run_check() -> Result<()> { - let client = build_client()?; - let endpoints = GithubEndpoints::default(); - let packages_dir = Path::new(PACKAGES_DIR); - let state_dir = Path::new(STATE_DIR); - let work_dir = Path::new(WORK_DIR); - - let packages = config::load_packages_dir(packages_dir)?; - if packages.is_empty() { - println!("no packages configured under {}/", packages_dir.display()); - return Ok(()); - } - - let mut any_failed = false; - for (name, pkg) in &packages { - println!("== {name} ({}) ==", pkg.repo); - if let Err(err) = process_package(&client, &endpoints, state_dir, work_dir, name, pkg) { - eprintln!(" error: {err:#}"); - any_failed = true; - } - } - - if any_failed { - bail!("one or more packages failed — see errors above"); - } - Ok(()) -} - -fn process_package( - client: &reqwest::blocking::Client, - endpoints: &GithubEndpoints, - state_dir: &Path, - work_dir: &Path, - name: &str, - pkg: &Package, -) -> Result<()> { - let latest = checker::latest_github_release(client, endpoints, &pkg.repo)?; - let last_seen = state::load_last_version(state_dir, name); - if last_seen.as_deref() == Some(latest.as_str()) { - println!(" up to date at {latest}"); - return Ok(()); - } - println!(" new version detected: {latest} (previously: {last_seen:?})"); - - let fetched = fetch_and_verify(client, endpoints, work_dir, name, pkg, &latest)?; - println!(" fetched {}", fetched.asset.path.display()); - println!( - " verification (tier {}): {} — {}", - fetched.verification.tier, - if fetched.verification.passed { - "PASS" - } else { - "FAIL" - }, - fetched.verification.justification - ); - - if !fetched.verification.passed { - println!(" verification failed — not publishing, not updating state"); - return Ok(()); - } - - if fetched.verification.tier <= 3 { - println!(" tier 1-3 pass: building + publishing"); - build_and_publish(name, pkg, &latest, &fetched)?; - state::save_last_version(state_dir, name, &latest)?; - state::clear_pending_version(state_dir, name)?; - println!(" published {name} {latest}"); - } else if state::load_pending_version(state_dir, name).as_deref() == Some(latest.as_str()) { - println!(" tier 4-6 pass: still pending review (`pkgwatch review` to see it)"); - } else { - state::save_pending_version(state_dir, name, &latest)?; - println!(" tier 4-6 pass: flagged for human review (`pkgwatch review` to approve)"); - } - Ok(()) -} - -struct FetchVerifyResult { - version: String, - asset_name: String, - dest_dir: PathBuf, - asset: DownloadedAsset, - verification: VerificationResult, -} - -/// Shared by the normal check loop (tier 1-3 auto-path) and `pkgwatch -/// review --approve` (which re-verifies before publishing rather than -/// trusting a possibly-stale flag from an earlier run). -fn fetch_and_verify( - client: &reqwest::blocking::Client, - endpoints: &GithubEndpoints, - work_dir: &Path, - name: &str, - pkg: &Package, - tag: &str, -) -> Result { - let version = checker::version_from_tag(tag).to_string(); - let asset_name = pkg.asset_pattern.replace("{version}", &version); - let dest_dir = work_dir.join(name).join(tag); - - let asset = fetcher::download_asset(client, endpoints, &pkg.repo, tag, &asset_name, &dest_dir)?; - let verification = verifier::verify( - client, - endpoints, - &pkg.verification, - &pkg.repo, - tag, - &asset.path, - &dest_dir, - )?; - - Ok(FetchVerifyResult { - version, - asset_name, - dest_dir, - asset, - verification, - }) -} - -fn build_and_publish( - name: &str, - pkg: &Package, - tag: &str, - fetched: &FetchVerifyResult, -) -> Result<()> { - // Fail fast if the repo isn't registered in pacman.conf, before - // spending several seconds on a makepkg build that would otherwise - // succeed and then publish somewhere pacman never syncs from. - let repo_dir = custom_repo_dir()?; - publisher::ensure_registered(CUSTOM_REPO_NAME, &repo_dir)?; - - let build_dir = fetched.dest_dir.join("build"); - let req = builder::BuildRequest { - pkg_name: name, - pkg, - version: &fetched.version, - repo: &pkg.repo, - asset_name: &fetched.asset_name, - download_url: &fetched.asset.download_url, - artifact_path: &fetched.asset.path, - }; - let built = builder::build(&req, &build_dir)?; - println!(" built {}", built.package_path.display()); - - if let Some(check) = &pkg.sanity_check { - let bin_dir = built.pkgdir.join("usr/bin"); - sanity::run(check, &bin_dir, &fetched.version) - .with_context(|| format!("sanity check for {name} {tag}"))?; - println!(" sanity check passed"); - } - - let published = publisher::publish(&built.package_path, &repo_dir, CUSTOM_REPO_NAME)?; - println!( - " added to {} repo: {}", - CUSTOM_REPO_NAME, - published.display() - ); - println!( - " not installed automatically — run `sudo pacman -Syu` (or `sudo pacman -S {name}`) to pick it up" - ); - Ok(()) -} - -fn run_review(args: &[String]) -> Result<()> { - let packages_dir = Path::new(PACKAGES_DIR); - let state_dir = Path::new(STATE_DIR); - let work_dir = Path::new(WORK_DIR); - let packages = config::load_packages_dir(packages_dir)?; - - match args { - [] => { - let mut any = false; - for (name, _) in &packages { - if let Some(pending) = state::load_pending_version(state_dir, name) { - println!( - "{name}: {pending} pending review (run `pkgwatch review {name} --approve`)" - ); - any = true; - } - } - if !any { - println!("no packages pending review"); - } - Ok(()) - } - [name, flag] if flag == "--approve" => { - let (_, pkg) = packages.iter().find(|(n, _)| n == name).with_context(|| { - format!("no package named '{name}' in {}/", packages_dir.display()) - })?; - let tag = state::load_pending_version(state_dir, name) - .with_context(|| format!("'{name}' has no pending review"))?; - approve(state_dir, work_dir, name, pkg, &tag) - } - _ => bail!("usage: pkgwatch review [ --approve]"), - } -} - -fn approve(state_dir: &Path, work_dir: &Path, name: &str, pkg: &Package, tag: &str) -> Result<()> { - let client = build_client()?; - let endpoints = GithubEndpoints::default(); - - // Re-verify rather than trusting the earlier flag: the artifact at - // this tag could in principle have changed since it was queued. - let fetched = fetch_and_verify(&client, &endpoints, work_dir, name, pkg, tag)?; - if !fetched.verification.passed { - bail!( - "re-verification failed on approve: {}", - fetched.verification.justification - ); - } - println!( - "re-verified (tier {}): {}", - fetched.verification.tier, fetched.verification.justification - ); - - build_and_publish(name, pkg, tag, &fetched)?; - state::save_last_version(state_dir, name, tag)?; - state::clear_pending_version(state_dir, name)?; - println!("approved and published {name} {tag}"); - Ok(()) -} diff --git a/src/pipeline.rs b/src/pipeline.rs new file mode 100644 index 0000000..cabf31d --- /dev/null +++ b/src/pipeline.rs @@ -0,0 +1,341 @@ +//! Orchestrates one run of check -> fetch -> verify -> build -> +//! sanity-check -> publish across every configured package, plus the +//! `review` subcommand for tier 4-6 approvals. The only module that calls +//! more than one other pipeline-stage module — see ARCHITECTURE.md > "main +//! is a dispatcher, not the program" for why this lives here and not in +//! `main.rs`. + +use crate::builder; +use crate::checker; +use crate::config::{self, Package}; +use crate::fetcher::{self, DownloadedAsset}; +use crate::github::GithubEndpoints; +use crate::publisher; +use crate::sanity; +use crate::state; +use crate::verifier::{self, VerificationResult}; +use anyhow::{Context, Result, bail}; +use std::path::{Path, PathBuf}; + +const PACKAGES_DIR: &str = "packages.d"; +const STATE_DIR: &str = "state"; +const WORK_DIR: &str = "work"; +/// Not a repo pkgwatch invents: this is the existing, already-registered +/// local pacman repo on this box (see `[custom]` in /etc/pacman.conf and +/// its `Server = file://...` line). pkgwatch adds packages to it; it does +/// not create the repo or touch pacman.conf. +const CUSTOM_REPO_NAME: &str = "custom"; +const CUSTOM_REPO_SUBPATH: &str = ".local/share/pacman/custom"; + +fn build_client() -> Result { + Ok(reqwest::blocking::Client::builder() + .user_agent("pkgwatch/0.1 (PoC; https://code.austinschaefer.com)") + .build()?) +} + +fn custom_repo_dir() -> Result { + // Override for testing against a scratch repo instead of the real one + // at $HOME/.local/share/pacman/custom. + if let Ok(dir) = std::env::var("PKGWATCH_REPO_DIR") { + return Ok(PathBuf::from(dir)); + } + let home = std::env::var("HOME").context("HOME is not set")?; + Ok(Path::new(&home).join(CUSTOM_REPO_SUBPATH)) +} + +pub fn run_check() -> Result<()> { + let client = build_client()?; + let endpoints = GithubEndpoints::default(); + let packages_dir = Path::new(PACKAGES_DIR); + let state_dir = Path::new(STATE_DIR); + let work_dir = Path::new(WORK_DIR); + + let packages = config::load_packages_dir(packages_dir)?; + if packages.is_empty() { + println!("no packages configured under {}/", packages_dir.display()); + return Ok(()); + } + + let mut any_failed = false; + for (name, pkg) in &packages { + println!("== {name} ({}) ==", pkg.repo); + if let Err(err) = process_package(&client, &endpoints, state_dir, work_dir, name, pkg) { + eprintln!(" error: {err:#}"); + any_failed = true; + } + } + + if any_failed { + bail!("one or more packages failed — see errors above"); + } + Ok(()) +} + +/// What to do about a package after verification, derived purely from the +/// verification outcome and whether this exact version is already queued +/// for review — no I/O. See ARCHITECTURE.md > "separate pure decision +/// logic from I/O." +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TierAction { + /// Verification failed outright — don't build, don't touch state. + VerificationFailed, + /// Tier 1-3: safe to auto-build and publish immediately. + Publish, + /// Tier 4-6, and this exact version was already flagged on an earlier + /// run — nothing new to report. + StillPending, + /// Tier 4-6, and this version hasn't been flagged yet. + NewlyPending, +} + +fn decide_tier_action(tier: u8, passed: bool, already_pending_this_version: bool) -> TierAction { + if !passed { + return TierAction::VerificationFailed; + } + if tier <= 3 { + return TierAction::Publish; + } + if already_pending_this_version { + TierAction::StillPending + } else { + TierAction::NewlyPending + } +} + +fn process_package( + client: &reqwest::blocking::Client, + endpoints: &GithubEndpoints, + state_dir: &Path, + work_dir: &Path, + name: &str, + pkg: &Package, +) -> Result<()> { + let latest = checker::latest_github_release(client, endpoints, &pkg.repo)?; + let last_seen = state::load_last_version(state_dir, name); + if last_seen.as_deref() == Some(latest.as_str()) { + println!(" up to date at {latest}"); + return Ok(()); + } + println!(" new version detected: {latest} (previously: {last_seen:?})"); + + let fetched = fetch_and_verify(client, endpoints, work_dir, name, pkg, &latest)?; + println!(" fetched {}", fetched.asset.path.display()); + println!( + " verification (tier {}): {} — {}", + fetched.verification.tier, + if fetched.verification.passed { + "PASS" + } else { + "FAIL" + }, + fetched.verification.justification + ); + + let already_pending = + state::load_pending_version(state_dir, name).as_deref() == Some(latest.as_str()); + match decide_tier_action( + fetched.verification.tier, + fetched.verification.passed, + already_pending, + ) { + TierAction::VerificationFailed => { + println!(" verification failed — not publishing, not updating state"); + } + TierAction::Publish => { + println!(" tier 1-3 pass: building + publishing"); + build_and_publish(name, pkg, &latest, &fetched)?; + state::save_last_version(state_dir, name, &latest)?; + state::clear_pending_version(state_dir, name)?; + println!(" published {name} {latest}"); + } + TierAction::StillPending => { + println!(" tier 4-6 pass: still pending review (`pkgwatch review` to see it)"); + } + TierAction::NewlyPending => { + state::save_pending_version(state_dir, name, &latest)?; + println!(" tier 4-6 pass: flagged for human review (`pkgwatch review` to approve)"); + } + } + Ok(()) +} + +struct FetchVerifyResult { + version: String, + asset_name: String, + dest_dir: PathBuf, + asset: DownloadedAsset, + verification: VerificationResult, +} + +/// Shared by the normal check loop (tier 1-3 auto-path) and `pkgwatch +/// review --approve` (which re-verifies before publishing rather than +/// trusting a possibly-stale flag from an earlier run). +fn fetch_and_verify( + client: &reqwest::blocking::Client, + endpoints: &GithubEndpoints, + work_dir: &Path, + name: &str, + pkg: &Package, + tag: &str, +) -> Result { + let version = checker::version_from_tag(tag).to_string(); + let asset_name = pkg.asset_pattern.replace("{version}", &version); + let dest_dir = work_dir.join(name).join(tag); + + let asset = fetcher::download_asset(client, endpoints, &pkg.repo, tag, &asset_name, &dest_dir)?; + let verification = verifier::verify( + client, + endpoints, + &pkg.verification, + &pkg.repo, + tag, + &asset.path, + &dest_dir, + )?; + + Ok(FetchVerifyResult { + version, + asset_name, + dest_dir, + asset, + verification, + }) +} + +fn build_and_publish( + name: &str, + pkg: &Package, + tag: &str, + fetched: &FetchVerifyResult, +) -> Result<()> { + // Fail fast if the repo isn't registered in pacman.conf, before + // spending several seconds on a makepkg build that would otherwise + // succeed and then publish somewhere pacman never syncs from. + let repo_dir = custom_repo_dir()?; + publisher::ensure_registered(CUSTOM_REPO_NAME, &repo_dir)?; + + let build_dir = fetched.dest_dir.join("build"); + let req = builder::BuildRequest { + pkg_name: name, + pkg, + version: &fetched.version, + repo: &pkg.repo, + asset_name: &fetched.asset_name, + download_url: &fetched.asset.download_url, + artifact_path: &fetched.asset.path, + }; + let built = builder::build(&req, &build_dir)?; + println!(" built {}", built.package_path.display()); + + if let Some(check) = &pkg.sanity_check { + let bin_dir = built.pkgdir.join("usr/bin"); + sanity::run(check, &bin_dir, &fetched.version) + .with_context(|| format!("sanity check for {name} {tag}"))?; + println!(" sanity check passed"); + } + + let published = publisher::publish(&built.package_path, &repo_dir, CUSTOM_REPO_NAME)?; + println!( + " added to {} repo: {}", + CUSTOM_REPO_NAME, + published.display() + ); + println!( + " not installed automatically — run `sudo pacman -Syu` (or `sudo pacman -S {name}`) to pick it up" + ); + Ok(()) +} + +pub fn run_review(args: &[String]) -> Result<()> { + let packages_dir = Path::new(PACKAGES_DIR); + let state_dir = Path::new(STATE_DIR); + let work_dir = Path::new(WORK_DIR); + let packages = config::load_packages_dir(packages_dir)?; + + match args { + [] => { + let mut any = false; + for (name, _) in &packages { + if let Some(pending) = state::load_pending_version(state_dir, name) { + println!( + "{name}: {pending} pending review (run `pkgwatch review {name} --approve`)" + ); + any = true; + } + } + if !any { + println!("no packages pending review"); + } + Ok(()) + } + [name, flag] if flag == "--approve" => { + let (_, pkg) = packages.iter().find(|(n, _)| n == name).with_context(|| { + format!("no package named '{name}' in {}/", packages_dir.display()) + })?; + let tag = state::load_pending_version(state_dir, name) + .with_context(|| format!("'{name}' has no pending review"))?; + approve(state_dir, work_dir, name, pkg, &tag) + } + _ => bail!("usage: pkgwatch review [ --approve]"), + } +} + +fn approve(state_dir: &Path, work_dir: &Path, name: &str, pkg: &Package, tag: &str) -> Result<()> { + let client = build_client()?; + let endpoints = GithubEndpoints::default(); + + // Re-verify rather than trusting the earlier flag: the artifact at + // this tag could in principle have changed since it was queued. + let fetched = fetch_and_verify(&client, &endpoints, work_dir, name, pkg, tag)?; + if !fetched.verification.passed { + bail!( + "re-verification failed on approve: {}", + fetched.verification.justification + ); + } + println!( + "re-verified (tier {}): {}", + fetched.verification.tier, fetched.verification.justification + ); + + build_and_publish(name, pkg, tag, &fetched)?; + state::save_last_version(state_dir, name, tag)?; + state::clear_pending_version(state_dir, name)?; + println!("approved and published {name} {tag}"); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn decide_tier_action_failed_verification_overrides_everything() { + assert_eq!( + decide_tier_action(2, false, false), + TierAction::VerificationFailed + ); + assert_eq!( + decide_tier_action(4, false, true), + TierAction::VerificationFailed + ); + } + + #[test] + fn decide_tier_action_tier_1_to_3_publishes() { + assert_eq!(decide_tier_action(1, true, false), TierAction::Publish); + assert_eq!(decide_tier_action(2, true, false), TierAction::Publish); + assert_eq!(decide_tier_action(3, true, true), TierAction::Publish); + } + + #[test] + fn decide_tier_action_tier_4_to_6_newly_pending_when_not_seen_before() { + assert_eq!(decide_tier_action(4, true, false), TierAction::NewlyPending); + assert_eq!(decide_tier_action(6, true, false), TierAction::NewlyPending); + } + + #[test] + fn decide_tier_action_tier_4_to_6_still_pending_when_already_flagged() { + assert_eq!(decide_tier_action(4, true, true), TierAction::StillPending); + } +} diff --git a/src/publisher.rs b/src/publisher.rs index 2fb65d9..f14c95b 100644 --- a/src/publisher.rs +++ b/src/publisher.rs @@ -1,3 +1,8 @@ +//! Gets a built package into the local pacman repo: copies it in, runs +//! `repo-add`, and checks the repo is actually registered in +//! `/etc/pacman.conf` first. The only module that touches the repo +//! directory or pacman.conf. + use anyhow::{Context, Result, bail}; use std::path::{Path, PathBuf}; use std::process::Command; diff --git a/src/sanity.rs b/src/sanity.rs index cc93f9d..4aab514 100644 --- a/src/sanity.rs +++ b/src/sanity.rs @@ -1,3 +1,7 @@ +//! Post-build correctness check: runs the freshly built binary and +//! confirms it reports the version pkgwatch believes it just built. Not a +//! security control — see SPEC.md > Verification trust tiers. + use crate::config::SanityCheck; use anyhow::{Context, Result, bail}; use regex::Regex; diff --git a/src/state.rs b/src/state.rs index d10ec2d..cdfe7b3 100644 --- a/src/state.rs +++ b/src/state.rs @@ -1,3 +1,7 @@ +//! Persists two independent per-package facts as plain files: the last +//! published version, and any version currently pending human review. +//! The only module that touches `state/` on disk. + use anyhow::Result; use std::path::Path; diff --git a/src/verifier.rs b/src/verifier.rs index a214f85..913aeeb 100644 --- a/src/verifier.rs +++ b/src/verifier.rs @@ -1,3 +1,8 @@ +//! Runs the trust-tier-specific check declared for a package against a +//! downloaded artifact, and reports a pass/fail plus the tier it implies. +//! The only module that knows what each `Verification::method` actually +//! proves — see SPEC.md > Verification trust tiers. + use crate::checker::version_from_tag; use crate::config::Verification; use crate::fetcher; -- 2.45.2 From f55188e36f816232394ed259976f1f8bdc09059d Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Thu, 17 Sep 2026 12:06:40 +0200 Subject: [PATCH 4/6] Fix issues from code review: shell-escaping gap, exit code, and more MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single-agent code review of this branch's diff (builder/pipeline/ publisher/sanity/hash + main/config/fetcher/state/verifier changes) found six real issues, all fixed here: - builder.rs: validate_shell_safe only rejected a literal single quote and newline, written for the single-quoted PKGBUILD fields. But asset_name (via install_source) and binary_name land in the install() line, which is necessarily double-quoted so ${srcdir}/${pkgdir} can expand — where $, backtick, and backslash are still live. Not currently exploitable (the one variable component, version, is already independently constrained by validate_pkgver's strict charset), but a latent gap relying on that coincidence rather than the validator actually covering its real use context. Widened the reject-list to cover both quoting styles, added regression tests including one at the generate_pkgbuild level. Corrected SPEC.md's "single-quoted" claim to match. - pipeline.rs: a verification failure returned Ok(()) from process_package, so run_check never counted it as a failure and the process exited 0 even on a failed cryptographic/attestation check — exactly the event a monitoring setup (systemd OnFailure=, cron mail-on-error) most needs a non-zero exit to catch. Now bails, which run_check already treats as a package failure. Added an integration test against a mocked GitHub server exercising this exact path. - builder.rs: find_built_package hardcoded the .pkg.tar.zst suffix, so a box with a different PKGEXT in makepkg.conf would report a false "makepkg failed" for a build that actually succeeded. Widened to match any .pkg.tar.* compression. Added direct unit tests (it had none). - pipeline.rs: a newer tier 4-6 version silently overwrote a still- unreviewed older pending version with no indication anything was superseded. Now says so explicitly. - hash.rs: builder/verifier each read a whole downloaded artifact into memory via std::fs::read just to hash it, doubling peak memory for no reason since the file's already on disk. Added sha256_hex_file, streamed in fixed-size chunks; both callers switched to it. - Deduplicated two near-identical test-only "write an executable shell script" helpers (publisher.rs, sanity.rs) into a shared src/test_support.rs. 75 tests (was 63), cargo make ci clean. Re-verified end to end against the real astral-sh/uv release after all six fixes — build, sanity check, and publish into a scratch repo all still succeed. Co-Authored-By: Claude Sonnet 5 --- SPEC.md | 8 ++-- src/builder.rs | 106 +++++++++++++++++++++++++++++++++++++++----- src/hash.rs | 64 +++++++++++++++++++++++--- src/main.rs | 2 + src/pipeline.rs | 103 ++++++++++++++++++++++++++++++++++++++++-- src/publisher.rs | 16 ++----- src/sanity.rs | 9 +--- src/test_support.rs | 21 +++++++++ src/verifier.rs | 7 ++- 9 files changed, 287 insertions(+), 49 deletions(-) create mode 100644 src/test_support.rs diff --git a/SPEC.md b/SPEC.md index 9e341a8..b6cfa09 100644 --- a/SPEC.md +++ b/SPEC.md @@ -353,9 +353,11 @@ Open questions on the schema: attestation verify` rather than reimplementing sigstore verification.)* - **Builder**: for tiers 1–3 on pass, generates a PKGBUILD (strict validation on every upstream-controlled string — version, asset name, - download URL — before it touches generated shell content; every - interpolated value is embedded in a single-quoted bash string and a - literal `'` or newline in the input is rejected outright, never + download URL — before it touches generated shell content; most fields + are single-quoted, but the `install()` line necessarily uses double + quotes so `${srcdir}`/`${pkgdir}` expand, so the validation rejects `'`, + newline, `$`, backtick, *and* backslash — safe for either quoting style + rather than assuming a value only ever lands in one of them — never unescaped interpolation) and runs `makepkg`. *(Implemented — `src/builder.rs`. One fixed "prebuilt binary" PKGBUILD shape covers both tracked packages so far: a bare-binary download (scaleway-cli) and a diff --git a/src/builder.rs b/src/builder.rs index fd5ca55..ff87c26 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -3,7 +3,7 @@ //! PKGBUILD templating and upstream-string validation behind `build()`. use crate::config::Package; -use crate::hash::sha256_hex; +use crate::hash; use anyhow::{Context, Result, bail}; use std::path::{Path, PathBuf}; use std::process::Command; @@ -84,9 +84,7 @@ fn generate_pkgbuild(req: &BuildRequest) -> Result { let binary_name = req.pkg.binary_name(req.pkg_name); validate_pkgname(binary_name)?; - let artifact_data = std::fs::read(req.artifact_path) - .with_context(|| format!("reading {}", req.artifact_path.display()))?; - let sha256 = sha256_hex(&artifact_data); + let sha256 = hash::sha256_hex_file(req.artifact_path)?; let install_source = match archive_stem(req.asset_name) { Some(stem) => format!("{stem}/{binary_name}"), @@ -118,6 +116,11 @@ fn generate_pkgbuild(req: &BuildRequest) -> Result { )) } +/// Matches `.pkg.tar.` — not hardcoded to +/// `.zst` specifically, since `PKGEXT` in makepkg.conf can be set to any +/// of pacman's supported compressions (`.xz`, `.gz`, `.bz2`, ...). This +/// box's default happens to be `.zst`, but guessing wrong would otherwise +/// report a false "makepkg failed" for a build that actually succeeded. fn find_built_package(build_dir: &Path, pkg_name: &str, version: &str) -> Result { let prefix = format!("{pkg_name}-{version}-"); for entry in std::fs::read_dir(build_dir)? { @@ -125,12 +128,12 @@ fn find_built_package(build_dir: &Path, pkg_name: &str, version: &str) -> Result let Some(file_name) = path.file_name().and_then(|n| n.to_str()) else { continue; }; - if file_name.starts_with(&prefix) && file_name.ends_with(".pkg.tar.zst") { + if file_name.starts_with(&prefix) && file_name.contains(".pkg.tar.") { return Ok(path); } } bail!( - "makepkg reported success but no {prefix}*.pkg.tar.zst found in {}", + "makepkg reported success but no {prefix}*.pkg.tar.* found in {}", build_dir.display() ) } @@ -145,12 +148,17 @@ fn archive_stem(asset_name: &str) -> Option<&str> { .find_map(|ext| asset_name.strip_suffix(ext)) } -/// Rejects a single quote or newline: both would let upstream-controlled -/// text (asset names, download URLs) break out of the single-quoted bash -/// strings the PKGBUILD template embeds them in. See SPEC.md > Architecture -/// > Builder ("never unescaped interpolation"). +/// Rejects characters that are dangerous in *either* quoting style the +/// PKGBUILD template uses: a single quote breaks out of the single-quoted +/// fields (`pkgname`, `sha256sums`, ...); `$`, a backtick, or a backslash +/// are still live inside the double-quoted `install()` line, where +/// `asset_name` (via `install_source`) and `binary_name` end up embedded +/// so `${srcdir}`/`${pkgdir}` can expand. A single check covering both +/// contexts is safer than trying to remember which fields land in which +/// quoting style. See SPEC.md > Architecture > Builder ("never unescaped +/// interpolation"). fn validate_shell_safe(field: &str, value: &str) -> Result<()> { - if value.contains('\'') || value.contains('\n') { + if value.contains(['\'', '\n', '$', '`', '\\']) { bail!("{field} '{value}' contains an unsafe character for a generated PKGBUILD"); } Ok(()) @@ -201,6 +209,40 @@ mod tests { assert_eq!(archive_stem("scaleway-cli_2.62.0_linux_amd64"), None); } + #[test] + fn find_built_package_matches_default_zst_extension() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("uv-0.12.15-1-x86_64.pkg.tar.zst"), b"").unwrap(); + let found = find_built_package(dir.path(), "uv", "0.12.15").unwrap(); + assert_eq!(found, dir.path().join("uv-0.12.15-1-x86_64.pkg.tar.zst")); + } + + #[test] + fn find_built_package_matches_non_default_pkgext() { + // A box with PKGEXT='.pkg.tar.xz' in makepkg.conf shouldn't report + // a false failure just because this crate's default assumption + // (.zst) doesn't match. + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("uv-0.12.15-1-x86_64.pkg.tar.xz"), b"").unwrap(); + let found = find_built_package(dir.path(), "uv", "0.12.15").unwrap(); + assert_eq!(found, dir.path().join("uv-0.12.15-1-x86_64.pkg.tar.xz")); + } + + #[test] + fn find_built_package_ignores_non_matching_prefix() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("other-0.12.15-1-x86_64.pkg.tar.zst"), b"").unwrap(); + assert!(find_built_package(dir.path(), "uv", "0.12.15").is_err()); + } + + #[test] + fn find_built_package_errors_with_clear_message_when_nothing_matches() { + let dir = tempfile::tempdir().unwrap(); + let err = find_built_package(dir.path(), "uv", "0.12.15").unwrap_err(); + assert!(err.to_string().contains("uv-0.12.15-")); + assert!(err.to_string().contains(".pkg.tar.*")); + } + #[test] fn validate_pkgver_accepts_dotted_version() { assert!(validate_pkgver("2.62.0").is_ok()); @@ -241,6 +283,24 @@ mod tests { assert!(validate_shell_safe("download url", "https://example.com/a\nb").is_err()); } + #[test] + fn validate_shell_safe_rejects_dollar_sign() { + // asset_name lands inside a double-quoted string via + // install_source — $() command substitution is still live there + // even though single-quote breakout isn't. + assert!(validate_shell_safe("asset name", "thing$(touch pwned).tar.gz").is_err()); + } + + #[test] + fn validate_shell_safe_rejects_backtick() { + assert!(validate_shell_safe("asset name", "thing`touch pwned`.tar.gz").is_err()); + } + + #[test] + fn validate_shell_safe_rejects_backslash() { + assert!(validate_shell_safe("asset name", "thing\\$(touch pwned).tar.gz").is_err()); + } + #[test] fn validate_shell_safe_accepts_normal_url() { assert!(validate_shell_safe("download url", "https://example.com/a/b.tar.gz").is_ok()); @@ -294,7 +354,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let artifact_path = dir.path().join("scaleway-cli_2.62.0_linux_amd64"); std::fs::write(&artifact_path, b"binary-bytes").unwrap(); - let expected_sha = sha256_hex(b"binary-bytes"); + let expected_sha = hash::sha256_hex(b"binary-bytes"); let req = BuildRequest { pkg_name: "scaleway-cli", @@ -360,4 +420,26 @@ mod tests { }; assert!(generate_pkgbuild(&req).is_err()); } + + #[test] + fn generate_pkgbuild_rejects_asset_name_with_command_substitution() { + // Regression test: asset_name feeds install_source, which is + // embedded in the double-quoted install() line, not a + // single-quoted field — a single-quote-only check would miss this. + let pkg = make_package(None); + let dir = tempfile::tempdir().unwrap(); + let artifact_path = dir.path().join("thing.tar.gz"); + std::fs::write(&artifact_path, b"data").unwrap(); + + let req = BuildRequest { + pkg_name: "thing", + pkg: &pkg, + version: "1.0.0", + repo: "o/r", + asset_name: "thing$(touch pwned).tar.gz", + download_url: "https://example.com/thing.tar.gz", + artifact_path: &artifact_path, + }; + assert!(generate_pkgbuild(&req).is_err()); + } } diff --git a/src/hash.rs b/src/hash.rs index 6406360..548a04c 100644 --- a/src/hash.rs +++ b/src/hash.rs @@ -1,19 +1,44 @@ -//! One function, shared by two real callers (`verifier`, `builder`) — +//! Two functions, shared by two real callers (`verifier`, `builder`) — //! not a general-purpose utils dump. See ARCHITECTURE.md > "organize by //! pipeline stage, not by layer" for why that distinction matters. +use anyhow::{Context, Result}; use sha2::{Digest, Sha256}; +use std::io::Read; +use std::path::Path; -/// Shared by `verifier` (same-origin-sha256 checks) and `builder` (every -/// generated PKGBUILD needs a `sha256sums` entry for makepkg's own local -/// integrity check, regardless of pkgwatch's own trust tier for that -/// package). -pub fn sha256_hex(data: &[u8]) -> String { +/// In-memory digest — test-only now that both real callers (`verifier`, +/// `builder`) hash a file already on disk via `sha256_hex_file` instead. +/// Kept for building expected hashes from in-memory test fixtures. +#[cfg(test)] +pub(crate) fn sha256_hex(data: &[u8]) -> String { let mut hasher = Sha256::new(); hasher.update(data); hex::encode(hasher.finalize()) } +/// Same digest as `sha256_hex(&std::fs::read(path)?)`, but streamed in +/// fixed-size chunks instead of reading the whole file into memory first — +/// downloaded release assets are tens of MB, and the file is already on +/// disk, so there's no reason to hold a second full copy in memory just to +/// hash it. +pub fn sha256_hex_file(path: &Path) -> Result { + let mut file = + std::fs::File::open(path).with_context(|| format!("opening {}", path.display()))?; + let mut hasher = Sha256::new(); + let mut buf = [0u8; 64 * 1024]; + loop { + let n = file + .read(&mut buf) + .with_context(|| format!("reading {}", path.display()))?; + if n == 0 { + break; + } + hasher.update(&buf[..n]); + } + Ok(hex::encode(hasher.finalize())) +} + #[cfg(test)] mod tests { use super::*; @@ -26,4 +51,31 @@ mod tests { "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" ); } + + #[test] + fn sha256_hex_file_matches_in_memory_digest() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("data.bin"); + // Bigger than one read chunk, to actually exercise the loop. + let data = vec![0x5au8; 200 * 1024]; + std::fs::write(&path, &data).unwrap(); + + assert_eq!(sha256_hex_file(&path).unwrap(), sha256_hex(&data)); + } + + #[test] + fn sha256_hex_file_matches_for_empty_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("empty.bin"); + std::fs::write(&path, b"").unwrap(); + + assert_eq!(sha256_hex_file(&path).unwrap(), sha256_hex(b"")); + } + + #[test] + fn sha256_hex_file_errors_on_missing_file() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("does-not-exist.bin"); + assert!(sha256_hex_file(&missing).is_err()); + } } diff --git a/src/main.rs b/src/main.rs index 61796cc..6ccf360 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,6 +12,8 @@ mod pipeline; mod publisher; mod sanity; mod state; +#[cfg(test)] +mod test_support; mod verifier; use anyhow::{Result, bail}; diff --git a/src/pipeline.rs b/src/pipeline.rs index cabf31d..0fa0408 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -131,15 +131,19 @@ fn process_package( fetched.verification.justification ); - let already_pending = - state::load_pending_version(state_dir, name).as_deref() == Some(latest.as_str()); + let previously_pending = state::load_pending_version(state_dir, name); + let already_pending = previously_pending.as_deref() == Some(latest.as_str()); match decide_tier_action( fetched.verification.tier, fetched.verification.passed, already_pending, ) { + // Bail rather than just print-and-return: a verification failure + // is exactly the kind of event a monitoring setup (systemd + // OnFailure=, cron mail-on-error) needs a non-zero exit to catch — + // see run_check, which treats an Err here as a failed package. TierAction::VerificationFailed => { - println!(" verification failed — not publishing, not updating state"); + bail!("verification failed — not publishing, not updating state"); } TierAction::Publish => { println!(" tier 1-3 pass: building + publishing"); @@ -153,7 +157,14 @@ fn process_package( } TierAction::NewlyPending => { state::save_pending_version(state_dir, name, &latest)?; - println!(" tier 4-6 pass: flagged for human review (`pkgwatch review` to approve)"); + match previously_pending { + Some(superseded) => println!( + " tier 4-6 pass: flagged for human review, superseding still-unreviewed {superseded} (`pkgwatch review` to approve {latest})" + ), + None => println!( + " tier 4-6 pass: flagged for human review (`pkgwatch review` to approve)" + ), + } } } Ok(()) @@ -338,4 +349,88 @@ mod tests { fn decide_tier_action_tier_4_to_6_still_pending_when_already_flagged() { assert_eq!(decide_tier_action(4, true, true), TierAction::StillPending); } + + /// Regression test for the exit-code gap this PR fixes: a verification + /// failure previously returned `Ok(())` from `process_package`, so + /// `run_check` never counted it as a failure and the process exited 0 + /// even though the single most security-relevant check had failed. + /// Exercises the full check -> fetch -> verify path against a mocked + /// GitHub (no real network), stopping before any build/publish step + /// since verification failure returns before reaching those. + #[test] + fn process_package_returns_err_on_verification_failure() { + let mut server = mockito::Server::new(); + let endpoints = GithubEndpoints { + web: server.url(), + api: server.url(), + }; + + let feed = format!( + r#""#, + server.url() + ); + let _atom = server + .mock("GET", "/o/r/releases.atom") + .with_status(200) + .with_body(feed) + .create(); + + let asset_url = format!("{}/download/thing.tar.gz", server.url()); + let sums_url = format!("{}/download/SHA256SUMS", server.url()); + let release_body = format!( + r#"{{"assets": [ + {{"name": "thing.tar.gz", "browser_download_url": "{asset_url}"}}, + {{"name": "SHA256SUMS", "browser_download_url": "{sums_url}"}} + ]}}"# + ); + let _release = server + .mock("GET", "/repos/o/r/releases/tags/v1.0.0") + .with_status(200) + .with_body(release_body) + .create(); + let _asset = server + .mock("GET", "/download/thing.tar.gz") + .with_status(200) + .with_body(b"artifact-bytes".as_slice()) + .create(); + // Wrong hash for "artifact-bytes" — forces a verification failure. + let _sums = server + .mock("GET", "/download/SHA256SUMS") + .with_status(200) + .with_body( + "0000000000000000000000000000000000000000000000000000000000000000 thing.tar.gz\n", + ) + .create(); + + let pkg: Package = toml::from_str( + r#" + repo = "o/r" + asset_pattern = "thing.tar.gz" + [verification] + method = "same-origin-sha256" + checksum_asset_pattern = "SHA256SUMS" + "#, + ) + .unwrap(); + + let client = reqwest::blocking::Client::new(); + let state_dir = tempfile::tempdir().unwrap(); + let work_dir = tempfile::tempdir().unwrap(); + + let err = process_package( + &client, + &endpoints, + state_dir.path(), + work_dir.path(), + "thing", + &pkg, + ) + .unwrap_err(); + + assert!(err.to_string().contains("verification failed")); + // Neither published nor queued for review — a failed verification + // shouldn't leave any trace in state. + assert_eq!(state::load_last_version(state_dir.path(), "thing"), None); + assert_eq!(state::load_pending_version(state_dir.path(), "thing"), None); + } } diff --git a/src/publisher.rs b/src/publisher.rs index f14c95b..89b3c04 100644 --- a/src/publisher.rs +++ b/src/publisher.rs @@ -96,21 +96,13 @@ fn ensure_registered_at(pacman_conf: &Path, repo_name: &str, repo_dir: &Path) -> #[cfg(test)] mod tests { use super::*; - - fn write_stub(dir: &Path, name: &str, script: &str) -> PathBuf { - let path = dir.join(name); - std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap(); - let mut perms = std::fs::metadata(&path).unwrap().permissions(); - std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o755); - std::fs::set_permissions(&path, perms).unwrap(); - path - } + use crate::test_support::write_executable_script; #[test] fn publish_copies_package_and_invokes_repo_add() { let stub_dir = tempfile::tempdir().unwrap(); let log_path = stub_dir.path().join("invoked_with.txt"); - let repo_add = write_stub( + let repo_add = write_executable_script( stub_dir.path(), "fake-repo-add", &format!("echo \"$@\" > {}", log_path.display()), @@ -137,7 +129,7 @@ mod tests { #[test] fn publish_errors_when_repo_add_fails() { let stub_dir = tempfile::tempdir().unwrap(); - let repo_add = write_stub(stub_dir.path(), "fake-repo-add-fail", "exit 1"); + let repo_add = write_executable_script(stub_dir.path(), "fake-repo-add-fail", "exit 1"); let src_dir = tempfile::tempdir().unwrap(); let package_path = src_dir.path().join("thing-1.0.0-1-x86_64.pkg.tar.zst"); @@ -151,7 +143,7 @@ mod tests { #[test] fn publish_creates_repo_dir_if_missing() { let stub_dir = tempfile::tempdir().unwrap(); - let repo_add = write_stub(stub_dir.path(), "fake-repo-add-ok", "exit 0"); + let repo_add = write_executable_script(stub_dir.path(), "fake-repo-add-ok", "exit 0"); let src_dir = tempfile::tempdir().unwrap(); let package_path = src_dir.path().join("thing-1.0.0-1-x86_64.pkg.tar.zst"); diff --git a/src/sanity.rs b/src/sanity.rs index 4aab514..eab4f7a 100644 --- a/src/sanity.rs +++ b/src/sanity.rs @@ -67,14 +67,7 @@ pub fn run(check: &SanityCheck, pkg_bin_dir: &Path, expected_version: &str) -> R #[cfg(test)] mod tests { use super::*; - - fn write_fake_binary(dir: &Path, name: &str, script: &str) { - let path = dir.join(name); - std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap(); - let mut perms = std::fs::metadata(&path).unwrap().permissions(); - std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o755); - std::fs::set_permissions(&path, perms).unwrap(); - } + use crate::test_support::write_executable_script as write_fake_binary; #[test] fn run_passes_when_reported_version_matches() { diff --git a/src/test_support.rs b/src/test_support.rs new file mode 100644 index 0000000..4ffaa5f --- /dev/null +++ b/src/test_support.rs @@ -0,0 +1,21 @@ +//! Test-only fixture helpers shared across modules' `#[cfg(test)]` code +//! (`publisher`, `sanity`) — not production code, and not built outside +//! `cargo test`. See ARCHITECTURE.md > "organize by pipeline stage, not +//! by layer": this exists to remove one specific piece of duplication +//! (two near-identical copies of "write an executable shell script"), not +//! as a general test-utils dump. + +use std::path::{Path, PathBuf}; + +/// Writes an executable `#!/bin/sh` script named `name` into `dir`, +/// running `body` as its contents. Used to stand in for a real binary +/// (`repo-add`, a package's own `--version` command) in tests, without +/// needing the real tool installed or a mutated global `PATH`. +pub(crate) fn write_executable_script(dir: &Path, name: &str, body: &str) -> PathBuf { + let path = dir.join(name); + std::fs::write(&path, format!("#!/bin/sh\n{body}\n")).unwrap(); + let mut perms = std::fs::metadata(&path).unwrap().permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o755); + std::fs::set_permissions(&path, perms).unwrap(); + path +} diff --git a/src/verifier.rs b/src/verifier.rs index 913aeeb..85f7168 100644 --- a/src/verifier.rs +++ b/src/verifier.rs @@ -7,7 +7,7 @@ use crate::checker::version_from_tag; use crate::config::Verification; use crate::fetcher; use crate::github::GithubEndpoints; -use crate::hash::sha256_hex; +use crate::hash; use anyhow::{Context, Result, bail}; use std::path::Path; use std::process::Command; @@ -51,8 +51,7 @@ pub fn verify( .context("artifact path has no filename")?; let expected = expected_checksum(&checksum_text, artifact_name)?; - let data = std::fs::read(artifact_path)?; - let actual = sha256_hex(&data); + let actual = hash::sha256_hex_file(artifact_path)?; let passed = actual == expected; Ok(VerificationResult { @@ -192,7 +191,7 @@ mod tests { let dest_dir = tempfile::tempdir().unwrap(); let artifact_path = dest_dir.path().join("thing.tar.gz"); std::fs::write(&artifact_path, b"hello world").unwrap(); - let expected_hash = sha256_hex(b"hello world"); + let expected_hash = hash::sha256_hex(b"hello world"); let release_url = format!("{}/download/SHA256SUMS", server.url()); let release_body = format!( -- 2.45.2 From 3e87282b2194cd60dfb514b8c6aae9fe05788f70 Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Thu, 17 Sep 2026 13:22:40 +0200 Subject: [PATCH 5/6] Move high-level docs into docs/, matching doubleo7's convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SPEC.md and ARCHITECTURE.md were sitting at the repo root alongside Cargo.toml/Makefile.toml/packages.d — moved both into docs/ (doubleo7 already does this for its own supplementary docs, so this matches an existing convention in the fleet rather than inventing a new one). Updated every doc-comment cross-reference across src/*.rs and Makefile.toml (23 references) to the new docs/SPEC.md / docs/ ARCHITECTURE.md paths. The two files' own cross-references to each other didn't need changing — they're still same-directory relative references. Also updated the project reference memory pointing at ARCHITECTURE.md's location, so it doesn't go stale pointing at a path that no longer exists. Co-Authored-By: Claude Sonnet 5 --- Makefile.toml | 2 +- ARCHITECTURE.md => docs/ARCHITECTURE.md | 0 SPEC.md => docs/SPEC.md | 0 src/builder.rs | 6 +++--- src/config.rs | 8 ++++---- src/hash.rs | 2 +- src/main.rs | 6 +++--- src/pipeline.rs | 4 ++-- src/publisher.rs | 2 +- src/sanity.rs | 4 ++-- src/state.rs | 4 ++-- src/test_support.rs | 2 +- src/verifier.rs | 6 +++--- 13 files changed, 23 insertions(+), 23 deletions(-) rename ARCHITECTURE.md => docs/ARCHITECTURE.md (100%) rename SPEC.md => docs/SPEC.md (100%) diff --git a/Makefile.toml b/Makefile.toml index bd75029..a5b2556 100644 --- a/Makefile.toml +++ b/Makefile.toml @@ -37,7 +37,7 @@ args = ["test"] # orchestration too (hence its own low number) — its one pure decision # function (decide_tier_action) is unit tested and should stay visible in # this report; excluding the whole file would hide that signal along with -# the untested parts. See ARCHITECTURE.md > "separate pure decision logic +# the untested parts. See docs/ARCHITECTURE.md > "separate pure decision logic # from I/O." [tasks.coverage-report] command = "cargo" diff --git a/ARCHITECTURE.md b/docs/ARCHITECTURE.md similarity index 100% rename from ARCHITECTURE.md rename to docs/ARCHITECTURE.md diff --git a/SPEC.md b/docs/SPEC.md similarity index 100% rename from SPEC.md rename to docs/SPEC.md diff --git a/src/builder.rs b/src/builder.rs index ff87c26..8ce0021 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -36,7 +36,7 @@ pub struct BuildResult { /// artifact, then runs `makepkg` in `build_dir`. /// /// Deliberately one fixed "prebuilt binary" shape, not a templating engine -/// — see SPEC.md > Scaling > Template reuse. Covers the two shapes the two +/// — see docs/SPEC.md > Scaling > Template reuse. Covers the two shapes the two /// currently-tracked packages actually need: a bare-binary download /// (scaleway-cli) and a tarball containing a same-named directory (uv). /// Extend when a third real shape shows up rather than guessing at @@ -71,7 +71,7 @@ pub fn build(req: &BuildRequest, build_dir: &Path) -> Result { } /// Builds the PKGBUILD text for `req`, validating every upstream-controlled -/// string first (see SPEC.md > Architecture > Builder: "strict validation +/// string first (see docs/SPEC.md > Architecture > Builder: "strict validation /// on any upstream-controlled string ... never unescaped interpolation"). /// Pure and side-effect-free so it's testable without invoking `makepkg`. fn generate_pkgbuild(req: &BuildRequest) -> Result { @@ -155,7 +155,7 @@ fn archive_stem(asset_name: &str) -> Option<&str> { /// `asset_name` (via `install_source`) and `binary_name` end up embedded /// so `${srcdir}`/`${pkgdir}` can expand. A single check covering both /// contexts is safer than trying to remember which fields land in which -/// quoting style. See SPEC.md > Architecture > Builder ("never unescaped +/// quoting style. See docs/SPEC.md > Architecture > Builder ("never unescaped /// interpolation"). fn validate_shell_safe(field: &str, value: &str) -> Result<()> { if value.contains(['\'', '\n', '$', '`', '\\']) { diff --git a/src/config.rs b/src/config.rs index e8c5874..9a61e95 100644 --- a/src/config.rs +++ b/src/config.rs @@ -16,7 +16,7 @@ struct PackageFile { pub struct Package { pub repo: String, /// Exact GitHub release asset name (still not a glob — see - /// SPEC.md > Architecture > Fetcher), optionally containing a + /// docs/SPEC.md > Architecture > Fetcher), optionally containing a /// `{version}` placeholder for projects whose asset names embed the /// version (e.g. `scaleway-cli_{version}_linux_amd64`). Substituted via /// `checker::version_from_tag` before matching. @@ -29,7 +29,7 @@ pub struct Package { /// the repo name). Defaults to the package name when omitted. pub binary_name: Option, /// Post-build correctness check (not a security control — see - /// SPEC.md > Verification trust tiers). Runs `command` against the + /// docs/SPEC.md > Verification trust tiers). Runs `command` against the /// freshly built binary and confirms `version_regex`'s capture group /// matches the version pkgwatch believes it just built. pub sanity_check: Option, @@ -53,7 +53,7 @@ pub struct SanityCheck { #[serde(tag = "method", rename_all = "kebab-case")] pub enum Verification { /// Tier 4: proves transport integrity only, not authorship. See - /// SPEC.md > Verification trust tiers. `checksum_asset_pattern` may + /// docs/SPEC.md > Verification trust tiers. `checksum_asset_pattern` may /// also contain a `{version}` placeholder, same as `asset_pattern`. SameOriginSha256 { checksum_asset_pattern: String }, /// Tier 2: GitHub build-provenance attestation, verified via `gh @@ -71,7 +71,7 @@ impl Verification { } /// Loads every `*.toml` file in `dir` (the `packages.d/` layout from -/// SPEC.md > Scaling to many packages), keyed by package name. +/// docs/SPEC.md > Scaling to many packages), keyed by package name. pub fn load_packages_dir(dir: &Path) -> Result> { let mut out = Vec::new(); for entry in std::fs::read_dir(dir).with_context(|| format!("reading {}", dir.display()))? { diff --git a/src/hash.rs b/src/hash.rs index 548a04c..854b99d 100644 --- a/src/hash.rs +++ b/src/hash.rs @@ -1,5 +1,5 @@ //! Two functions, shared by two real callers (`verifier`, `builder`) — -//! not a general-purpose utils dump. See ARCHITECTURE.md > "organize by +//! not a general-purpose utils dump. See docs/ARCHITECTURE.md > "organize by //! pipeline stage, not by layer" for why that distinction matters. use anyhow::{Context, Result}; diff --git a/src/main.rs b/src/main.rs index 6ccf360..300ac6b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,6 @@ //! Entry point: parses `argv` and dispatches to `pipeline`. Nothing here //! makes a network/subprocess call or contains a decision worth a test — -//! see ARCHITECTURE.md > "main is a dispatcher, not the program." +//! see docs/ARCHITECTURE.md > "main is a dispatcher, not the program." mod builder; mod checker; @@ -20,8 +20,8 @@ use anyhow::{Result, bail}; /// check -> fetch -> verify -> build -> sanity-check -> publish, for /// whatever is in packages.d/. Tier 1-3 passes auto-publish; tier 4-6 -/// passes queue for `pkgwatch review`. See SPEC.md > Architecture for what -/// each stage does, and ARCHITECTURE.md for how the code implementing it +/// passes queue for `pkgwatch review`. See docs/SPEC.md > Architecture for what +/// each stage does, and docs/ARCHITECTURE.md for how the code implementing it /// is organized. fn main() -> Result<()> { let args: Vec = std::env::args().skip(1).collect(); diff --git a/src/pipeline.rs b/src/pipeline.rs index 0fa0408..8d0b037 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -1,7 +1,7 @@ //! Orchestrates one run of check -> fetch -> verify -> build -> //! sanity-check -> publish across every configured package, plus the //! `review` subcommand for tier 4-6 approvals. The only module that calls -//! more than one other pipeline-stage module — see ARCHITECTURE.md > "main +//! more than one other pipeline-stage module — see docs/ARCHITECTURE.md > "main //! is a dispatcher, not the program" for why this lives here and not in //! `main.rs`. @@ -73,7 +73,7 @@ pub fn run_check() -> Result<()> { /// What to do about a package after verification, derived purely from the /// verification outcome and whether this exact version is already queued -/// for review — no I/O. See ARCHITECTURE.md > "separate pure decision +/// for review — no I/O. See docs/ARCHITECTURE.md > "separate pure decision /// logic from I/O." #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum TierAction { diff --git a/src/publisher.rs b/src/publisher.rs index 89b3c04..7bb7f79 100644 --- a/src/publisher.rs +++ b/src/publisher.rs @@ -8,7 +8,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; /// Pacman's system-wide config — hardcoded like the rest of this tool's -/// Arch/Manjaro-specific assumptions (see SPEC.md > Scope). +/// Arch/Manjaro-specific assumptions (see docs/SPEC.md > Scope). const PACMAN_CONF: &str = "/etc/pacman.conf"; /// Copies the built package into `repo_dir` and runs `repo-add` against diff --git a/src/sanity.rs b/src/sanity.rs index eab4f7a..b56efda 100644 --- a/src/sanity.rs +++ b/src/sanity.rs @@ -1,6 +1,6 @@ //! Post-build correctness check: runs the freshly built binary and //! confirms it reports the version pkgwatch believes it just built. Not a -//! security control — see SPEC.md > Verification trust tiers. +//! security control — see docs/SPEC.md > Verification trust tiers. use crate::config::SanityCheck; use anyhow::{Context, Result, bail}; @@ -14,7 +14,7 @@ use std::process::Command; /// whatever's already on the system. Confirms `check.version_regex`'s /// capture group matches `expected_version`. /// -/// Correctness check only, not a security control — see SPEC.md > +/// Correctness check only, not a security control — see docs/SPEC.md > /// Verification trust tiers. Catches checker bugs and mangled/wrong-asset /// downloads, not malicious releases. pub fn run(check: &SanityCheck, pkg_bin_dir: &Path, expected_version: &str) -> Result<()> { diff --git a/src/state.rs b/src/state.rs index cdfe7b3..70dbbb2 100644 --- a/src/state.rs +++ b/src/state.rs @@ -8,7 +8,7 @@ use std::path::Path; /// Last-known-published version per package, so re-runs don't re-flag a /// version already handled. Deliberately just one file per package for /// now — this is where a real review-queue persistence layer plugs in -/// later (see SPEC.md > Architecture > Reviewer queue). +/// later (see docs/SPEC.md > Architecture > Reviewer queue). pub fn load_last_version(state_dir: &Path, name: &str) -> Option { std::fs::read_to_string(state_dir.join(format!("{name}.version"))) .ok() @@ -22,7 +22,7 @@ pub fn save_last_version(state_dir: &Path, name: &str, version: &str) -> Result< } /// Tag currently awaiting human review for a tier 4-6 package (see -/// SPEC.md > Architecture > Reviewer queue), if any. Separate from +/// docs/SPEC.md > Architecture > Reviewer queue), if any. Separate from /// `load_last_version`/`save_last_version`: approving a review doesn't /// mean future versions auto-publish, so the two must be tracked /// independently. diff --git a/src/test_support.rs b/src/test_support.rs index 4ffaa5f..49b701a 100644 --- a/src/test_support.rs +++ b/src/test_support.rs @@ -1,6 +1,6 @@ //! Test-only fixture helpers shared across modules' `#[cfg(test)]` code //! (`publisher`, `sanity`) — not production code, and not built outside -//! `cargo test`. See ARCHITECTURE.md > "organize by pipeline stage, not +//! `cargo test`. See docs/ARCHITECTURE.md > "organize by pipeline stage, not //! by layer": this exists to remove one specific piece of duplication //! (two near-identical copies of "write an executable shell script"), not //! as a general test-utils dump. diff --git a/src/verifier.rs b/src/verifier.rs index 85f7168..7ffa4d9 100644 --- a/src/verifier.rs +++ b/src/verifier.rs @@ -1,7 +1,7 @@ //! Runs the trust-tier-specific check declared for a package against a //! downloaded artifact, and reports a pass/fail plus the tier it implies. //! The only module that knows what each `Verification::method` actually -//! proves — see SPEC.md > Verification trust tiers. +//! proves — see docs/SPEC.md > Verification trust tiers. use crate::checker::version_from_tag; use crate::config::Verification; @@ -19,7 +19,7 @@ pub struct VerificationResult { } /// Runs the verification method declared for a package against a -/// downloaded artifact. See SPEC.md > Verification trust tiers for what +/// downloaded artifact. See docs/SPEC.md > Verification trust tiers for what /// each tier does and does not prove. pub fn verify( client: &reqwest::blocking::Client, @@ -59,7 +59,7 @@ pub fn verify( passed, justification: if passed { "same-origin sha256 matched — proves transport integrity only, \ - not authorship (see tier 4 in SPEC.md)" + not authorship (see tier 4 in docs/SPEC.md)" .into() } else { format!("sha256 mismatch: expected {expected}, got {actual}") -- 2.45.2 From 0044eda532a4376e42b948f9de37c3a47718ae58 Mon Sep 17 00:00:00 2001 From: Austin Schaefer Date: Fri, 18 Sep 2026 12:46:28 +0200 Subject: [PATCH 6/6] Add a third builder shape for archives with no wrapping directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging master's claude-code.toml onto this branch surfaced a real gap: builder.rs only knew "bare binary download" and "tarball extracting into a same-named directory" (uv). claude-code's tarball extracts a bare `claude` file with no wrapping directory, and that inner filename doesn't match the package name either — makepkg's package() failed with "cannot stat .../claude-linux-x64/claude-code" (confirmed by actually running the build). Add Package::archive_binary_path, an explicit override for the in-archive path builder.rs installs from, used verbatim when present instead of the stem/binary_name convention. Set binary_name = "claude" too, matching the box's actual command name (/opt/claude-code/bin/claude) rather than the claude-code package name. Also added a sanity_check block (claude --version), matching uv's pattern, confirmed against the real built binary's output ("2.1.276 (Claude Code)"). Verified end to end against the real repo with PKGWATCH_REPO_DIR pointed at a scratch dir: check -> fetch -> verify -> review --approve -> build -> sanity-check -> publish all pass for claude-code v2.1.276. Co-Authored-By: Claude Sonnet 5 --- packages.d/claude-code.toml | 14 +++++++ src/builder.rs | 83 ++++++++++++++++++++++++++++++++++--- src/config.rs | 10 +++++ 3 files changed, 101 insertions(+), 6 deletions(-) diff --git a/packages.d/claude-code.toml b/packages.d/claude-code.toml index 0409e2d..18abf8b 100644 --- a/packages.d/claude-code.toml +++ b/packages.d/claude-code.toml @@ -16,11 +16,25 @@ # no `{version}` placeholder is needed, same as uv's config. Tracking the # glibc x86_64 Linux build (`claude-linux-x64.tar.gz`), not the musl # variant, to match this machine. +# +# Archive shape doesn't match uv's or scaleway-cli's: the tarball extracts a +# bare `claude` file with no wrapping directory (confirmed via `tar tzvf` +# against the real v2.1.276 asset) — hence archive_binary_path below (see +# builder.rs's third shape). binary_name is also set explicitly to `claude` +# (the real upstream command name, not the `claude-code` package name) so +# the installed binary matches what this box already invokes as `claude` +# (see /opt/claude-code/bin/claude). [package.claude-code] repo = "anthropics/claude-code" asset_pattern = "claude-linux-x64.tar.gz" +binary_name = "claude" +archive_binary_path = "claude" [package.claude-code.verification] method = "same-origin-sha256" checksum_asset_pattern = "SHASUMS256.txt" + +[package.claude-code.sanity_check] +command = "claude --version" +version_regex = '(\d+\.\d+\.\d+) \(Claude Code\)' diff --git a/src/builder.rs b/src/builder.rs index 8ce0021..d91bcb7 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -36,10 +36,12 @@ pub struct BuildResult { /// artifact, then runs `makepkg` in `build_dir`. /// /// Deliberately one fixed "prebuilt binary" shape, not a templating engine -/// — see docs/SPEC.md > Scaling > Template reuse. Covers the two shapes the two +/// — see docs/SPEC.md > Scaling > Template reuse. Covers the shapes /// currently-tracked packages actually need: a bare-binary download -/// (scaleway-cli) and a tarball containing a same-named directory (uv). -/// Extend when a third real shape shows up rather than guessing at +/// (scaleway-cli), a tarball containing a same-named directory (uv), and a +/// tarball with no wrapping directory at all whose inner filename doesn't +/// match the package name (claude-code — see `Package::archive_binary_path`). +/// Extend when a fourth real shape shows up rather than guessing at /// generality now. pub fn build(req: &BuildRequest, build_dir: &Path) -> Result { let pkgbuild = generate_pkgbuild(req)?; @@ -86,9 +88,14 @@ fn generate_pkgbuild(req: &BuildRequest) -> Result { let sha256 = hash::sha256_hex_file(req.artifact_path)?; - let install_source = match archive_stem(req.asset_name) { - Some(stem) => format!("{stem}/{binary_name}"), - None => req.asset_name.to_string(), + let install_source = if let Some(path) = &req.pkg.archive_binary_path { + validate_shell_safe("archive binary path", path)?; + path.clone() + } else { + match archive_stem(req.asset_name) { + Some(stem) => format!("{stem}/{binary_name}"), + None => req.asset_name.to_string(), + } }; Ok(format!( @@ -328,6 +335,23 @@ mod tests { toml::from_str(&toml_text).unwrap() } + fn make_package_with_archive_binary_path( + binary_name: &str, + archive_binary_path: &str, + ) -> Package { + let toml_text = format!( + r#" + repo = "o/r" + asset_pattern = "x" + binary_name = "{binary_name}" + archive_binary_path = "{archive_binary_path}" + [verification] + method = "github-attestation" + "# + ); + toml::from_str(&toml_text).unwrap() + } + #[test] fn build_rejects_unsafe_version() { let pkg = make_package(None); @@ -402,6 +426,53 @@ mod tests { )); } + #[test] + fn generate_pkgbuild_flat_archive_installs_from_archive_binary_path_override() { + // claude-code's shape: a tarball with no wrapping directory, whose + // inner filename ("claude") doesn't match the package name + // ("claude-code") — neither existing shape (stem/binary_name, or + // bare-binary-no-archive) fits, hence the explicit override. + let pkg = make_package_with_archive_binary_path("claude-code", "claude"); + let dir = tempfile::tempdir().unwrap(); + let artifact_path = dir.path().join("claude-linux-x64.tar.gz"); + std::fs::write(&artifact_path, b"tarball-bytes").unwrap(); + + let req = BuildRequest { + pkg_name: "claude-code", + pkg: &pkg, + version: "2.1.276", + repo: "anthropics/claude-code", + asset_name: "claude-linux-x64.tar.gz", + download_url: "https://github.com/anthropics/claude-code/releases/download/v2.1.276/claude-linux-x64.tar.gz", + artifact_path: &artifact_path, + }; + let pkgbuild = generate_pkgbuild(&req).unwrap(); + + assert!( + pkgbuild + .contains("install -Dm755 \"${srcdir}/claude\" \"${pkgdir}/usr/bin/claude-code\"") + ); + } + + #[test] + fn generate_pkgbuild_rejects_archive_binary_path_with_command_substitution() { + let pkg = make_package_with_archive_binary_path("claude-code", "claude$(touch pwned)"); + let dir = tempfile::tempdir().unwrap(); + let artifact_path = dir.path().join("claude-linux-x64.tar.gz"); + std::fs::write(&artifact_path, b"data").unwrap(); + + let req = BuildRequest { + pkg_name: "claude-code", + pkg: &pkg, + version: "2.1.276", + repo: "anthropics/claude-code", + asset_name: "claude-linux-x64.tar.gz", + download_url: "https://github.com/anthropics/claude-code/releases/download/v2.1.276/claude-linux-x64.tar.gz", + artifact_path: &artifact_path, + }; + assert!(generate_pkgbuild(&req).is_err()); + } + #[test] fn generate_pkgbuild_rejects_download_url_with_single_quote() { let pkg = make_package(None); diff --git a/src/config.rs b/src/config.rs index 9a61e95..9d0cff2 100644 --- a/src/config.rs +++ b/src/config.rs @@ -28,6 +28,16 @@ pub struct Package { /// checking the currently-installed extra package, not guessable from /// the repo name). Defaults to the package name when omitted. pub binary_name: Option, + /// Explicit path to the binary inside the extracted archive, relative + /// to `srcdir`, for archive layouts that don't match the "extracts into + /// a directory named after the archive stem" convention `builder.rs` + /// otherwise assumes (e.g. claude-code's tarball extracts a bare + /// `claude` file with no wrapping directory, and that inner filename + /// doesn't match the package name either). Only meaningful when + /// `asset_pattern` names an archive; ignored for bare-binary downloads, + /// where the downloaded file *is* the source path already. Defaults to + /// the stem/`binary_name` convention when omitted. + pub archive_binary_path: Option, /// Post-build correctness check (not a security control — see /// docs/SPEC.md > Verification trust tiers). Runs `command` against the /// freshly built binary and confirms `version_regex`'s capture group -- 2.45.2