pkgwatch/src/sanity.rs

123 lines
4.2 KiB
Rust
Raw Normal View History

Add ARCHITECTURE.md and apply it to this PR's code 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 <noreply@anthropic.com>
2026-09-17 09:42:18 +00:00
//! 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 docs/SPEC.md > Verification trust tiers.
Add ARCHITECTURE.md and apply it to this PR's code 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 <noreply@anthropic.com>
2026-09-17 09:42:18 +00:00
Close the loop: build, sanity-check, and publish for the first time 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 <name> --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 <noreply@anthropic.com>
2026-09-17 09:13:13 +00:00
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 docs/SPEC.md >
Close the loop: build, sanity-check, and publish for the first time 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 <name> --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 <noreply@anthropic.com>
2026-09-17 09:13:13 +00:00
/// 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::*;
Fix issues from code review: shell-escaping gap, exit code, and more 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 <noreply@anthropic.com>
2026-09-17 10:06:40 +00:00
use crate::test_support::write_executable_script as write_fake_binary;
Close the loop: build, sanity-check, and publish for the first time 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 <name> --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 <noreply@anthropic.com>
2026-09-17 09:13:13 +00:00
#[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"));
}
}