pkgwatch/docs/ARCHITECTURE.md

# pkgwatch — code organization

Status: written 2026-09-17, once the build/publish pipeline PR gave this
project enough real code to have actual conventions worth writing down,
instead of guessing at them in advance.

This is distinct from `SPEC.md`, which is the product design (what
pkgwatch does and why). This file is about how the *code* implementing
that design is organized, so it stays readable as it grows past PoC size
instead of quietly accumulating debt. Researched against current industry
practice rather than asserted from habit — see Further reading.

## Principles

1. **One module, one job — and say what it is, up front.**
   Ousterhout's "deep modules": the best modules expose a lot of
   functionality through a simple interface, hiding the complexity behind
   it. The two failure modes he names — *change amplification* (one
   conceptual change forces edits in many places) and *obscurity* (a
   reader can't tell where responsibility lives) — are both symptoms of
   modules that don't have one clear job.
   **Rule**: every `src/*.rs` file opens with a `//!` doc comment stating
   its one responsibility in a sentence. If it can't be one sentence, the
   module is doing too much.
   **Example already here**: `builder.rs`'s job is "turn an
   already-downloaded, already-verified artifact into a built package." It
   hides PKGBUILD templating, upstream-string validation, and the
   `makepkg` invocation behind one `build()` call — none of that leaks to
   callers.

2. **Organize by pipeline stage (feature), not by technical layer.**
   The package-by-feature vs. package-by-layer research is consistent:
   feature-based grouping gives high cohesion within a module and low
   coupling between modules; layer-based grouping (`models/`, `utils/`,
   `helpers/`) tends toward the opposite, and a single feature change ends
   up touching files scattered across every layer.
   **Rule**: modules are named after what they do in the pipeline
   (`checker`, `fetcher`, `verifier`, `builder`, `sanity`, `publisher`,
   `state`), not generic buckets. A new pipeline stage gets a new module
   named after the stage, not a method bolted onto an existing one.
   **Anti-example to keep watching for**: a `utils.rs` grab-bag. `hash.rs`
   could look like one but isn't — it exists for exactly one piece of
   shared logic (`sha256_hex`) that two real stages (`verifier`,
   `builder`) both need, not as a place to dump unrelated helpers.

3. **Separate pure decision logic from I/O ("functional core, imperative
   shell").**
   A function that decides *and* does in the same body can't be tested
   without standing up everything the "does" half touches — often a
   network call, a subprocess, or the filesystem. Pulling the decision out
   into its own pure function makes it trivially unit-testable and makes
   the I/O half thin enough that it obviously matches the decision.
   **Applied this PR**: `pipeline::process_package`'s tier dispatch
   (publish now / still pending / newly pending / verification failed) was
   originally inline in a function that also made the real network and
   build calls. Pulled out into `decide_tier_action`, a pure function with
   its own unit tests covering all four outcomes, no I/O involved.

4. **Every network, subprocess, filesystem-root, or environment boundary
   is injectable.**
   Same testability goal as #3, applied to the specific ways this program
   reaches outside itself. A consistent shape beats ad hoc mocking invented
   per call site.
   **Already in force**: `GithubEndpoints` (checker/fetcher/verifier),
   `repo_add_bin` and the pacman.conf path (publisher), `PKGWATCH_REPO_DIR`
   (main, for manual dry runs against a scratch repo instead of the real
   one). A new external call follows the same shape: production code calls
   a thin wrapper with the real default; tests call the parameterized
   version with a fake.

5. **`main.rs` is a dispatcher, not the program.**
   Found by looking at this project's own `main.rs`: it grew to 278 lines
   and zero tests over the course of one PR, because "it's just the entry
   point" is an easy excuse to skip separating logic from wiring — even
   though Rust doesn't actually stop you from unit-testing a binary
   crate's `main.rs`. The Rust community convention of splitting
   entry-point parsing from application logic exists precisely so the
   logic ends up somewhere it's normal to test.
   **Rule**: `main.rs` may parse `argv`, build shared clients, and print
   output. It must not contain a pipeline decision, a network/subprocess
   call, or anything with a test worth writing — that belongs in
   `pipeline.rs`.
   **Applied this PR**: moved `process_package`, `fetch_and_verify`,
   `build_and_publish`, `run_review`, and `approve` out of `main.rs` into a
   new `pipeline.rs`, leaving `main.rs` as argument dispatch only.

6. **Validate at the boundary, once — don't scatter checks.**
   Already stated project-wide (see the user's global instructions: don't
   validate scenarios that can't happen, validate at system boundaries).
   **Example already here**: `builder.rs`'s `validate_pkgname`/
   `validate_pkgver`/`validate_shell_safe` run once, at PKGBUILD-generation
   time, against every upstream-controlled string — not sprinkled through
   whatever code happens to produce those strings.

7. **Don't build generality the currently-tracked packages don't need.**
   Already the load-bearing design principle in `SPEC.md` ("a small fixed
   set of PKGBUILD shapes," "extend when a third real shape shows up").
   Restated here because it's also a tech-debt principle in its own right:
   speculative abstraction is debt too — every future reader has to
   understand it whether or not it's ever exercised.

8. **Every non-obvious structural decision gets one sentence of "why,"
   inline.**
   Standard tech-debt-prevention advice is to keep Architecture Decision
   Records; a single-crate personal tool doesn't need a `docs/adr/`
   directory, but the same information — why this way and not the obvious
   alternative — needs to live somewhere a future reader will actually see
   it: the doc comment on the thing itself.
   **Example already here**: `checker.rs`'s doc comment on
   `latest_github_release` explains why the newest Atom-feed entry isn't
   trusted outright (scaleway-cli's `-dbg1` tag has no real Release behind
   it) — the reasoning lives right next to the code it justifies, not in a
   commit message or a separate design doc no one will find later.

## What's machine-enforced vs. what isn't

`cargo make ci` (format, clippy, cognitive-complexity threshold, coverage,
audit) mechanically enforces what's checkable: style, a handful of lint
categories, a complexity ceiling, and that coverage doesn't quietly
regress. It does **not** enforce module cohesion, naming, or "is this
logic in the right module" — those stay code-review questions. Worth
being honest about that boundary rather than implying CI catches
everything above.

## Further reading

- [A Philosophy of Software Design — deep modules & information hiding, summary](https://medium.com/swlh/a-philosophy-of-software-design-by-john-ousterhout-4a00d0ff9f1c)
- [Package by feature vs. package by layer](https://medium.com/@felixnjunge78/package-by-feature-vs-package-by-layer-which-one-wins-11ee03921fed)
- [Coupling and cohesion as the foundations of a maintainable codebase](https://medium.com/@iamprovidence/coupling-and-cohesion-foundations-that-affect-your-entire-codebase-77d06d44af0d)
- [Rust module and crate organization best practices](https://softwarepatternslexicon.com/rust/idiomatic-rust-patterns/module-and-crate-organization-best-practices/)
- [Reducing technical debt in 2026 — IBM](https://www.ibm.com/think/insights/reduce-technical-debt)