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
|
|
|
//! Two functions, shared by two real callers (`verifier`, `builder`) —
|
2026-09-17 11:22:40 +00:00
|
|
|
//! not a general-purpose utils dump. See docs/ARCHITECTURE.md > "organize by
|
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
|
|
|
//! pipeline stage, not by layer" for why that distinction matters.
|
|
|
|
|
|
Fix issues from code review: shell-escaping gap, exit code, and more
A single-agent code review of this branch's diff (builder/pipeline/
publisher/sanity/hash + main/config/fetcher/state/verifier changes)
found six real issues, all fixed here:
- builder.rs: validate_shell_safe only rejected a literal single quote
and newline, written for the single-quoted PKGBUILD fields. But
asset_name (via install_source) and binary_name land in the install()
line, which is necessarily double-quoted so ${srcdir}/${pkgdir} can
expand — where $, backtick, and backslash are still live. Not
currently exploitable (the one variable component, version, is already
independently constrained by validate_pkgver's strict charset), but a
latent gap relying on that coincidence rather than the validator
actually covering its real use context. Widened the reject-list to
cover both quoting styles, added regression tests including one at the
generate_pkgbuild level. Corrected SPEC.md's "single-quoted" claim to
match.
- pipeline.rs: a verification failure returned Ok(()) from
process_package, so run_check never counted it as a failure and the
process exited 0 even on a failed cryptographic/attestation check —
exactly the event a monitoring setup (systemd OnFailure=, cron
mail-on-error) most needs a non-zero exit to catch. Now bails, which
run_check already treats as a package failure. Added an integration
test against a mocked GitHub server exercising this exact path.
- builder.rs: find_built_package hardcoded the .pkg.tar.zst suffix, so a
box with a different PKGEXT in makepkg.conf would report a false
"makepkg failed" for a build that actually succeeded. Widened to match
any .pkg.tar.* compression. Added direct unit tests (it had none).
- pipeline.rs: a newer tier 4-6 version silently overwrote a still-
unreviewed older pending version with no indication anything was
superseded. Now says so explicitly.
- hash.rs: builder/verifier each read a whole downloaded artifact into
memory via std::fs::read just to hash it, doubling peak memory for no
reason since the file's already on disk. Added sha256_hex_file,
streamed in fixed-size chunks; both callers switched to it.
- Deduplicated two near-identical test-only "write an executable shell
script" helpers (publisher.rs, sanity.rs) into a shared
src/test_support.rs.
75 tests (was 63), cargo make ci clean. Re-verified end to end against
the real astral-sh/uv release after all six fixes — build, sanity check,
and publish into a scratch repo all still succeed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 10:06:40 +00:00
|
|
|
use anyhow::{Context, Result};
|
Close the loop: build, sanity-check, and publish for the first time
Implements the last unimplemented pipeline stage from SPEC.md: PKGBUILD
generation + makepkg (builder.rs), a post-build version sanity check
(sanity.rs), and repo-add publishing (publisher.rs), wired into main.rs
for both the tier 1-3 auto-publish path and a new tier 4-6 review queue
(`pkgwatch review` / `pkgwatch review <name> --approve`, persisted via
state::{load,save,clear}_pending_version, tracked separately from
last-published-version since approving one release isn't a standing
auto-publish grant for future ones).
Publishing targets an existing, already-registered local pacman repo
(~/.local/share/pacman/custom, `[custom]` in /etc/pacman.conf) rather
than one pkgwatch invents — found already in real use for a hand-packaged
AppImage, which resolves SPEC's open question on where the repo lives
without pkgwatch ever touching pacman.conf. Publishing stops at
`repo-add`; actually installing/upgrading (`pacman -Syu`/`pacman -S`) is
left to the operator, not run automatically.
Getting a real second package (scaleway-cli, tier 4) through the new
pipeline immediately surfaced a real gap: its pacman package is named
`scaleway-cli` but the actual binary is `scw` (confirmed via `pacman -Ql`
against the currently-installed extra package) — without a way to
declare that, the build would install alongside extra's package under
the wrong name instead of shadowing it. Added `Package::binary_name`
(config.rs) to cover it.
Every upstream-controlled string (version, asset name, download URL)
is validated before it touches generated shell content in the PKGBUILD
template — rejects anything containing a single quote or newline, since
values are embedded in single-quoted bash strings.
Verified for real, end to end: uv (tier 2) auto-built and published
against the real astral-sh/uv release with no human step; scaleway-cli
(tier 4) queued for review, then approved via `pkgwatch review
scaleway-cli --approve`, which re-verified, built, and published it —
confirmed the built package contains exactly usr/bin/scw. Both landed in
the real custom repo's database. Left scaleway-cli's real-repo review
pending rather than approving it myself: the tier 4-6 gate exists for a
human judgment call, not the agent's.
69 tests, cargo make ci clean (fmt, clippy, complexity, coverage, audit).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 09:13:13 +00:00
|
|
|
use sha2::{Digest, Sha256};
|
Fix issues from code review: shell-escaping gap, exit code, and more
A single-agent code review of this branch's diff (builder/pipeline/
publisher/sanity/hash + main/config/fetcher/state/verifier changes)
found six real issues, all fixed here:
- builder.rs: validate_shell_safe only rejected a literal single quote
and newline, written for the single-quoted PKGBUILD fields. But
asset_name (via install_source) and binary_name land in the install()
line, which is necessarily double-quoted so ${srcdir}/${pkgdir} can
expand — where $, backtick, and backslash are still live. Not
currently exploitable (the one variable component, version, is already
independently constrained by validate_pkgver's strict charset), but a
latent gap relying on that coincidence rather than the validator
actually covering its real use context. Widened the reject-list to
cover both quoting styles, added regression tests including one at the
generate_pkgbuild level. Corrected SPEC.md's "single-quoted" claim to
match.
- pipeline.rs: a verification failure returned Ok(()) from
process_package, so run_check never counted it as a failure and the
process exited 0 even on a failed cryptographic/attestation check —
exactly the event a monitoring setup (systemd OnFailure=, cron
mail-on-error) most needs a non-zero exit to catch. Now bails, which
run_check already treats as a package failure. Added an integration
test against a mocked GitHub server exercising this exact path.
- builder.rs: find_built_package hardcoded the .pkg.tar.zst suffix, so a
box with a different PKGEXT in makepkg.conf would report a false
"makepkg failed" for a build that actually succeeded. Widened to match
any .pkg.tar.* compression. Added direct unit tests (it had none).
- pipeline.rs: a newer tier 4-6 version silently overwrote a still-
unreviewed older pending version with no indication anything was
superseded. Now says so explicitly.
- hash.rs: builder/verifier each read a whole downloaded artifact into
memory via std::fs::read just to hash it, doubling peak memory for no
reason since the file's already on disk. Added sha256_hex_file,
streamed in fixed-size chunks; both callers switched to it.
- Deduplicated two near-identical test-only "write an executable shell
script" helpers (publisher.rs, sanity.rs) into a shared
src/test_support.rs.
75 tests (was 63), cargo make ci clean. Re-verified end to end against
the real astral-sh/uv release after all six fixes — build, sanity check,
and publish into a scratch repo all still succeed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 10:06:40 +00:00
|
|
|
use std::io::Read;
|
|
|
|
|
use std::path::Path;
|
Close the loop: build, sanity-check, and publish for the first time
Implements the last unimplemented pipeline stage from SPEC.md: PKGBUILD
generation + makepkg (builder.rs), a post-build version sanity check
(sanity.rs), and repo-add publishing (publisher.rs), wired into main.rs
for both the tier 1-3 auto-publish path and a new tier 4-6 review queue
(`pkgwatch review` / `pkgwatch review <name> --approve`, persisted via
state::{load,save,clear}_pending_version, tracked separately from
last-published-version since approving one release isn't a standing
auto-publish grant for future ones).
Publishing targets an existing, already-registered local pacman repo
(~/.local/share/pacman/custom, `[custom]` in /etc/pacman.conf) rather
than one pkgwatch invents — found already in real use for a hand-packaged
AppImage, which resolves SPEC's open question on where the repo lives
without pkgwatch ever touching pacman.conf. Publishing stops at
`repo-add`; actually installing/upgrading (`pacman -Syu`/`pacman -S`) is
left to the operator, not run automatically.
Getting a real second package (scaleway-cli, tier 4) through the new
pipeline immediately surfaced a real gap: its pacman package is named
`scaleway-cli` but the actual binary is `scw` (confirmed via `pacman -Ql`
against the currently-installed extra package) — without a way to
declare that, the build would install alongside extra's package under
the wrong name instead of shadowing it. Added `Package::binary_name`
(config.rs) to cover it.
Every upstream-controlled string (version, asset name, download URL)
is validated before it touches generated shell content in the PKGBUILD
template — rejects anything containing a single quote or newline, since
values are embedded in single-quoted bash strings.
Verified for real, end to end: uv (tier 2) auto-built and published
against the real astral-sh/uv release with no human step; scaleway-cli
(tier 4) queued for review, then approved via `pkgwatch review
scaleway-cli --approve`, which re-verified, built, and published it —
confirmed the built package contains exactly usr/bin/scw. Both landed in
the real custom repo's database. Left scaleway-cli's real-repo review
pending rather than approving it myself: the tier 4-6 gate exists for a
human judgment call, not the agent's.
69 tests, cargo make ci clean (fmt, clippy, complexity, coverage, audit).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 09:13:13 +00:00
|
|
|
|
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
|
|
|
/// In-memory digest — test-only now that both real callers (`verifier`,
|
|
|
|
|
/// `builder`) hash a file already on disk via `sha256_hex_file` instead.
|
|
|
|
|
/// Kept for building expected hashes from in-memory test fixtures.
|
|
|
|
|
#[cfg(test)]
|
|
|
|
|
pub(crate) fn sha256_hex(data: &[u8]) -> String {
|
Close the loop: build, sanity-check, and publish for the first time
Implements the last unimplemented pipeline stage from SPEC.md: PKGBUILD
generation + makepkg (builder.rs), a post-build version sanity check
(sanity.rs), and repo-add publishing (publisher.rs), wired into main.rs
for both the tier 1-3 auto-publish path and a new tier 4-6 review queue
(`pkgwatch review` / `pkgwatch review <name> --approve`, persisted via
state::{load,save,clear}_pending_version, tracked separately from
last-published-version since approving one release isn't a standing
auto-publish grant for future ones).
Publishing targets an existing, already-registered local pacman repo
(~/.local/share/pacman/custom, `[custom]` in /etc/pacman.conf) rather
than one pkgwatch invents — found already in real use for a hand-packaged
AppImage, which resolves SPEC's open question on where the repo lives
without pkgwatch ever touching pacman.conf. Publishing stops at
`repo-add`; actually installing/upgrading (`pacman -Syu`/`pacman -S`) is
left to the operator, not run automatically.
Getting a real second package (scaleway-cli, tier 4) through the new
pipeline immediately surfaced a real gap: its pacman package is named
`scaleway-cli` but the actual binary is `scw` (confirmed via `pacman -Ql`
against the currently-installed extra package) — without a way to
declare that, the build would install alongside extra's package under
the wrong name instead of shadowing it. Added `Package::binary_name`
(config.rs) to cover it.
Every upstream-controlled string (version, asset name, download URL)
is validated before it touches generated shell content in the PKGBUILD
template — rejects anything containing a single quote or newline, since
values are embedded in single-quoted bash strings.
Verified for real, end to end: uv (tier 2) auto-built and published
against the real astral-sh/uv release with no human step; scaleway-cli
(tier 4) queued for review, then approved via `pkgwatch review
scaleway-cli --approve`, which re-verified, built, and published it —
confirmed the built package contains exactly usr/bin/scw. Both landed in
the real custom repo's database. Left scaleway-cli's real-repo review
pending rather than approving it myself: the tier 4-6 gate exists for a
human judgment call, not the agent's.
69 tests, cargo make ci clean (fmt, clippy, complexity, coverage, audit).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 09:13:13 +00:00
|
|
|
let mut hasher = Sha256::new();
|
|
|
|
|
hasher.update(data);
|
|
|
|
|
hex::encode(hasher.finalize())
|
|
|
|
|
}
|
|
|
|
|
|
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
|
|
|
/// Same digest as `sha256_hex(&std::fs::read(path)?)`, but streamed in
|
|
|
|
|
/// fixed-size chunks instead of reading the whole file into memory first —
|
|
|
|
|
/// downloaded release assets are tens of MB, and the file is already on
|
|
|
|
|
/// disk, so there's no reason to hold a second full copy in memory just to
|
|
|
|
|
/// hash it.
|
|
|
|
|
pub fn sha256_hex_file(path: &Path) -> Result<String> {
|
|
|
|
|
let mut file =
|
|
|
|
|
std::fs::File::open(path).with_context(|| format!("opening {}", path.display()))?;
|
|
|
|
|
let mut hasher = Sha256::new();
|
|
|
|
|
let mut buf = [0u8; 64 * 1024];
|
|
|
|
|
loop {
|
|
|
|
|
let n = file
|
|
|
|
|
.read(&mut buf)
|
|
|
|
|
.with_context(|| format!("reading {}", path.display()))?;
|
|
|
|
|
if n == 0 {
|
|
|
|
|
break;
|
|
|
|
|
}
|
|
|
|
|
hasher.update(&buf[..n]);
|
|
|
|
|
}
|
|
|
|
|
Ok(hex::encode(hasher.finalize()))
|
|
|
|
|
}
|
|
|
|
|
|
Close the loop: build, sanity-check, and publish for the first time
Implements the last unimplemented pipeline stage from SPEC.md: PKGBUILD
generation + makepkg (builder.rs), a post-build version sanity check
(sanity.rs), and repo-add publishing (publisher.rs), wired into main.rs
for both the tier 1-3 auto-publish path and a new tier 4-6 review queue
(`pkgwatch review` / `pkgwatch review <name> --approve`, persisted via
state::{load,save,clear}_pending_version, tracked separately from
last-published-version since approving one release isn't a standing
auto-publish grant for future ones).
Publishing targets an existing, already-registered local pacman repo
(~/.local/share/pacman/custom, `[custom]` in /etc/pacman.conf) rather
than one pkgwatch invents — found already in real use for a hand-packaged
AppImage, which resolves SPEC's open question on where the repo lives
without pkgwatch ever touching pacman.conf. Publishing stops at
`repo-add`; actually installing/upgrading (`pacman -Syu`/`pacman -S`) is
left to the operator, not run automatically.
Getting a real second package (scaleway-cli, tier 4) through the new
pipeline immediately surfaced a real gap: its pacman package is named
`scaleway-cli` but the actual binary is `scw` (confirmed via `pacman -Ql`
against the currently-installed extra package) — without a way to
declare that, the build would install alongside extra's package under
the wrong name instead of shadowing it. Added `Package::binary_name`
(config.rs) to cover it.
Every upstream-controlled string (version, asset name, download URL)
is validated before it touches generated shell content in the PKGBUILD
template — rejects anything containing a single quote or newline, since
values are embedded in single-quoted bash strings.
Verified for real, end to end: uv (tier 2) auto-built and published
against the real astral-sh/uv release with no human step; scaleway-cli
(tier 4) queued for review, then approved via `pkgwatch review
scaleway-cli --approve`, which re-verified, built, and published it —
confirmed the built package contains exactly usr/bin/scw. Both landed in
the real custom repo's database. Left scaleway-cli's real-repo review
pending rather than approving it myself: the tier 4-6 gate exists for a
human judgment call, not the agent's.
69 tests, cargo make ci clean (fmt, clippy, complexity, coverage, audit).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 09:13:13 +00:00
|
|
|
#[cfg(test)]
|
|
|
|
|
mod tests {
|
|
|
|
|
use super::*;
|
|
|
|
|
|
|
|
|
|
#[test]
|
|
|
|
|
fn matches_known_sha256() {
|
|
|
|
|
// printf 'hello world' | sha256sum
|
|
|
|
|
assert_eq!(
|
|
|
|
|
sha256_hex(b"hello world"),
|
|
|
|
|
"b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9"
|
|
|
|
|
);
|
|
|
|
|
}
|
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
|
|
|
|
|
|
|
|
#[test]
|
|
|
|
|
fn sha256_hex_file_matches_in_memory_digest() {
|
|
|
|
|
let dir = tempfile::tempdir().unwrap();
|
|
|
|
|
let path = dir.path().join("data.bin");
|
|
|
|
|
// Bigger than one read chunk, to actually exercise the loop.
|
|
|
|
|
let data = vec![0x5au8; 200 * 1024];
|
|
|
|
|
std::fs::write(&path, &data).unwrap();
|
|
|
|
|
|
|
|
|
|
assert_eq!(sha256_hex_file(&path).unwrap(), sha256_hex(&data));
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
#[test]
|
|
|
|
|
fn sha256_hex_file_matches_for_empty_file() {
|
|
|
|
|
let dir = tempfile::tempdir().unwrap();
|
|
|
|
|
let path = dir.path().join("empty.bin");
|
|
|
|
|
std::fs::write(&path, b"").unwrap();
|
|
|
|
|
|
|
|
|
|
assert_eq!(sha256_hex_file(&path).unwrap(), sha256_hex(b""));
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
#[test]
|
|
|
|
|
fn sha256_hex_file_errors_on_missing_file() {
|
|
|
|
|
let dir = tempfile::tempdir().unwrap();
|
|
|
|
|
let missing = dir.path().join("does-not-exist.bin");
|
|
|
|
|
assert!(sha256_hex_file(&missing).is_err());
|
|
|
|
|
}
|
Close the loop: build, sanity-check, and publish for the first time
Implements the last unimplemented pipeline stage from SPEC.md: PKGBUILD
generation + makepkg (builder.rs), a post-build version sanity check
(sanity.rs), and repo-add publishing (publisher.rs), wired into main.rs
for both the tier 1-3 auto-publish path and a new tier 4-6 review queue
(`pkgwatch review` / `pkgwatch review <name> --approve`, persisted via
state::{load,save,clear}_pending_version, tracked separately from
last-published-version since approving one release isn't a standing
auto-publish grant for future ones).
Publishing targets an existing, already-registered local pacman repo
(~/.local/share/pacman/custom, `[custom]` in /etc/pacman.conf) rather
than one pkgwatch invents — found already in real use for a hand-packaged
AppImage, which resolves SPEC's open question on where the repo lives
without pkgwatch ever touching pacman.conf. Publishing stops at
`repo-add`; actually installing/upgrading (`pacman -Syu`/`pacman -S`) is
left to the operator, not run automatically.
Getting a real second package (scaleway-cli, tier 4) through the new
pipeline immediately surfaced a real gap: its pacman package is named
`scaleway-cli` but the actual binary is `scw` (confirmed via `pacman -Ql`
against the currently-installed extra package) — without a way to
declare that, the build would install alongside extra's package under
the wrong name instead of shadowing it. Added `Package::binary_name`
(config.rs) to cover it.
Every upstream-controlled string (version, asset name, download URL)
is validated before it touches generated shell content in the PKGBUILD
template — rejects anything containing a single quote or newline, since
values are embedded in single-quoted bash strings.
Verified for real, end to end: uv (tier 2) auto-built and published
against the real astral-sh/uv release with no human step; scaleway-cli
(tier 4) queued for review, then approved via `pkgwatch review
scaleway-cli --approve`, which re-verified, built, and published it —
confirmed the built package contains exactly usr/bin/scw. Both landed in
the real custom repo's database. Left scaleway-cli's real-repo review
pending rather than approving it myself: the tier 4-6 gate exists for a
human judgment call, not the agent's.
69 tests, cargo make ci clean (fmt, clippy, complexity, coverage, audit).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 09:13:13 +00:00
|
|
|
}
|