Apply self-review feedback on the Forgejo source

Fix stale ARCHITECTURE/config docs, soften source.rs's overclaim, trim the
SPEC's crate paragraph (no brittle counts), and merge the duplicate
same-origin Package test helper into test_support.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

docs/ARCHITECTURE.md

@@ -61,8 +61,8 @@ practice rather than asserted from habit — see Further reading.
    Same testability goal as #3, applied to the specific ways this program
    reaches outside itself. A consistent shape beats ad hoc mocking invented
    per call site.
-   **Already in force**: `GithubEndpoints` (checker/fetcher/verifier),
+   **Already in force**: `source::Endpoints` (checker), the plain API root
-   `repo_add_bin` and the pacman.conf path (publisher), `PKGWATCH_REPO_DIR`
+   it hands to fetcher/verifier, `repo_add_bin` and the pacman.conf path (publisher), `PKGWATCH_REPO_DIR`
    (main, for manual dry runs against a scratch repo instead of the real
    one). A new external call follows the same shape: production code calls
    a thin wrapper with the real default; tests call the parameterized

docs/SPEC.md

@@ -382,18 +382,14 @@ Open questions on the schema:
   GitHub's confirm-each-tag step. `check_interval`/per-package cadence not
   wired up yet — checks are a fixed hourly tick.)*
 
-  Both hosts' HTTP is hand-rolled on the `reqwest` already in the tree,
+  The HTTP is hand-rolled on the `reqwest` already in the tree, not an
-  not an API-client crate: what pkgwatch needs is two `GET`s
+  API-client crate: pkgwatch needs two `GET`s, GitHub's check deliberately
-  (latest-release, release-by-tag), GitHub's check deliberately uses the
+  uses an Atom feed no API crate covers (to stay off the rate-limited REST
-  Atom feed that no API crate covers (to stay off the rate-limited REST
+  API), and the release-by-tag call is shared verbatim by both hosts.
-  API), and the release-by-tag call is shared verbatim between the two
+  `octocrab` is async against our blocking `reqwest` with a default tree
-  hosts, which two per-host crates would split in two. `octocrab` is
+  larger than pkgwatch's whole current one; `forgejo-api` is a generated
-  async/tokio/hyper against this project's blocking `reqwest`, and its
+  binding of the entire API for one endpoint. Revisit if pkgwatch needs
-  default tree alone (217 crates) is larger than all of pkgwatch's today
+  authenticated or write API calls.
-  (143); `forgejo-api` has a `sync` feature but is a generated binding of
-  the whole Forgejo API for one endpoint. Revisit if pkgwatch ever needs
-  authenticated or write API calls (e.g. publishing its own releases from
-  code rather than CI).
 - **Fetcher**: downloads the artifact (and any checksum/signature/
   attestation companion) for a resolved version. *(Implemented —
   `src/fetcher.rs`, via the GitHub or Forgejo releases API — same

src/config.rs

@@ -34,7 +34,7 @@ pub struct Package {
     /// (the API lives under `/api/v1`). Required for, and only meaningful
     /// with, `source = "forgejo-release"`.
     pub base_url: Option<String>,
-    /// Exact GitHub release asset name (still not a glob — see
+    /// Exact release asset name (still not a glob — see
     /// docs/SPEC.md > Architecture > Fetcher), optionally containing a
     /// `{version}` placeholder for projects whose asset names embed the
     /// version (e.g. `scaleway-cli_{version}_linux_amd64`). Substituted via

src/pipeline.rs

@@ -347,6 +347,7 @@ fn approve(state_dir: &Path, work_dir: &Path, name: &str, pkg: &Package, tag: &s
 mod tests {
     use super::*;
     use crate::github::GithubEndpoints;
+    use crate::test_support::same_origin_package;
 
     #[test]
     fn decide_tier_action_failed_verification_overrides_everything() {
@@ -414,22 +415,6 @@ mod tests {
         ]
     }
 
-    /// `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!(
-            r#"
-            repo = "o/r"
-            asset_pattern = "thing.tar.gz"
-            {pkg_toml_extra}
-            [verification]
-            method = "same-origin-sha256"
-            checksum_asset_pattern = "SHA256SUMS"
-            "#
-        ))
-        .unwrap()
-    }
-
     /// Runs `process_package` for a package whose checksum is wrong and
     /// asserts it errors with "verification failed" while leaving no trace
     /// in state.
@@ -497,9 +482,8 @@ mod tests {
             .create();
         let _release_mocks = mock_release_with_bad_checksum(&mut server);
 
-        let pkg = same_origin_package(
+        // The package's own `source` is irrelevant here: `endpoints` is
-            "source = \"forgejo-release\"\nbase_url = \"https://forge.example.com\"",
+        // hand-built to point at the mock server, bypassing `for_package`.
-        );
+        assert_verification_failure_is_an_error(&endpoints, &same_origin_package(""));
-        assert_verification_failure_is_an_error(&endpoints, &pkg);
     }
 }

src/source.rs

@@ -1,6 +1,6 @@
 //! Maps a package's configured `source` to the endpoints the pipeline
-//! stages talk to. The only module that knows how each hosting service
+//! stages talk to. `checker` and `fetcher` take what it hands them and
-//! lays out its URLs; `checker` and `fetcher` take what it hands them.
+//! build their own paths under it.
 
 use crate::config::{Package, Source};
 use crate::github::GithubEndpoints;
@@ -16,10 +16,8 @@ pub enum Endpoints {
 }
 
 impl Endpoints {
-    /// Errors only if a `forgejo-release` package has no `base_url`, which
+    /// Defensive: errors only if a `forgejo-release` package has no
-    /// `config::load_packages_dir` already rejects — this is the same check
+    /// `base_url`, which `config::load_packages_dir` already guarantees.
-    /// again for a `Package` built some other way, not a second source of
-    /// truth.
     pub fn for_package(pkg: &Package) -> Result<Self> {
         match pkg.source {
             Source::GithubRelease => Ok(Endpoints::Github(GithubEndpoints::default())),
@@ -50,20 +48,7 @@ impl Endpoints {
 #[cfg(test)]
 mod tests {
     use super::*;
-
+    use crate::test_support::same_origin_package as package;
-    fn package(extra: &str) -> Package {
-        toml::from_str(&format!(
-            r#"
-            repo = "o/r"
-            asset_pattern = "x"
-            {extra}
-            [verification]
-            method = "same-origin-sha256"
-            checksum_asset_pattern = "SUMS"
-            "#
-        ))
-        .unwrap()
-    }
 
     #[test]
     fn github_source_uses_real_github() {

src/test_support.rs

@@ -1,10 +1,11 @@
 //! Test-only fixture helpers shared across modules' `#[cfg(test)]` code
-//! (`publisher`, `sanity`, `notifier`) — not production code, and not built outside
+//! — not production code, and not built outside `cargo test`. See
-//! `cargo test`. See docs/ARCHITECTURE.md > "organize by pipeline stage, not
+//! docs/ARCHITECTURE.md > "organize by pipeline stage, not by layer": this
-//! by layer": this exists to remove one specific piece of duplication
+//! exists to remove specific pieces of duplication (near-identical copies
-//! (two near-identical copies of "write an executable shell script"), not
+//! of "write an executable shell script" and of "build a same-origin
-//! as a general test-utils dump.
+//! `Package` from TOML"), not as a general test-utils dump.
 
+use crate::config::Package;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 use std::time::{Duration, Instant};
@@ -52,3 +53,22 @@ fn wait_until_executable(path: &Path) {
         }
     }
 }
+
+/// A `same-origin-sha256` `Package` for repo `o/r`, asset `thing.tar.gz`,
+/// checksum asset `SHA256SUMS`. `extra` is spliced in as top-level keys
+/// before the `[verification]` table (e.g. `source`/`base_url`), and is
+/// parsed directly rather than through `config::load_packages_dir`, so it
+/// skips load-time validation.
+pub(crate) fn same_origin_package(extra: &str) -> Package {
+    toml::from_str(&format!(
+        r#"
+        repo = "o/r"
+        asset_pattern = "thing.tar.gz"
+        {extra}
+        [verification]
+        method = "same-origin-sha256"
+        checksum_asset_pattern = "SHA256SUMS"
+        "#
+    ))
+    .unwrap()
+}