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;