pkgwatch/src/pipeline.rs

506 lines
18 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
//! 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 docs/ARCHITECTURE.md > "main
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
//! 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::notifier::{self, Event};
use crate::paths::Paths;
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
use crate::publisher;
use crate::sanity;
use crate::source::Endpoints;
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
use crate::state;
use crate::verifier::{self, VerificationResult};
use anyhow::{Context, Result, bail};
use std::path::{Path, PathBuf};
/// 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<reqwest::blocking::Client> {
Ok(reqwest::blocking::Client::builder()
.user_agent("pkgwatch/0.1 (PoC; https://code.austinschaefer.com)")
.build()?)
}
fn custom_repo_dir() -> Result<PathBuf> {
// 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))
}
/// Adds a hint to the bare "No such file" a missing config dir would give:
/// the dir is no longer relative to the cwd, so a checkout's `packages.d/`
/// isn't picked up on its own (see docs/SPEC.md > Paths).
fn load_packages(packages_dir: &Path) -> Result<Vec<(String, Package)>> {
config::load_packages_dir(packages_dir).with_context(|| {
format!(
"no package config at {} (set PKGWATCH_CONFIG_DIR to the directory containing \
packages.d, or see docs/SPEC.md > Paths for moving a checkout's packages.d/ there)",
packages_dir.display()
)
})
}
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
pub fn run_check() -> Result<()> {
let client = build_client()?;
let Paths {
packages_dir,
state_dir,
work_dir,
} = Paths::from_env()?;
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
let packages = load_packages(&packages_dir)?;
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
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);
let result = Endpoints::for_package(pkg).and_then(|endpoints| {
process_package(&client, &endpoints, &state_dir, &work_dir, name, pkg)
});
if let Err(err) = result {
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
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 docs/ARCHITECTURE.md > "separate pure decision
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
/// 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: &Endpoints,
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
state_dir: &Path,
work_dir: &Path,
name: &str,
pkg: &Package,
) -> Result<()> {
let latest = checker::latest_release(client, endpoints, &pkg.repo)?;
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
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
);
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
let previously_pending = state::load_pending_version(state_dir, name);
let already_pending = previously_pending.as_deref() == Some(latest.as_str());
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
match decide_tier_action(
fetched.verification.tier,
fetched.verification.passed,
already_pending,
) {
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
// 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.
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
TierAction::VerificationFailed => {
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
bail!("verification failed — not publishing, not updating state");
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
}
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}");
notifier::notify(Event::Published {
name,
version: &latest,
});
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
}
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)?;
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
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)"
),
}
notifier::notify(Event::NeedsReview {
name,
version: &latest,
});
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
}
}
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: &Endpoints,
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
work_dir: &Path,
name: &str,
pkg: &Package,
tag: &str,
) -> Result<FetchVerifyResult> {
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 api = endpoints.api();
let asset = fetcher::download_asset(client, api, &pkg.repo, tag, &asset_name, &dest_dir)?;
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
let verification = verifier::verify(
client,
api,
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
&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 Paths {
packages_dir,
state_dir,
work_dir,
} = Paths::from_env()?;
let packages = load_packages(&packages_dir)?;
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
match args {
[] => {
let mut any = false;
for (name, _) in &packages {
if let Some(pending) = state::load_pending_version(&state_dir, name) {
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
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)
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
.with_context(|| format!("'{name}' has no pending review"))?;
approve(&state_dir, &work_dir, name, pkg, &tag)
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
}
_ => bail!("usage: pkgwatch review [<name> --approve]"),
}
}
fn approve(state_dir: &Path, work_dir: &Path, name: &str, pkg: &Package, tag: &str) -> Result<()> {
let client = build_client()?;
let endpoints = Endpoints::for_package(pkg)?;
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
// 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::*;
use crate::github::GithubEndpoints;
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
#[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);
}
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
/// Mocks the release-tag, asset, and checksum endpoints for `o/r@v1.0.0`
/// with a checksum that doesn't match the asset, so verification fails.
/// These are identical on GitHub and Forgejo (see `Endpoints::api`); only
/// how the latest tag is found differs per test. Returned mocks must
/// stay alive for the test's duration.
fn mock_release_with_bad_checksum(server: &mut mockito::ServerGuard) -> Vec<mockito::Mock> {
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
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}"}}
]}}"#
);
vec![
server
.mock("GET", "/repos/o/r/releases/tags/v1.0.0")
.with_status(200)
.with_body(release_body)
.create(),
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.
server
.mock("GET", "/download/SHA256SUMS")
.with_status(200)
.with_body(
"0000000000000000000000000000000000000000000000000000000000000000 thing.tar.gz\n",
)
.create(),
]
}
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
/// `pkg_toml_extra` is spliced in before the `[verification]` table, so
/// it can carry top-level keys like `source`.
fn same_origin_package(pkg_toml_extra: &str) -> Package {
toml::from_str(&format!(
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
r#"
repo = "o/r"
asset_pattern = "thing.tar.gz"
{pkg_toml_extra}
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
[verification]
method = "same-origin-sha256"
checksum_asset_pattern = "SHA256SUMS"
"#
))
.unwrap()
}
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
/// Runs `process_package` for a package whose checksum is wrong and
/// asserts it errors with "verification failed" while leaving no trace
/// in state.
fn assert_verification_failure_is_an_error(endpoints: &Endpoints, pkg: &Package) {
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
let client = reqwest::blocking::Client::new();
let state_dir = tempfile::tempdir().unwrap();
let work_dir = tempfile::tempdir().unwrap();
let err = process_package(
&client,
endpoints,
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
state_dir.path(),
work_dir.path(),
"thing",
pkg,
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
)
.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);
}
/// 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 = Endpoints::Github(GithubEndpoints {
web: server.url(),
api: server.url(),
});
let feed = format!(
r#"<feed><link rel="alternate" href="{}/o/r/releases/tag/v1.0.0"/></feed>"#,
server.url()
);
let _atom = server
.mock("GET", "/o/r/releases.atom")
.with_status(200)
.with_body(feed)
.create();
let _release_mocks = mock_release_with_bad_checksum(&mut server);
assert_verification_failure_is_an_error(&endpoints, &same_origin_package(""));
}
/// Same as above but through a Forgejo source, proving the whole
/// check -> fetch -> verify path works there too: the error is the
/// verification failure, not a fetch or check failure on the way to it.
#[test]
fn process_package_returns_err_on_verification_failure_via_forgejo() {
let mut server = mockito::Server::new();
let endpoints = Endpoints::Forgejo { api: server.url() };
let _latest = server
.mock("GET", "/repos/o/r/releases/latest")
.with_status(200)
.with_body(r#"{"tag_name": "v1.0.0"}"#)
.create();
let _release_mocks = mock_release_with_bad_checksum(&mut server);
let pkg = same_origin_package(
"source = \"forgejo-release\"\nbase_url = \"https://forge.example.com\"",
);
assert_verification_failure_is_an_error(&endpoints, &pkg);
}
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
}