From 3eb9ccda3996c05d3cf21bd27b8a7f0e5fb47c44 Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 17:50:31 +0200 Subject: [PATCH 01/13] Modernize Rust workspace and simplify checker and renderer --- .github/workflows/ci.yml | 7 +- .github/workflows/pr-check.yml | 2 +- .github/workflows/render.yml | 2 +- Makefile | 16 +- ci/Cargo.lock | 148 +++--- ci/Cargo.toml | 46 +- ci/pr-check/Cargo.toml | 49 +- ci/pr-check/src/criteria.rs | 471 +++++++++++++++++++ ci/pr-check/src/main.rs | 730 ++---------------------------- ci/pr-check/src/network.rs | 233 ++++++++++ ci/pr-check/src/report.rs | 268 +++++++++++ ci/render/Cargo.toml | 60 +-- ci/render/src/bin/main.rs | 256 ++++++----- ci/render/src/deprecation.rs | 174 +++++++ ci/render/src/lib.rs | 220 ++------- ci/render/src/lints.rs | 40 +- ci/render/src/regression_tests.rs | 287 ++++++++++++ ci/render/src/types.rs | 54 +-- rust-toolchain.toml | 2 +- 19 files changed, 1861 insertions(+), 1204 deletions(-) create mode 100644 ci/pr-check/src/criteria.rs create mode 100644 ci/pr-check/src/network.rs create mode 100644 ci/pr-check/src/report.rs create mode 100644 ci/render/src/deprecation.rs create mode 100644 ci/render/src/regression_tests.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 96008bb3fa..0c5e57b6a6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,8 +56,11 @@ jobs: - name: Install Rust toolchain uses: dtolnay/rust-toolchain@6c977a6ca4077a0ceb28ffbe03f59d46e9ac8772 # master with: - toolchain: 1.98.0 - components: clippy + toolchain: 1.98.1 + components: clippy, rustfmt + + - name: Check formatting + run: make fmt-check - name: Run Clippy run: make clippy diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index d81ae66a1a..3bed4cb94f 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -75,7 +75,7 @@ jobs: if: steps.tools.outputs.found == 'true' uses: dtolnay/rust-toolchain@6c977a6ca4077a0ceb28ffbe03f59d46e9ac8772 # master with: - toolchain: 1.98.0 + toolchain: 1.98.1 - name: Build trusted checker if: steps.tools.outputs.found == 'true' diff --git a/.github/workflows/render.yml b/.github/workflows/render.yml index e2ebe9ab4f..bf9a920fbb 100644 --- a/.github/workflows/render.yml +++ b/.github/workflows/render.yml @@ -19,7 +19,7 @@ jobs: - name: Install Rust toolchain uses: dtolnay/rust-toolchain@6c977a6ca4077a0ceb28ffbe03f59d46e9ac8772 # master with: - toolchain: 1.98.0 + toolchain: 1.98.1 - name: Render list run: make render diff --git a/Makefile b/Makefile index 7b61a0939c..d2a3985e34 100644 --- a/Makefile +++ b/Makefile @@ -1,35 +1,39 @@ # Static Analysis Tools Repository Makefile -.PHONY: render render-skip-deprecated check clippy fmt test clean help +.PHONY: render render-skip-deprecated check clippy fmt fmt-check test clean help # Default target shows help help: @echo "Available targets:" @echo " render - Render README.md and JSON API from YAML sources" - @echo " render-skip-deprecated - Render without deprecated tools" + @echo " render-skip-deprecated - Render using cached deprecation data (no GitHub requests)" @echo " check - Run cargo check" @echo " clippy - Run clippy lints" @echo " fmt - Format Rust code" + @echo " fmt-check - Check Rust formatting without changing files" @echo " test - Run tests" @echo " clean - Clean build artifacts" @echo " help - Show this help" # Main rendering targets render: - cargo run --manifest-path ci/Cargo.toml -p render -- --tags data/tags.yml --tools data/tools --collections data/collections --md-out README.md --json-out data/api + cargo run --manifest-path ci/Cargo.toml --locked -p render -- --tags data/tags.yml --tools data/tools --collections data/collections --md-out README.md --json-out data/api render-skip-deprecated: - cargo run --manifest-path ci/Cargo.toml -p render -- --tags data/tags.yml --tools data/tools --collections data/collections --md-out README.md --json-out data/api --skip-deprecated + cargo run --manifest-path ci/Cargo.toml --locked -p render -- --tags data/tags.yml --tools data/tools --collections data/collections --md-out README.md --json-out data/api --skip-deprecated # Development targets check: - cargo check --manifest-path ci/Cargo.toml + cargo check --manifest-path ci/Cargo.toml --workspace --all-targets --locked clippy: cargo clippy --manifest-path ci/Cargo.toml --workspace --all-targets --all-features --locked -- -D warnings fmt: - cargo fmt --manifest-path ci/Cargo.toml + cargo fmt --manifest-path ci/Cargo.toml --all + +fmt-check: + cargo fmt --manifest-path ci/Cargo.toml --all --check test: cargo test --manifest-path ci/Cargo.toml --workspace --all-targets --all-features --locked diff --git a/ci/Cargo.lock b/ci/Cargo.lock index 478dae7896..7ec916a5e0 100644 --- a/ci/Cargo.lock +++ b/ci/Cargo.lock @@ -68,7 +68,7 @@ dependencies = [ "rustc-hash", "serde", "serde_derive", - "syn 3.0.4", + "syn 3.0.6", ] [[package]] @@ -107,9 +107,9 @@ checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" [[package]] name = "aws-lc-rs" -version = "1.18.0" +version = "1.18.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ce2b2dcc879c3bae0d371e77c99f2238400ef24ec001394befa67b6e543add9e" +checksum = "b281d307588d634de920874890732659e2e7672f72b5e10e81badc1a8a83621e" dependencies = [ "aws-lc-sys", "zeroize", @@ -117,9 +117,9 @@ dependencies = [ [[package]] name = "aws-lc-sys" -version = "0.44.0" +version = "0.45.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f09fae7be8bb3174e05c6afdb34199e6dc0c7c04ba9fa237b1967adfbde27483" +checksum = "9bff6c3b54fad79a2e60b8102caf565819711497c1f5f092f49508e2f5c31b27" dependencies = [ "cc", "cmake", @@ -151,9 +151,9 @@ dependencies = [ [[package]] name = "bitflags" -version = "2.13.1" +version = "2.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b588b76d00fde79687d7646a9b5bdf3cc0f655e0bbd080335a95d7e96f3587da" +checksum = "3ded4057c258ba199e2d26386d3af3780957ecaee6c4ef4041c6b4b8b97c0b06" [[package]] name = "bumpalo" @@ -169,9 +169,9 @@ checksum = "fc652a48c352aef3ea3aed32080501cf3ef6ed5da78602a020c991775b0aff04" [[package]] name = "cc" -version = "1.4.4" +version = "1.4.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0ad534f4357a5264cce5019c989cf66a4f0dc4e0d1b1d15f8aacec0ff7360273" +checksum = "a3eb0f42d6c360dc3f8a821f6bf2fdea7f72bfd36b3076eb0e6d1e9e0752fff4" dependencies = [ "find-msvc-tools", "jobserver", @@ -181,9 +181,9 @@ dependencies = [ [[package]] name = "cfg-if" -version = "1.0.4" +version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" +checksum = "4e7648175b45a9a48536d676f68d918270699102aa8dab5496df06904c914600" [[package]] name = "cfg_aliases" @@ -290,7 +290,7 @@ checksum = "c6232dd377dcc64799954cbd3a9bb882e9cdc1308ccd87b1c098f1fb2eaf82a8" dependencies = [ "proc-macro2", "quote", - "syn 3.0.4", + "syn 3.0.6", ] [[package]] @@ -325,9 +325,9 @@ dependencies = [ [[package]] name = "find-msvc-tools" -version = "0.1.11" +version = "0.1.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d45db016d36b838f563236e9193d0ee6ce38f3f68b6c94e914b4929c96bbb890" +checksum = "3e0f1c7c3a72c66fd80abe965175f7523475c0489a87d3ff9d6e8c87d87a9d2d" [[package]] name = "form_urlencoded" @@ -412,9 +412,9 @@ checksum = "e4eba85ea1d0a966a983acd07deee566e67395d2d96b6fb39e62b5a833f1eb0b" [[package]] name = "granit-parser" -version = "1.2.1" +version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "48aa83cc6ac4dc610adf5501a5392b4bcc543b2c1f24eaf77469a96e31ec9abe" +checksum = "e20f99e46474f56bd905c56e817ebddcf377a611f94c53ac4649e4d3fa3c0cd0" dependencies = [ "arraydeque", "smallvec", @@ -649,9 +649,9 @@ dependencies = [ [[package]] name = "ipnet" -version = "2.12.1" +version = "2.12.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6a756c3fac73139e83f14c2d742155dd2b78d3ee56597b419a0579b7bdd6dd78" +checksum = "791930b43c0d5973160d90a8f3894509f2b273430f5c5c73b668636d0287c5c0" [[package]] name = "itoa" @@ -720,9 +720,9 @@ dependencies = [ [[package]] name = "js-sys" -version = "0.3.104" +version = "0.3.105" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0e0c1080212aad755ea003d18543e8768dd432c48819efd73a7bf1e39b7a5a3a" +checksum = "ce57d20d1ea864ce2ac172ab472d409214f4fd359f0b2a2775abdf522e2af99e" dependencies = [ "cfg-if", "futures-util", @@ -749,9 +749,9 @@ checksum = "f9f8bd3e56ce4dfc153cf470fffbfa98c7620958b312ca5c3a4b8d5181fd13c6" [[package]] name = "lru-slab" -version = "0.1.2" +version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "112b39cec0b298b6c1999fee3e31427f74f676e4cb9879ed1a121b43661a4154" +checksum = "4050469837a6ff301cd14c1f8f24f88549e6d548f24f64e2148eb0f72cebc51f" [[package]] name = "memchr" @@ -761,9 +761,9 @@ checksum = "cf8baf1c55e62ffcace7a9f06f4bd9cd3f0c4beb022d3b367256b91b87513d98" [[package]] name = "mio" -version = "1.2.2" +version = "1.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "30d65c71f1ce40ab09135ce117d742b9f8a19ff91a41a8b57ed50bc2de59c427" +checksum = "4b18443e9c262bfe8fa82f51666e2642c53393f7e5c27b3e1aeab922cff5b9d8" dependencies = [ "libc", "wasi", @@ -788,7 +788,7 @@ dependencies = [ "proc-macro2", "quote", "rustversion", - "syn 3.0.4", + "syn 3.0.6", ] [[package]] @@ -858,7 +858,6 @@ dependencies = [ "anyhow", "askama", "chrono", - "pico-args", "reqwest", "serde", "serde-saphyr", @@ -876,9 +875,9 @@ dependencies = [ [[package]] name = "quinn" -version = "0.11.11" +version = "0.11.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0c1a41e437b6bbd489372cd4971de128e85c855f56c57f283d20ff016cf7c0a8" +checksum = "4051e23e9185c255a7e33ef59cdbca87a22d359052eecd22fc6b901fb37d9d11" dependencies = [ "bytes", "cfg_aliases", @@ -896,9 +895,9 @@ dependencies = [ [[package]] name = "quinn-proto" -version = "0.11.17" +version = "0.11.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "04759210543be93709136e28212294a659ef5001836ff4eab4d663e4529bba83" +checksum = "a9746dbde176634f4f2f1faf2404e30a31b2bc1e9cafb5329c95d8177a18c9fc" dependencies = [ "aws-lc-rs", "bytes", @@ -1056,9 +1055,9 @@ dependencies = [ [[package]] name = "rustls" -version = "0.23.43" +version = "0.23.45" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0283386ce02abc0151e1761d08802dfe86c173b0b494af5cbc086574e453da06" +checksum = "0d41d731c7d2f962d1ccc364cec258de3c0e93b38c2fb3ba97ac74513048d634" dependencies = [ "aws-lc-rs", "once_cell", @@ -1200,9 +1199,9 @@ dependencies = [ [[package]] name = "serde-saphyr" -version = "1.2.0" +version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3afb591f9cdb6223c88ba39269aff895620c7f0716dc42b705b5733d5c7c0823" +checksum = "b8050abb251097357e24aff63ba2c52a6309ecb7d23a5474023df960a02694d8" dependencies = [ "annotate-snippets", "encoding_rs_io", @@ -1229,7 +1228,7 @@ checksum = "e7a5d71263a5a7d47b41f6b3f06ba276f10cc18b0931f1799f710578e2309348" dependencies = [ "proc-macro2", "quote", - "syn 3.0.4", + "syn 3.0.6", ] [[package]] @@ -1285,9 +1284,9 @@ dependencies = [ [[package]] name = "smallvec" -version = "1.15.2" +version = "1.16.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ed6a63f02c8539c91a8685a86f4099661ba3da017932f6ebbea6de3f0fa7c90" +checksum = "ba467056f1b547ed52077911161fc86985becbc60e8e1857c8a144dab0def891" [[package]] name = "socket2" @@ -1324,9 +1323,9 @@ dependencies = [ [[package]] name = "syn" -version = "3.0.4" +version = "3.0.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6275cddf4610d1775e6d1fe9469b2e77d0f39fd98fb7450901b821e0c53649f" +checksum = "8593e8e72159ed2257d083c7a454a85cbf854f37a0966d8d483aff8c8a3ebcee" dependencies = [ "proc-macro2", "quote", @@ -1344,13 +1343,13 @@ dependencies = [ [[package]] name = "synstructure" -version = "0.13.2" +version = "0.14.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "728a70f3dbaf5bab7f0c4b1ac8d7ae5ea60a4b5549c8a5914361c99147a709d2" +checksum = "901704edd0dfe137f1987838ee4f259e4e063c31371bdb423f7ae38ec6f77f02" dependencies = [ "proc-macro2", "quote", - "syn 2.0.119", + "syn 3.0.6", ] [[package]] @@ -1391,7 +1390,7 @@ checksum = "bc04cd3e1236dd4a98afca4569f2deb3f120e5422a4023be2cb683f8486292af" dependencies = [ "proc-macro2", "quote", - "syn 3.0.4", + "syn 3.0.6", ] [[package]] @@ -1406,18 +1405,9 @@ dependencies = [ [[package]] name = "tinyvec" -version = "1.12.0" +version = "1.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bb4ebadaa0af04fab11ae01eb5f9fdb5f9c5b875506e210e71c07873528baa7f" -dependencies = [ - "tinyvec_macros", -] - -[[package]] -name = "tinyvec_macros" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" +checksum = "fd3ca314f692efd6c868f8408f53fe444634a845f96c028b97d35f6a1f79f0ee" [[package]] name = "tokio" @@ -1442,14 +1432,14 @@ checksum = "78773a2a397f451582ce068015985c33193cf6dea8b74d2a639fe457b2f07b0e" dependencies = [ "proc-macro2", "quote", - "syn 3.0.4", + "syn 3.0.6", ] [[package]] name = "tokio-rustls" -version = "0.26.4" +version = "0.26.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1729aa945f29d91ba541258c8df89027d5792d85a8841fb65e8bf0f4ede4ef61" +checksum = "b0c85f2c3ef0b1cd58b36682f4b17aaa995f0e5db534d85692b4903abce21f67" dependencies = [ "rustls", "tokio", @@ -1527,9 +1517,9 @@ checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" [[package]] name = "unicode-ident" -version = "1.0.24" +version = "1.0.26" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" +checksum = "d245f478577f809a851594d02313b640fb437e0bb33866753cff937863096954" [[package]] name = "unicode-width" @@ -1588,9 +1578,9 @@ checksum = "ccf3ec651a847eb01de73ccad15eb7d99f80485de043efb2f370cd654f4ea44b" [[package]] name = "wasm-bindgen" -version = "0.2.127" +version = "0.2.128" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1b70935747edd64d89de3efa29d73789b806c15798f8e7dca4d8ac356b50ce70" +checksum = "aecb87a33d3b0c5e3b7aa46336eaf486cffafbd281b195e4c8b80d50df2351bf" dependencies = [ "cfg-if", "once_cell", @@ -1601,9 +1591,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-futures" -version = "0.4.77" +version = "0.4.78" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6b7777d5cc23d0e91404e53ce2d5e8ec7acae3026b16233dba62cd3246457950" +checksum = "6ef4c5d3d2cdf5c54f4231181768f5510842e350db025faf1f7163b1030ed928" dependencies = [ "js-sys", "wasm-bindgen", @@ -1611,9 +1601,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro" -version = "0.2.127" +version = "0.2.128" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "77775f8f3f7217702089053b94958f8f54061a3f663417df76e19cbdcca29bc1" +checksum = "a690d511e3c1a8b3a55e33511e3c2c00c78415cd23650f32b808627f5696b9ed" dependencies = [ "quote", "wasm-bindgen-macro-support", @@ -1621,31 +1611,31 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro-support" -version = "0.2.127" +version = "0.2.128" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e11d33f857dc2fb11b8bc75aee111aa9cbeb12cd9f25efd3d4c2a3dd4e235284" +checksum = "411e4887f0071ef2d2164a9d5fdf2d20efbef78fccd3a78b0c10a1dc5295e48a" dependencies = [ "bumpalo", "proc-macro2", "quote", - "syn 2.0.119", + "syn 3.0.6", "wasm-bindgen-shared", ] [[package]] name = "wasm-bindgen-shared" -version = "0.2.127" +version = "0.2.128" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7ef64dbcc55df09c7e5a46182d181c2cfa3e925f3da937ea764728b4bbb9dcbf" +checksum = "81941cd78d0c92026c33e5e01312845a4cb1e9af3407f9134b100dd03144103e" dependencies = [ "unicode-ident", ] [[package]] name = "web-sys" -version = "0.3.104" +version = "0.3.105" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c435338968042f4f59a557f690a253676d47ce13ceb55d70100e7facf6620a30" +checksum = "9fbddc4a036f00ec4f18c83445bd3115cb306a91da554919a099d9222fe4a7f8" dependencies = [ "js-sys", "wasm-bindgen", @@ -1859,13 +1849,13 @@ dependencies = [ [[package]] name = "yoke-derive" -version = "0.8.2" +version = "0.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "de844c262c8848816172cef550288e7dc6c7b7814b4ee56b3e1553f275f1858e" +checksum = "33811428bee40dbceb6d545e95754741d17a6aef9a4849f0fd62e2ba4f412a78" dependencies = [ "proc-macro2", "quote", - "syn 2.0.119", + "syn 3.0.6", "synstructure", ] @@ -1880,13 +1870,13 @@ dependencies = [ [[package]] name = "zerofrom-derive" -version = "0.1.7" +version = "0.1.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "11532158c46691caf0f2593ea8358fed6bbf68a0315e80aae9bd41fbade684a1" +checksum = "f75b4683f6c7f45248d4d64056a24298c6281e0993356d7d1b4a1a962ef10d4a" dependencies = [ "proc-macro2", "quote", - "syn 2.0.119", + "syn 3.0.6", "synstructure", ] @@ -1926,7 +1916,7 @@ checksum = "34df6fc39dbd26ddc9c10e6a2984476e13acce22e64e4487636ef494369225da" dependencies = [ "proc-macro2", "quote", - "syn 3.0.4", + "syn 3.0.6", ] [[package]] diff --git a/ci/Cargo.toml b/ci/Cargo.toml index ffd78c5a7f..72452219f3 100644 --- a/ci/Cargo.toml +++ b/ci/Cargo.toml @@ -1,20 +1,48 @@ [workspace] -members = [ - "render", - "pr-check", -] -resolver = "2" +members = ["render", "pr-check"] +resolver = "3" [workspace.package] -rust-version = "1.98" +edition = "2024" +rust-version = "1.98.1" +license = "MIT" +repository = "https://github.com/analysis-tools-dev/static-analysis" +publish = false [workspace.dependencies] anyhow = "1.0.104" askama = "0.16" chrono = { version = "0.4.45", features = ["serde"] } pico-args = "0.5" -reqwest = { version = "0.13.4", default-features = false, features = ["json", "rustls", "system-proxy"] } +reqwest = { version = "0.13.5", default-features = false, features = ["json", "rustls", "system-proxy"] } serde = { version = "1.0.229", features = ["derive"] } serde_json = "1.0.151" -serde-saphyr = { version = "1.2", default-features = false, features = ["deserialize"] } -tokio = { version = "1.53.1", features = ["rt-multi-thread", "macros"] } +serde-saphyr = { version = "1.3", default-features = false, features = ["deserialize"] } +tokio = { version = "1.53.1", features = ["rt", "macros"] } + +[workspace.lints.rust] +unsafe_code = "forbid" + +[workspace.lints.clippy] +correctness = { level = "deny", priority = -1 } +style = { level = "warn", priority = -1 } +complexity = { level = "warn", priority = -1 } +perf = { level = "warn", priority = -1 } +suspicious = { level = "warn", priority = -1 } +cargo = { level = "warn", priority = -1 } +pedantic = { level = "warn", priority = -1 } +nursery = { level = "warn", priority = -1 } +missing_errors_doc = "warn" +missing_panics_doc = "warn" +unwrap_used = "deny" +expect_used = "warn" +panic = "deny" +unimplemented = "deny" +unreachable = "deny" +todo = "warn" +dbg_macro = "warn" +# TLS and target-support compatibility crates intentionally coexist. +multiple_crate_versions = "allow" +module_name_repetitions = "allow" +similar_names = "allow" +too_many_lines = "allow" diff --git a/ci/pr-check/Cargo.toml b/ci/pr-check/Cargo.toml index 94a0c8257a..e1c916a1ae 100644 --- a/ci/pr-check/Cargo.toml +++ b/ci/pr-check/Cargo.toml @@ -1,43 +1,22 @@ [package] name = "pr-check" version = "0.1.0" -edition = "2024" +edition.workspace = true rust-version.workspace = true description = "Checks pull requests against contributing criteria" -license = "MIT" -repository = "https://github.com/analysis-tools-dev/static-analysis" -publish = false +license.workspace = true +repository.workspace = true +publish.workspace = true -[lints.clippy] -correctness = { level = "deny", priority = -1 } -style = { level = "warn", priority = -1 } -complexity = { level = "warn", priority = -1 } -perf = { level = "warn", priority = -1 } -suspicious = { level = "warn", priority = -1 } -cargo = { level = "warn", priority = -1 } -pedantic = { level = "warn", priority = -1 } -nursery = { level = "warn", priority = -1 } -missing_errors_doc = "warn" -missing_panics_doc = "warn" -unwrap_used = "deny" -expect_used = "warn" -panic = "deny" -unimplemented = "deny" -unreachable = "deny" -todo = "warn" -dbg_macro = "warn" -# TLS and target-support compatibility crates intentionally coexist. -multiple_crate_versions = "allow" -module_name_repetitions = "allow" -similar_names = "allow" -too_many_lines = "allow" +[lints] +workspace = true [dependencies] -anyhow = { workspace = true } -askama = { workspace = true } -chrono = { workspace = true } -pico-args = { workspace = true } -serde = { workspace = true } -serde-saphyr = { workspace = true } -tokio = { workspace = true } -reqwest = { workspace = true } \ No newline at end of file +anyhow.workspace = true +askama.workspace = true +chrono.workspace = true + +reqwest.workspace = true +serde.workspace = true +serde-saphyr.workspace = true +tokio.workspace = true diff --git a/ci/pr-check/src/criteria.rs b/ci/pr-check/src/criteria.rs new file mode 100644 index 0000000000..ebc442ca35 --- /dev/null +++ b/ci/pr-check/src/criteria.rs @@ -0,0 +1,471 @@ +//! Contribution criteria and URL classification, independent of network access. + +use anyhow::{Context, Result}; +use chrono::{DateTime, Months, Utc}; +use serde::Deserialize; + +use crate::report::{CheckResult, ToolReport}; + +/// A minimal tool entry parsed from `data/tools/.yml`. +/// Only the fields needed for the contributing criteria check are required. +#[derive(Debug, Deserialize)] +pub struct ToolEntry { + pub name: String, + pub source: Option, + pub homepage: Option, +} + +/// Response from `GET /repos/{owner}/{repo}`. +#[derive(Debug, Deserialize)] +pub struct RepoInfo { + pub stargazers_count: u64, + pub created_at: DateTime, +} + +/// One item from `GET /repos/{owner}/{repo}/contributors`. +#[derive(Debug, Deserialize)] +pub struct Contributor { + login: String, + #[serde(rename = "type")] + account_type: String, +} + +impl Contributor { + pub fn counts_as_human(&self) -> bool { + let login = self.login.to_ascii_lowercase(); + self.account_type.eq_ignore_ascii_case("User") + && !login.ends_with("[bot]") + && !AUTOMATION_LOGINS.contains(&login.as_str()) + } +} + +// Some automation accounts are reported as ordinary users by GitHub. +// Use exact logins rather than broad patterns that could exclude human contributors. +const AUTOMATION_LOGINS: &[&str] = &["claude", "dependabot", "renovate-bot"]; + +const MIN_STARS: u64 = 20; +const MIN_CONTRIBUTORS: usize = 2; +const MIN_AGE_MONTHS: u32 = 6; + +/// Parses `owner` and `repo` out of a GitHub URL like +/// `https://github.com/owner/repo` or `https://github.com/owner/repo/`. +/// Returns `None` for non-GitHub URLs or malformed paths. +pub fn parse_github_repo(url: &str) -> Option<(&str, &str)> { + let url = url.trim_end_matches('/'); + let without_scheme = url + .strip_prefix("https://github.com/") + .or_else(|| url.strip_prefix("http://github.com/"))?; + + let (owner, repo) = without_scheme.split_once('/')?; + if owner.is_empty() || repo.is_empty() || repo.contains('/') { + return None; + } + Some((owner, repo)) +} + +/// Evaluate fetched metadata without performing I/O; unavailable checks require review. +pub fn repository_report( + tool: &ToolEntry, + repo_result: &Result>, + contributors_result: Result>, + now: DateTime, +) -> Result { + let stars_check = match repo_result { + Ok(Some(info)) => { + let s = info.stargazers_count; + if s >= MIN_STARS { + CheckResult::Pass(format!("{s} stars")) + } else { + CheckResult::Fail(format!("{s} stars (minimum is {MIN_STARS})")) + } + } + Ok(None) => CheckResult::Skip("repository not found".into()), + Err(e) => CheckResult::Skip(format!("Could not fetch repo info: {e}")), + }; + + let age_check = match repo_result { + Ok(Some(info)) => { + let minimum_created_at = now + .checked_sub_months(Months::new(MIN_AGE_MONTHS)) + .context("Current date cannot be shifted back by six months")?; + let days = now.signed_duration_since(info.created_at).num_days(); + + if info.created_at <= minimum_created_at { + CheckResult::Pass(format!("created {days} days ago (at least 6 months)")) + } else { + let eligible_at = info + .created_at + .checked_add_months(Months::new(MIN_AGE_MONTHS)) + .with_context(|| { + format!( + "Repository creation date {} cannot be shifted forward by {MIN_AGE_MONTHS} months", + info.created_at + ) + })?; + let remaining = eligible_at.signed_duration_since(now).num_days().max(1); + CheckResult::Fail(format!( + "created {days} days ago, needs {remaining} more days to meet the 6-month minimum" + )) + } + } + Ok(None) => CheckResult::Skip("repository not found".into()), + Err(_) => CheckResult::Skip("Could not determine age (repo info unavailable)".into()), + }; + + let contributors_check = match contributors_result { + Ok(Some(count)) => { + if count >= MIN_CONTRIBUTORS { + CheckResult::Pass(format!("{count} human contributors")) + } else { + CheckResult::Fail(format!( + "{count} human contributor(s) (minimum is {MIN_CONTRIBUTORS})" + )) + } + } + Ok(None) => CheckResult::Skip("repository not found".into()), + Err(e) => CheckResult::Skip(format!("Could not fetch contributors: {e}")), + }; + + let repo_not_found = matches!(repo_result, Ok(None)); + let note = repo_not_found.then_some( + "The source URL returned a 404. Please check that the repository exists and is public.", + ); + + Ok(ToolReport { + name: tool.name.clone(), + source: tool.source.clone(), + stars: stars_check, + contributors: contributors_check, + age: age_check, + domain: None, + note: note.map(str::to_owned), + }) +} + +pub fn homepage_domain(homepage: &str) -> Option { + let url = reqwest::Url::parse(homepage).ok()?; + if !matches!(url.scheme(), "https" | "http") { + return None; + } + let domain = url.domain()?.trim_end_matches('.'); + // Do not fall back to parent domains: their age may belong to a hosting provider. + Some(domain.strip_prefix("www.").unwrap_or(domain).to_owned()) +} + +pub fn domain_age_result( + domain: &str, + registered: DateTime, + now: DateTime, +) -> CheckResult { + let Some(eligible) = registered.checked_add_months(Months::new(MIN_AGE_MONTHS)) else { + return CheckResult::Skip("Invalid domain registration date".into()); + }; + if registered > now { + return CheckResult::Skip( + "Domain registration date is in the future; manual review required".into(), + ); + } + let message = format!( + "The homepage domain `{domain}` was registered on {} and reaches the six-month minimum on {}.", + registered.format("%B %-d, %Y"), + eligible.format("%B %-d, %Y") + ); + if now < eligible { + CheckResult::Fail(message) + } else { + CheckResult::Pass(format!( + "Domain registered on {} (at least six months ago). Service age still requires manual review.", + registered.format("%B %-d, %Y") + )) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn excludes_non_user_account_types() { + for account_type in ["Bot", "bot", "Organization", "unknown"] { + let contributor = Contributor { + login: "otherwise-ordinary-name".into(), + account_type: account_type.into(), + }; + assert!(!contributor.counts_as_human(), "{account_type}"); + } + } + + #[test] + fn keeps_humans_with_similar_names() { + for login in [ + "alice", + "claude-smith", + "dependabot-maintainer", + "robotics-researcher", + "human-bot", + ] { + let contributor = Contributor { + login: login.into(), + account_type: "User".into(), + }; + assert!(contributor.counts_as_human(), "{login}"); + } + } + + #[test] + fn human_and_automation_do_not_meet_contributor_minimum() -> Result<()> { + let contributors: Vec = serde_saphyr::from_str( + "- {login: alice, type: User}\n- {login: claude, type: User}\n- {login: 'dependabot[bot]', type: Bot}\n- {login: renovate-bot, type: User}\n- {login: bob, type: User}", + )?; + let count = + |accounts: &[Contributor]| accounts.iter().filter(|c| c.counts_as_human()).count(); + assert_eq!(count(&contributors[..4]), 1); + assert!(count(&contributors[..4]) < MIN_CONTRIBUTORS); + assert_eq!(count(&contributors), MIN_CONTRIBUTORS); + Ok(()) + } + + #[test] + fn parses_plain_github_url() { + let result = parse_github_repo("https://github.com/owner/repo"); + assert_eq!(result, Some(("owner", "repo"))); + } + + #[test] + fn parses_trailing_slash() { + let result = parse_github_repo("https://github.com/owner/repo/"); + assert_eq!(result, Some(("owner", "repo"))); + } + + #[test] + fn rejects_subpath() { + let result = parse_github_repo("https://github.com/owner/repo/tree/main/subdir"); + assert!(result.is_none()); + } + + #[test] + fn rejects_gitlab() { + let result = parse_github_repo("https://gitlab.com/owner/repo"); + assert!(result.is_none()); + } + + #[test] + fn rejects_missing_repo() { + let result = parse_github_repo("https://github.com/owner"); + assert!(result.is_none()); + } + + #[test] + fn homepage_domains_are_not_reduced_to_hosting_providers() { + assert_eq!( + homepage_domain("https://www.battletest.dev/path"), + Some("battletest.dev".into()) + ); + assert_eq!( + homepage_domain("https://tool.github.io"), + Some("tool.github.io".into()) + ); + assert_eq!( + homepage_domain("https://app.example.co.uk"), + Some("app.example.co.uk".into()) + ); + for url in [ + "not a URL", + "file:///tmp/tool", + "https://127.0.0.1", + "https://[::1]", + ] { + assert!(homepage_domain(url).is_none()); + } + } + + #[test] + fn domain_age_uses_six_calendar_months() -> Result<()> { + let registered = "2026-05-01T20:44:07Z".parse::>()?; + let before = "2026-11-01T20:44:06Z".parse::>()?; + let boundary = "2026-11-01T20:44:07Z".parse::>()?; + let result = domain_age_result("battletest.dev", registered, before); + assert!(result.is_fail()); + assert!(result.message().contains("registered on May 1, 2026")); + assert!(result.message().contains("minimum on November 1, 2026")); + assert!(domain_age_result("battletest.dev", registered, boundary).is_pass()); + assert!(matches!( + domain_age_result("battletest.dev", boundary, registered), + CheckResult::Skip(_) + )); + Ok(()) + } + + #[test] + fn excludes_automation_even_when_github_reports_a_user() { + for login in [ + "claude", + "Claude", + "dependabot", + "Dependabot", + "renovate-bot", + "RENOVATE-BOT", + "dependabot[bot]", + "github-actions[bot]", + "copilot[bot]", + "coderabbitai[bot]", + "some-new-app[BOT]", + ] { + let contributor = Contributor { + login: login.into(), + account_type: "User".into(), + }; + assert!(!contributor.counts_as_human(), "{login}"); + } + } + + fn example_tool() -> ToolEntry { + ToolEntry { + name: "Example".into(), + source: Some("https://github.com/example/tool".into()), + homepage: None, + } + } + + #[test] + fn repository_thresholds_and_calendar_age_boundary() -> Result<()> { + let created_at = "2026-03-01T12:00:00Z".parse::>()?; + let boundary = "2026-09-01T12:00:00Z".parse::>()?; + for stars in [19, 20, 21] { + for contributors in [0, 1, 2, 3] { + for seconds in [-1, 0, 1] { + let report = repository_report( + &example_tool(), + &Ok(Some(RepoInfo { + stargazers_count: stars, + created_at, + })), + Ok(Some(contributors)), + boundary + chrono::Duration::seconds(seconds), + )?; + assert_eq!(report.stars.is_pass(), stars >= 20); + assert_eq!(report.contributors.is_pass(), contributors >= 2); + assert_eq!(report.age.is_pass(), seconds >= 0); + assert_eq!( + report.should_close(), + stars < 20 || contributors < 2 || seconds < 0 + ); + if seconds == -1 { + assert!(report.age.message().contains("needs 1 more days")); + } + } + } + } + Ok(()) + } + + #[test] + fn repository_age_preserves_month_end_cutoff() -> Result<()> { + // The repository rule subtracts six calendar months from the check date. + let now = "2026-08-31T12:00:00Z".parse::>()?; + for (created, passes) in [ + ("2026-02-28T12:00:00Z", true), + ("2026-02-28T12:00:01Z", false), + ] { + let report = repository_report( + &example_tool(), + &Ok(Some(RepoInfo { + stargazers_count: 20, + created_at: created.parse()?, + })), + Ok(Some(2)), + now, + )?; + assert_eq!(report.age.is_pass(), passes); + } + Ok(()) + } + + #[test] + fn missing_or_unavailable_metadata_is_not_a_verified_failure() -> Result<()> { + let now = "2026-09-01T12:00:00Z".parse::>()?; + let missing = repository_report(&example_tool(), &Ok(None), Ok(None), now)?; + assert_eq!(missing.status(), "REVIEW"); + assert_eq!(missing.stars.message(), "repository not found"); + assert_eq!(missing.age.message(), "repository not found"); + assert_eq!(missing.contributors.message(), "repository not found"); + assert!( + missing + .note + .as_deref() + .is_some_and(|note| note.contains("404")) + ); + + let unavailable = repository_report( + &example_tool(), + &Err(anyhow::anyhow!("rate limited")), + Err(anyhow::anyhow!("connection failed")), + now, + )?; + assert_eq!(unavailable.status(), "REVIEW"); + assert_eq!( + unavailable.stars.message(), + "Could not fetch repo info: rate limited" + ); + assert_eq!( + unavailable.age.message(), + "Could not determine age (repo info unavailable)" + ); + assert_eq!( + unavailable.contributors.message(), + "Could not fetch contributors: connection failed" + ); + assert!(unavailable.note.is_none()); + + let verified = repository_report( + &example_tool(), + &Err(anyhow::anyhow!("rate limited")), + Ok(Some(1)), + now, + )?; + assert_eq!(verified.status(), "FAIL"); + Ok(()) + } + + #[test] + fn unavailable_contributors_do_not_hide_verified_repository_failures() -> Result<()> { + let now = "2026-09-01T12:00:00Z".parse::>()?; + for (stars, created, expected) in [ + (20, "2020-01-01T00:00:00Z", "REVIEW"), + (19, "2020-01-01T00:00:00Z", "FAIL"), + (20, "2026-08-01T00:00:00Z", "FAIL"), + ] { + let report = repository_report( + &example_tool(), + &Ok(Some(RepoInfo { + stargazers_count: stars, + created_at: created.parse()?, + })), + Err(anyhow::anyhow!("API unavailable")), + now, + )?; + assert_eq!(report.status(), expected); + } + Ok(()) + } + + #[test] + fn github_url_classification_preserves_supported_forms() { + for url in [ + "http://github.com/owner/repo", + "https://github.com/owner/repo///", + ] { + assert_eq!(parse_github_repo(url), Some(("owner", "repo"))); + } + for url in [ + "https://github.com/", + "https://github.com//repo", + "https://github.com/owner/", + "https://github.com/owner/repo//tree/main", + "https://github.com.evil/owner/repo", + "git@github.com:owner/repo.git", + ] { + assert_eq!(parse_github_repo(url), None, "{url}"); + } + } +} diff --git a/ci/pr-check/src/main.rs b/ci/pr-check/src/main.rs index ada6f43789..fedd79ba6f 100644 --- a/ci/pr-check/src/main.rs +++ b/ci/pr-check/src/main.rs @@ -21,230 +21,18 @@ //! `GITHUB_TOKEN` - a token for reading public repository metadata //! `COMMENT_OUTPUT_FILE` - (optional) report output path; defaults to stdout -use anyhow::{Context, Result, bail}; -use askama::Template; -use chrono::{DateTime, Months, Utc}; -use serde::Deserialize; +mod criteria; +mod network; +mod report; +use anyhow::{Context, Result}; use std::env; use std::path::{Path, PathBuf}; +use std::process::ExitCode; -/// A minimal tool entry parsed from `data/tools/.yml`. -/// Only the fields needed for the contributing criteria check are required. -#[derive(Debug, Deserialize)] -struct ToolEntry { - name: String, - source: Option, - homepage: Option, -} - -/// Response from `GET /repos/{owner}/{repo}`. -#[derive(Debug, Deserialize)] -struct RepoInfo { - stargazers_count: u64, - created_at: DateTime, -} - -/// One item from `GET /repos/{owner}/{repo}/contributors`. -#[derive(Debug, Deserialize)] -struct Contributor { - login: String, - #[serde(rename = "type")] - account_type: String, -} - -impl Contributor { - fn counts_as_human(&self) -> bool { - let login = self.login.to_ascii_lowercase(); - self.account_type.eq_ignore_ascii_case("User") - && !login.ends_with("[bot]") - && !AUTOMATION_LOGINS.contains(&login.as_str()) - } -} - -// Some automation accounts are reported as ordinary users by GitHub. -// Use exact logins rather than broad patterns that could exclude human contributors. -const AUTOMATION_LOGINS: &[&str] = &["claude", "dependabot", "renovate-bot"]; - -const MIN_STARS: u64 = 20; -const MIN_CONTRIBUTORS: usize = 2; -const MIN_AGE_MONTHS: u32 = 6; - -// Identifies the report as output from the contribution checker. -const COMMENT_MARKER: &str = ""; - -/// The outcome of one criterion check. -#[derive(Debug)] -enum CheckResult { - Pass(String), - Fail(String), - Skip(String), -} - -impl CheckResult { - const fn is_pass(&self) -> bool { - matches!(self, Self::Pass(_)) - } - - const fn is_fail(&self) -> bool { - matches!(self, Self::Fail(_)) - } - - const fn symbol(&self) -> &'static str { - match self { - Self::Pass(_) => "pass", - Self::Fail(_) => "fail", - Self::Skip(_) => "skip", - } - } - - fn message(&self) -> &str { - match self { - Self::Pass(m) | Self::Fail(m) | Self::Skip(m) => m, - } - } -} - -/// All checks for a single tool. -#[derive(Debug)] -struct ToolReport { - name: String, - source: Option, - stars: CheckResult, - contributors: CheckResult, - age: CheckResult, - /// Domain registration is evidence for review, not proof of service age. - domain: Option, - /// Explains checks that require manual review. - note: Option, -} - -impl ToolReport { - const fn any_fail(&self) -> bool { - !self.stars.is_pass() || !self.contributors.is_pass() || !self.age.is_pass() - } - - const fn should_close(&self) -> bool { - let repository_age_failed = self.age.is_fail() && self.domain.is_none(); - self.stars.is_fail() || self.contributors.is_fail() || repository_age_failed - } - - const fn status(&self) -> &'static str { - if self.should_close() { - "FAIL" - } else if self.any_fail() { - "REVIEW" - } else { - "PASS" - } - } -} - -#[derive(Template)] -#[template(path = "comment.md")] -struct CommentTemplate<'a> { - marker: &'a str, - reports: &'a [ToolReport], - any_failures: bool, - should_close: bool, -} - -struct GithubClient { - client: reqwest::Client, - token: String, -} - -impl GithubClient { - /// Creates a new client. - /// - /// # Errors - /// - /// Returns an error if the `reqwest` client cannot be constructed. - fn new(token: String) -> Result { - let client = reqwest::Client::builder() - .user_agent("pr-check-bot/1.0 (analysis-tools-dev)") - .build() - .context("Failed to build HTTP client")?; - Ok(Self { client, token }) - } - - /// Sends an authenticated GET request and deserialises the JSON body. - /// - /// # Errors - /// - /// Returns an error on network failure or if the response cannot be - /// deserialised as `T`. - async fn get Deserialize<'de>>(&self, url: &str) -> Result> { - let resp = self - .client - .get(url) - .bearer_auth(&self.token) - .header("Accept", "application/vnd.github+json") - .header("X-GitHub-Api-Version", "2022-11-28") - .send() - .await - .with_context(|| format!("GET {url} failed"))?; - - let status = resp.status(); - if status == reqwest::StatusCode::NOT_FOUND { - return Ok(None); - } - if !status.is_success() { - let body = resp.text().await.unwrap_or_default(); - bail!("GET {url} returned {status}: {body}"); - } - - resp.json::() - .await - .with_context(|| format!("Failed to deserialise response from {url}")) - .map(Some) - } - - /// Fetches repository metadata. - /// - /// # Errors - /// - /// Returns an error if the API call fails. - async fn repo_info(&self, owner: &str, repo: &str) -> Result> { - let url = format!("https://api.github.com/repos/{owner}/{repo}"); - self.get::(&url).await - } - - /// Counts human contributors among the first 100 GitHub contributor accounts. - /// - /// # Errors - /// - /// Returns an error if the API call fails. - async fn contributor_count(&self, owner: &str, repo: &str) -> Result> { - let url = - format!("https://api.github.com/repos/{owner}/{repo}/contributors?per_page=100&anon=0"); - let Some(contributors) = self.get::>(&url).await? else { - return Ok(None); - }; - let human_count = contributors.iter().filter(|c| c.counts_as_human()).count(); - Ok(Some(human_count)) - } -} - -/// Parses `owner` and `repo` out of a GitHub URL like -/// `https://github.com/owner/repo` or `https://github.com/owner/repo/`. -/// Returns `None` for non-GitHub URLs or malformed paths. -fn parse_github_repo(url: &str) -> Option<(String, String)> { - let url = url.trim_end_matches('/'); - let without_scheme = url - .strip_prefix("https://github.com/") - .or_else(|| url.strip_prefix("http://github.com/"))?; - - let parts: Vec<&str> = without_scheme.splitn(3, '/').collect(); - if parts.len() < 2 || parts[0].is_empty() || parts[1].is_empty() { - return None; - } - // Reject sub-paths inside a repo (e.g. /tree/main/…). - if parts.len() == 3 && !parts[2].is_empty() { - return None; - } - Some((parts[0].to_owned(), parts[1].to_owned())) -} +use criteria::ToolEntry; +use network::{GithubClient, check_tool}; +use report::{render_comment, report_exit_code}; /// Reads and deserialises a single tool YAML file. /// @@ -256,246 +44,24 @@ fn read_tool(path: &Path) -> Result { serde_saphyr::from_reader(f).with_context(|| format!("Cannot parse {}", path.display())) } -/// Runs all contributing-criteria checks for one tool. -/// -/// # Errors -/// -/// Returns an error only for unexpected failures (network, auth). Missing -/// criteria produce `CheckResult::Fail` values, not errors. -async fn check_tool(client: &GithubClient, tool: &ToolEntry) -> Result { - let source = tool.source.clone(); - - let gh_coords = source.as_deref().and_then(parse_github_repo); - - if let Some((owner, repo)) = gh_coords { - let repo_result = client.repo_info(&owner, &repo).await; - let contributors_result = client.contributor_count(&owner, &repo).await; - - let stars_check = match &repo_result { - Ok(Some(info)) => { - let s = info.stargazers_count; - if s >= MIN_STARS { - CheckResult::Pass(format!("{s} stars")) - } else { - CheckResult::Fail(format!("{s} stars (minimum is {MIN_STARS})")) - } - } - Ok(None) => CheckResult::Skip("repository not found".into()), - Err(e) => CheckResult::Skip(format!("Could not fetch repo info: {e}")), - }; - - let age_check = match &repo_result { - Ok(Some(info)) => { - let now = Utc::now(); - let minimum_created_at = now - .checked_sub_months(Months::new(MIN_AGE_MONTHS)) - .context("Current date cannot be shifted back by six months")?; - let days = now.signed_duration_since(info.created_at).num_days(); - - if info.created_at <= minimum_created_at { - CheckResult::Pass(format!("created {days} days ago (at least 6 months)")) - } else { - let eligible_at = info - .created_at - .checked_add_months(Months::new(MIN_AGE_MONTHS)) - .with_context(|| { - format!( - "Repository creation date {} cannot be shifted forward by {MIN_AGE_MONTHS} months", - info.created_at - ) - })?; - let remaining = eligible_at.signed_duration_since(now).num_days().max(1); - CheckResult::Fail(format!( - "created {days} days ago, needs {remaining} more days to meet the 6-month minimum" - )) - } - } - Ok(None) => CheckResult::Skip("repository not found".into()), - Err(_) => CheckResult::Skip("Could not determine age (repo info unavailable)".into()), - }; - - let contributors_check = match contributors_result { - Ok(Some(count)) => { - if count >= MIN_CONTRIBUTORS { - CheckResult::Pass(format!("{count} human contributors")) - } else { - CheckResult::Fail(format!( - "{count} human contributor(s) (minimum is {MIN_CONTRIBUTORS})" - )) - } - } - Ok(None) => CheckResult::Skip("repository not found".into()), - Err(e) => CheckResult::Skip(format!("Could not fetch contributors: {e}")), - }; - - let repo_not_found = matches!(repo_result, Ok(None)); - let note = repo_not_found.then_some( - "The source URL returned a 404. Please check that the repository exists and is public.", - ); - - Ok(ToolReport { - name: tool.name.clone(), - source, - stars: stars_check, - contributors: contributors_check, - age: age_check, - domain: None, - note: note.map(str::to_owned), - }) - } else { - let domain = if source.is_none() { - tool.homepage.as_deref().and_then(homepage_domain) - } else { - None - }; - let age = if let Some(domain) = &domain { - check_domain_age(domain).await - } else { - CheckResult::Skip("No supported homepage domain or GitHub source URL".into()) - }; - let note = "No GitHub source URL found. Please verify the contributing criteria manually. \ - Domain registration dates do not establish when a service launched. \ - If the service previously operated under another domain, please provide evidence of that history."; - - Ok(ToolReport { - name: tool.name.clone(), - source, - stars: CheckResult::Skip("N/A".into()), - contributors: CheckResult::Skip("N/A".into()), - age, - domain, - note: Some(note.into()), - }) - } -} - -#[derive(Debug, Deserialize)] -struct RdapDomain { - #[serde(rename = "ldhName")] - name: String, - #[serde(default)] - events: Vec, -} - -#[derive(Debug, Deserialize)] -#[serde(rename_all = "camelCase")] -struct RdapEvent { - event_action: String, - event_date: DateTime, -} - -fn homepage_domain(homepage: &str) -> Option { - let url = reqwest::Url::parse(homepage).ok()?; - if !matches!(url.scheme(), "https" | "http") { - return None; - } - let domain = url.domain()?.trim_end_matches('.'); - // Do not fall back to parent domains: their age may belong to a hosting provider. - Some(domain.strip_prefix("www.").unwrap_or(domain).to_owned()) -} - -async fn check_domain_age(domain: &str) -> CheckResult { - match fetch_domain_registration(domain).await { - Ok(Some(registered)) => domain_age_result(domain, registered, Utc::now()), - Ok(None) => { - CheckResult::Skip("Domain registration date unavailable; manual review required".into()) - } - Err(error) => CheckResult::Skip(format!("Could not check domain registration: {error}")), - } -} - -async fn fetch_domain_registration(domain: &str) -> Result>> { - // RDAP requests must never carry the GitHub token. rdap.org redirects to the registry. - let client = reqwest::Client::builder() - .user_agent("pr-check-bot/1.0 (analysis-tools-dev)") - .https_only(true) - .timeout(std::time::Duration::from_secs(15)) - .build()?; - let response = client - .get(format!("https://rdap.org/domain/{domain}")) - .send() - .await?; - if response.status() == reqwest::StatusCode::NOT_FOUND { - return Ok(None); - } - let record = response.error_for_status()?.json::().await?; - if !record.name.eq_ignore_ascii_case(domain) { - bail!("RDAP returned a different domain"); - } - Ok(record - .events - .into_iter() - .find(|event| event.event_action == "registration") - .map(|event| event.event_date)) -} - -fn domain_age_result(domain: &str, registered: DateTime, now: DateTime) -> CheckResult { - let Some(eligible) = registered.checked_add_months(Months::new(MIN_AGE_MONTHS)) else { - return CheckResult::Skip("Invalid domain registration date".into()); - }; - if registered > now { - return CheckResult::Skip( - "Domain registration date is in the future; manual review required".into(), - ); - } - let message = format!( - "The homepage domain `{domain}` was registered on {} and reaches the six-month minimum on {}.", - registered.format("%B %-d, %Y"), - eligible.format("%B %-d, %Y") - ); - if now < eligible { - CheckResult::Fail(message) - } else { - CheckResult::Pass(format!( - "Domain registered on {} (at least six months ago). Service age still requires manual review.", - registered.format("%B %-d, %Y") - )) - } -} - -/// Renders all tool reports into a Markdown comment body. -/// -/// # Errors -/// -/// Returns an error if the template fails to render. -fn render_comment(reports: &[ToolReport]) -> Result { - let any_failures = reports.iter().any(ToolReport::any_fail); - CommentTemplate { - marker: COMMENT_MARKER, - reports, - any_failures, - should_close: reports.iter().any(ToolReport::should_close), - } - .render() - .context("Failed to render comment template") +fn is_tool_path(path: &Path) -> bool { + path.starts_with("data/tools") + && matches!( + path.extension().and_then(|extension| extension.to_str()), + Some("yml" | "yaml") + ) } -fn report_exit_code(reports: &[ToolReport]) -> i32 { - if reports.iter().any(ToolReport::should_close) { - 2 - } else { - i32::from(reports.iter().any(ToolReport::any_fail)) - } -} - -#[tokio::main] -async fn main() -> Result<()> { +#[tokio::main(flavor = "current_thread")] +async fn main() -> Result { let token = env::var("GITHUB_TOKEN").context("GITHUB_TOKEN not set")?; // Remaining CLI arguments are the paths to check. // Usage: pr-check data/tools/foo.yml data/tools/bar.yml - let pico = pico_args::Arguments::from_env(); - let tool_paths: Vec = pico - .finish() - .into_iter() + let tool_paths: Vec = env::args_os() + .skip(1) .map(PathBuf::from) - .filter(|p| { - p.starts_with("data/tools") - && matches!( - p.extension().and_then(|extension| extension.to_str()), - Some("yml" | "yaml") - ) - }) + .filter(|path| is_tool_path(path)) .collect(); let client = GithubClient::new(token)?; @@ -530,80 +96,15 @@ async fn main() -> Result<()> { eprintln!( "One or more tools failed or require manual review of the contributing criteria." ); - std::process::exit(exit_code); } - Ok(()) + Ok(ExitCode::from(exit_code)) } #[cfg(test)] mod tests { use super::*; - #[test] - fn excludes_automation_even_when_github_reports_a_user() { - for login in [ - "claude", - "Claude", - "dependabot", - "Dependabot", - "renovate-bot", - "RENOVATE-BOT", - "dependabot[bot]", - "github-actions[bot]", - "copilot[bot]", - "coderabbitai[bot]", - "some-new-app[BOT]", - ] { - let contributor = Contributor { - login: login.into(), - account_type: "User".into(), - }; - assert!(!contributor.counts_as_human(), "{login}"); - } - } - - #[test] - fn excludes_non_user_account_types() { - for account_type in ["Bot", "bot", "Organization", "unknown"] { - let contributor = Contributor { - login: "otherwise-ordinary-name".into(), - account_type: account_type.into(), - }; - assert!(!contributor.counts_as_human(), "{account_type}"); - } - } - - #[test] - fn keeps_humans_with_similar_names() { - for login in [ - "alice", - "claude-smith", - "dependabot-maintainer", - "robotics-researcher", - "human-bot", - ] { - let contributor = Contributor { - login: login.into(), - account_type: "User".into(), - }; - assert!(contributor.counts_as_human(), "{login}"); - } - } - - #[test] - fn human_and_automation_do_not_meet_contributor_minimum() -> Result<()> { - let contributors: Vec = serde_saphyr::from_str( - "- {login: alice, type: User}\n- {login: claude, type: User}\n- {login: 'dependabot[bot]', type: Bot}\n- {login: renovate-bot, type: User}\n- {login: bob, type: User}", - )?; - let count = - |accounts: &[Contributor]| accounts.iter().filter(|c| c.counts_as_human()).count(); - assert_eq!(count(&contributors[..4]), 1); - assert!(count(&contributors[..4]) < MIN_CONTRIBUTORS); - assert_eq!(count(&contributors), MIN_CONTRIBUTORS); - Ok(()) - } - #[test] fn parses_catalog() -> Result<()> { let tools = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../data/tools"); @@ -621,181 +122,34 @@ mod tests { } #[test] - fn parses_plain_github_url() { - let result = parse_github_repo("https://github.com/owner/repo"); - assert_eq!(result, Some(("owner".into(), "repo".into()))); - } - - #[test] - fn parses_trailing_slash() { - let result = parse_github_repo("https://github.com/owner/repo/"); - assert_eq!(result, Some(("owner".into(), "repo".into()))); - } - - #[test] - fn rejects_subpath() { - let result = parse_github_repo("https://github.com/owner/repo/tree/main/subdir"); - assert!(result.is_none()); - } - - #[test] - fn rejects_gitlab() { - let result = parse_github_repo("https://gitlab.com/owner/repo"); - assert!(result.is_none()); - } - - #[test] - fn rejects_missing_repo() { - let result = parse_github_repo("https://github.com/owner"); - assert!(result.is_none()); - } - - fn passing_report() -> ToolReport { - ToolReport { - name: "Example".into(), - source: Some("https://github.com/example/tool".into()), - stars: CheckResult::Pass("20 stars".into()), - contributors: CheckResult::Pass("2 contributors".into()), - age: CheckResult::Pass("at least 6 months".into()), - domain: None, - note: None, - } - } - - #[test] - fn passing_tools_do_not_close_pr() -> Result<()> { - let reports = [passing_report()]; - assert_eq!(report_exit_code(&reports), 0); - let comment = render_comment(&reports)?; - assert!(comment.contains("All tool eligibility criteria passed")); - assert!(!comment.contains("closing this pull request")); - assert_eq!(report_exit_code(&[]), 0); - Ok(()) - } - - #[test] - fn each_verified_failure_closes_pr_and_invites_resubmission() -> Result<()> { - for criterion in 0..3 { - let mut report = passing_report(); - let check = match criterion { - 0 => &mut report.stars, - 1 => &mut report.contributors, - _ => &mut report.age, - }; - *check = CheckResult::Fail("below minimum".into()); - assert_eq!(report.status(), "FAIL"); - let reports = [passing_report(), report]; - assert_eq!(report_exit_code(&reports), 2); - let comment = render_comment(&reports)?; - assert!(comment.contains("closing this pull request")); - assert!(comment.contains("submit a new pull request once all criteria are met")); - } - Ok(()) - } - - #[test] - fn unverified_checks_require_review_not_closure() -> Result<()> { - for reason in ["N/A", "repository not found", "GitHub API unavailable"] { - let mut report = passing_report(); - report.stars = CheckResult::Skip(reason.into()); - report.contributors = CheckResult::Skip(reason.into()); - report.age = CheckResult::Skip(reason.into()); - assert_eq!(report.status(), "REVIEW"); - let reports = [report]; - assert_eq!(report_exit_code(&reports), 1); - let comment = render_comment(&reports)?; - assert!(comment.contains("needs manual review")); - assert!(!comment.contains("closing this pull request")); - } - Ok(()) - } - - #[test] - fn verified_failure_still_closes_when_another_check_is_unverified() { - let mut report = passing_report(); - report.stars = CheckResult::Skip("GitHub API unavailable".into()); - report.contributors = CheckResult::Fail("1 contributor".into()); - assert_eq!(report_exit_code(&[report]), 2); - } - - #[test] - fn homepage_domains_are_not_reduced_to_hosting_providers() { - assert_eq!( - homepage_domain("https://www.battletest.dev/path"), - Some("battletest.dev".into()) - ); - assert_eq!( - homepage_domain("https://tool.github.io"), - Some("tool.github.io".into()) - ); - assert_eq!( - homepage_domain("https://app.example.co.uk"), - Some("app.example.co.uk".into()) - ); - for url in [ - "not a URL", - "file:///tmp/tool", - "https://127.0.0.1", - "https://[::1]", + fn tool_path_filter_accepts_only_catalog_yaml_arguments() { + for path in [ + "data/tools/tool.yml", + "data/tools/tool.yaml", + "data/tools/nested/tool.yml", ] { - assert!(homepage_domain(url).is_none()); - } - } - - #[test] - fn domain_age_uses_six_calendar_months() -> Result<()> { - let registered = "2026-05-01T20:44:07Z".parse::>()?; - let before = "2026-11-01T20:44:06Z".parse::>()?; - let boundary = "2026-11-01T20:44:07Z".parse::>()?; - let result = domain_age_result("battletest.dev", registered, before); - assert!(result.is_fail()); - assert!(result.message().contains("registered on May 1, 2026")); - assert!(result.message().contains("minimum on November 1, 2026")); - assert!(domain_age_result("battletest.dev", registered, boundary).is_pass()); - assert!(matches!( - domain_age_result("battletest.dev", boundary, registered), - CheckResult::Skip(_) - )); - Ok(()) - } - - #[test] - fn domain_checks_require_review_without_closing() -> Result<()> { - for age in [ - CheckResult::Fail("Domain younger than six months".into()), - CheckResult::Pass("Domain older than six months".into()), - CheckResult::Skip("RDAP unavailable".into()), + assert!(is_tool_path(Path::new(path)), "{path}"); + } + for path in [ + "README.md", + "data/tags.yml", + "data/tools-extra/tool.yml", + "data/tools/tool.YML", + "data/tools/tool.json", + "/data/tools/tool.yml", ] { - let mut report = passing_report(); - report.source = None; - report.domain = Some("battletest.dev".into()); - report.stars = CheckResult::Skip("N/A".into()); - report.contributors = CheckResult::Skip("N/A".into()); - report.age = age; - assert!(!report.should_close()); - assert_eq!(report.status(), "REVIEW"); - let reports = [report]; - assert_eq!(report_exit_code(&reports), 1); - let comment = render_comment(&reports)?; - assert!(comment.contains("Homepage domain age")); - assert!(comment.contains("https://rdap.org/domain/battletest.dev")); - assert!(!comment.contains("closing this pull request")); - assert!(comment.contains("needs manual review")); + assert!(!is_tool_path(Path::new(path)), "{path}"); } - Ok(()) - } - - #[test] - fn render_comment_no_files() -> Result<()> { - let comment = render_comment(&[])?; - assert!(comment.contains("No new tool files detected")); - Ok(()) } #[test] - fn render_comment_contains_marker() -> Result<()> { - let comment = render_comment(&[])?; - assert!(comment.contains(COMMENT_MARKER)); + fn tool_yaml_keeps_optional_fields_and_ignores_unrelated_metadata() -> Result<()> { + let tool: ToolEntry = + serde_saphyr::from_str("name: Example\nlicense: MIT\ntags: [rust]\n")?; + assert_eq!(tool.name, "Example"); + assert!(tool.source.is_none()); + assert!(tool.homepage.is_none()); + assert!(serde_saphyr::from_str::("source: https://example.com").is_err()); Ok(()) } } diff --git a/ci/pr-check/src/network.rs b/ci/pr-check/src/network.rs new file mode 100644 index 0000000000..a1e189033e --- /dev/null +++ b/ci/pr-check/src/network.rs @@ -0,0 +1,233 @@ +//! Read-only GitHub and RDAP access and per-tool check orchestration. + +use anyhow::{Context, Result, bail}; +use chrono::{DateTime, Utc}; +use serde::{Deserialize, de::DeserializeOwned}; + +use crate::criteria::{ + Contributor, RepoInfo, ToolEntry, domain_age_result, homepage_domain, parse_github_repo, + repository_report, +}; +use crate::report::{CheckResult, ToolReport}; + +pub struct GithubClient { + client: reqwest::Client, + token: String, +} + +impl GithubClient { + /// Creates a new client. + /// + /// # Errors + /// + /// Returns an error if the `reqwest` client cannot be constructed. + pub fn new(token: String) -> Result { + let client = reqwest::Client::builder() + .user_agent("pr-check-bot/1.0 (analysis-tools-dev)") + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to build HTTP client")?; + Ok(Self { client, token }) + } + + /// Sends an authenticated GET request and deserialises the JSON body. + /// + /// # Errors + /// + /// Returns an error on network failure or if the response cannot be + /// deserialised as `T`. + async fn get(&self, url: &str) -> Result> { + let resp = self + .client + .get(url) + .bearer_auth(&self.token) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .send() + .await + .with_context(|| format!("GET {url} failed"))?; + + let status = resp.status(); + if status == reqwest::StatusCode::NOT_FOUND { + return Ok(None); + } + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + bail!("GET {url} returned {status}: {body}"); + } + + resp.json::() + .await + .with_context(|| format!("Failed to deserialise response from {url}")) + .map(Some) + } + + /// Fetches repository metadata. + /// + /// # Errors + /// + /// Returns an error if the API call fails. + async fn repo_info(&self, owner: &str, repo: &str) -> Result> { + let url = format!("https://api.github.com/repos/{owner}/{repo}"); + self.get::(&url).await + } + + /// Counts human contributors among the first 100 GitHub contributor accounts. + /// + /// # Errors + /// + /// Returns an error if the API call fails. + async fn contributor_count(&self, owner: &str, repo: &str) -> Result> { + let url = + format!("https://api.github.com/repos/{owner}/{repo}/contributors?per_page=100&anon=0"); + Ok(self + .get::>(&url) + .await? + .map(|contributors| contributors.iter().filter(|c| c.counts_as_human()).count())) + } +} + +/// Runs all contributing-criteria checks for one tool. +/// +/// # Errors +/// +/// Returns an error if date arithmetic fails. Network and authentication failures +/// become unverified checks, while verified criteria failures become failed checks. +pub async fn check_tool(client: &GithubClient, tool: &ToolEntry) -> Result { + let source = &tool.source; + + let gh_coords = source.as_deref().and_then(parse_github_repo); + + if let Some((owner, repo)) = gh_coords { + let repo_result = client.repo_info(owner, repo).await; + let contributors_result = client.contributor_count(owner, repo).await; + + repository_report(tool, &repo_result, contributors_result, Utc::now()) + } else { + let domain = if source.is_none() { + tool.homepage.as_deref().and_then(homepage_domain) + } else { + None + }; + let age = if let Some(domain) = &domain { + check_domain_age(domain).await + } else { + CheckResult::Skip("No supported homepage domain or GitHub source URL".into()) + }; + let note = "No GitHub source URL found. Please verify the contributing criteria manually. \ + Domain registration dates do not establish when a service launched. \ + If the service previously operated under another domain, please provide evidence of that history."; + + Ok(ToolReport { + name: tool.name.clone(), + source: source.clone(), + stars: CheckResult::Skip("N/A".into()), + contributors: CheckResult::Skip("N/A".into()), + age, + domain, + note: Some(note.into()), + }) + } +} + +#[derive(Debug, Deserialize)] +struct RdapDomain { + #[serde(rename = "ldhName")] + name: String, + #[serde(default)] + events: Vec, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct RdapEvent { + event_action: String, + event_date: DateTime, +} + +async fn check_domain_age(domain: &str) -> CheckResult { + match fetch_domain_registration(domain).await { + Ok(Some(registered)) => domain_age_result(domain, registered, Utc::now()), + Ok(None) => { + CheckResult::Skip("Domain registration date unavailable; manual review required".into()) + } + Err(error) => CheckResult::Skip(format!("Could not check domain registration: {error}")), + } +} + +async fn fetch_domain_registration(domain: &str) -> Result>> { + // RDAP requests must never carry the GitHub token. rdap.org redirects to the registry. + let client = reqwest::Client::builder() + .user_agent("pr-check-bot/1.0 (analysis-tools-dev)") + .https_only(true) + .timeout(std::time::Duration::from_secs(15)) + .build()?; + let response = client + .get(format!("https://rdap.org/domain/{domain}")) + .send() + .await?; + if response.status() == reqwest::StatusCode::NOT_FOUND { + return Ok(None); + } + let record = response.error_for_status()?.json::().await?; + if !record.name.eq_ignore_ascii_case(domain) { + bail!("RDAP returned a different domain"); + } + Ok(record + .events + .into_iter() + .find(|event| event.event_action == "registration") + .map(|event| event.event_date)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn unsupported_source_does_not_fall_back_to_homepage_age() -> Result<()> { + let client = GithubClient::new("unused-test-token".into())?; + for source in [ + "https://gitlab.com/example/tool", + "", + "https://github.com/owner/repo/tree/main", + ] { + let tool = ToolEntry { + name: "Example".into(), + source: Some(source.into()), + homepage: Some("https://example.invalid".into()), + }; + let report = check_tool(&client, &tool).await?; + assert_eq!(report.status(), "REVIEW"); + assert_eq!(report.source, tool.source); + assert!(report.domain.is_none()); + assert_eq!(report.stars.message(), "N/A"); + assert_eq!(report.contributors.message(), "N/A"); + assert_eq!( + report.age.message(), + "No supported homepage domain or GitHub source URL" + ); + assert!(report.note.as_deref().is_some_and(|note| { + note.contains("Please verify the contributing criteria manually") + })); + } + Ok(()) + } + + #[tokio::test] + async fn missing_source_and_unsupported_homepage_require_review() -> Result<()> { + let client = GithubClient::new("unused-test-token".into())?; + for homepage in [None, Some("file:///tmp/tool"), Some("not a URL")] { + let tool = ToolEntry { + name: "Example".into(), + source: None, + homepage: homepage.map(str::to_owned), + }; + let report = check_tool(&client, &tool).await?; + assert_eq!(report.status(), "REVIEW"); + assert!(report.domain.is_none()); + assert!(!report.should_close()); + } + Ok(()) + } +} diff --git a/ci/pr-check/src/report.rs b/ci/pr-check/src/report.rs new file mode 100644 index 0000000000..ea7207ab75 --- /dev/null +++ b/ci/pr-check/src/report.rs @@ -0,0 +1,268 @@ +//! Report status, Markdown rendering, and workflow exit-code contract. + +use anyhow::{Context, Result}; +use askama::Template; + +// Identifies the report as output from the contribution checker. +const COMMENT_MARKER: &str = ""; + +/// The outcome of one criterion check. +#[derive(Debug)] +pub enum CheckResult { + Pass(String), + Fail(String), + Skip(String), +} + +impl CheckResult { + pub const fn is_pass(&self) -> bool { + matches!(self, Self::Pass(_)) + } + + pub const fn is_fail(&self) -> bool { + matches!(self, Self::Fail(_)) + } + + pub const fn symbol(&self) -> &'static str { + match self { + Self::Pass(_) => "pass", + Self::Fail(_) => "fail", + Self::Skip(_) => "skip", + } + } + + pub fn message(&self) -> &str { + match self { + Self::Pass(m) | Self::Fail(m) | Self::Skip(m) => m, + } + } +} + +/// All checks for a single tool. +#[derive(Debug)] +pub struct ToolReport { + pub name: String, + pub source: Option, + pub stars: CheckResult, + pub contributors: CheckResult, + pub age: CheckResult, + /// Domain registration is evidence for review, not proof of service age. + pub domain: Option, + /// Explains checks that require manual review. + pub note: Option, +} + +impl ToolReport { + pub const fn has_nonpassing_checks(&self) -> bool { + !self.stars.is_pass() || !self.contributors.is_pass() || !self.age.is_pass() + } + + pub const fn should_close(&self) -> bool { + let repository_age_failed = self.age.is_fail() && self.domain.is_none(); + self.stars.is_fail() || self.contributors.is_fail() || repository_age_failed + } + + pub const fn status(&self) -> &'static str { + if self.should_close() { + "FAIL" + } else if self.has_nonpassing_checks() { + "REVIEW" + } else { + "PASS" + } + } +} + +#[derive(Template)] +#[template(path = "comment.md")] +struct CommentTemplate<'a> { + marker: &'a str, + reports: &'a [ToolReport], + any_failures: bool, + should_close: bool, +} + +/// Renders all tool reports into a Markdown comment body. +/// +/// # Errors +/// +/// Returns an error if the template fails to render. +pub fn render_comment(reports: &[ToolReport]) -> Result { + let any_failures = reports.iter().any(ToolReport::has_nonpassing_checks); + CommentTemplate { + marker: COMMENT_MARKER, + reports, + any_failures, + should_close: reports.iter().any(ToolReport::should_close), + } + .render() + .context("Failed to render comment template") +} + +pub fn report_exit_code(reports: &[ToolReport]) -> u8 { + if reports.iter().any(ToolReport::should_close) { + 2 + } else { + u8::from(reports.iter().any(ToolReport::has_nonpassing_checks)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn passing_report() -> ToolReport { + ToolReport { + name: "Example".into(), + source: Some("https://github.com/example/tool".into()), + stars: CheckResult::Pass("20 stars".into()), + contributors: CheckResult::Pass("2 contributors".into()), + age: CheckResult::Pass("at least 6 months".into()), + domain: None, + note: None, + } + } + + #[test] + fn passing_tools_do_not_close_pr() -> Result<()> { + let reports = [passing_report()]; + assert_eq!(report_exit_code(&reports), 0); + let comment = render_comment(&reports)?; + assert!(comment.contains("All tool eligibility criteria passed")); + assert!(!comment.contains("closing this pull request")); + assert_eq!(report_exit_code(&[]), 0); + Ok(()) + } + + #[test] + fn each_verified_failure_closes_pr_and_invites_resubmission() -> Result<()> { + for criterion in 0..3 { + let mut report = passing_report(); + let check = match criterion { + 0 => &mut report.stars, + 1 => &mut report.contributors, + _ => &mut report.age, + }; + *check = CheckResult::Fail("below minimum".into()); + assert_eq!(report.status(), "FAIL"); + let reports = [passing_report(), report]; + assert_eq!(report_exit_code(&reports), 2); + let comment = render_comment(&reports)?; + assert!(comment.contains("closing this pull request")); + assert!(comment.contains("submit a new pull request once all criteria are met")); + } + Ok(()) + } + + #[test] + fn unverified_checks_require_review_not_closure() -> Result<()> { + for reason in ["N/A", "repository not found", "GitHub API unavailable"] { + let mut report = passing_report(); + report.stars = CheckResult::Skip(reason.into()); + report.contributors = CheckResult::Skip(reason.into()); + report.age = CheckResult::Skip(reason.into()); + assert_eq!(report.status(), "REVIEW"); + let reports = [report]; + assert_eq!(report_exit_code(&reports), 1); + let comment = render_comment(&reports)?; + assert!(comment.contains("needs manual review")); + assert!(!comment.contains("closing this pull request")); + } + Ok(()) + } + + #[test] + fn verified_failure_still_closes_when_another_check_is_unverified() { + let mut report = passing_report(); + report.stars = CheckResult::Skip("GitHub API unavailable".into()); + report.contributors = CheckResult::Fail("1 contributor".into()); + assert_eq!(report_exit_code(&[report]), 2); + } + + #[test] + fn domain_checks_require_review_without_closing() -> Result<()> { + for age in [ + CheckResult::Fail("Domain younger than six months".into()), + CheckResult::Pass("Domain older than six months".into()), + CheckResult::Skip("RDAP unavailable".into()), + ] { + let mut report = passing_report(); + report.source = None; + report.domain = Some("battletest.dev".into()); + report.stars = CheckResult::Skip("N/A".into()); + report.contributors = CheckResult::Skip("N/A".into()); + report.age = age; + assert!(!report.should_close()); + assert_eq!(report.status(), "REVIEW"); + let reports = [report]; + assert_eq!(report_exit_code(&reports), 1); + let comment = render_comment(&reports)?; + assert!(comment.contains("Homepage domain age")); + assert!(comment.contains("https://rdap.org/domain/battletest.dev")); + assert!(!comment.contains("closing this pull request")); + assert!(comment.contains("needs manual review")); + } + Ok(()) + } + #[test] + fn render_comment_no_files() -> Result<()> { + let comment = render_comment(&[])?; + assert!(comment.contains("No new tool files detected")); + Ok(()) + } + + #[test] + fn render_comment_contains_marker() -> Result<()> { + let comment = render_comment(&[])?; + assert!(comment.contains(COMMENT_MARKER)); + Ok(()) + } + + #[test] + fn all_check_combinations_preserve_status_comment_and_exit_code() -> Result<()> { + fn check(state: u8) -> CheckResult { + match state { + 0 => CheckResult::Pass("verified".into()), + 1 => CheckResult::Fail("below minimum".into()), + _ => CheckResult::Skip("unavailable".into()), + } + } + for domain in [None, Some("example.com")] { + for stars in 0..3 { + for contributors in 0..3 { + for age in 0..3 { + let report = ToolReport { + stars: check(stars), + contributors: check(contributors), + age: check(age), + domain: domain.map(str::to_owned), + ..passing_report() + }; + let close = + stars == 1 || contributors == 1 || (age == 1 && domain.is_none()); + let review = stars != 0 || contributors != 0 || age != 0; + let (status, exit) = if close { + ("FAIL", 2) + } else if review { + ("REVIEW", 1) + } else { + ("PASS", 0) + }; + assert_eq!(report.status(), status); + let reports = [passing_report(), report]; + assert_eq!(report_exit_code(&reports), exit); + let comment = render_comment(&reports)?; + assert!(comment.starts_with(COMMENT_MARKER)); + assert_eq!(comment.contains("closing this pull request"), close); + assert_eq!(comment.contains("needs manual review"), review && !close); + assert_eq!( + comment.contains("All tool eligibility criteria passed"), + !review + ); + } + } + } + } + Ok(()) + } +} diff --git a/ci/render/Cargo.toml b/ci/render/Cargo.toml index fab1153f5a..96575df036 100644 --- a/ci/render/Cargo.toml +++ b/ci/render/Cargo.toml @@ -2,56 +2,26 @@ name = "render" version = "0.2.0" authors = ["Matthias Endler "] -edition = "2024" +edition.workspace = true rust-version.workspace = true description = "Static analysis tools catalog renderer" -license = "MIT" -repository = "https://github.com/analysis-tools-dev/static-analysis" +license.workspace = true +repository.workspace = true keywords = ["static-analysis", "linting", "tools", "catalog"] categories = ["development-tools"] -publish = false +publish.workspace = true -[lints.clippy] -# Correctness lints (enabled by default, but being explicit) -correctness = { level = "deny", priority = -1 } - -# Style lints -style = { level = "warn", priority = -1 } -complexity = { level = "warn", priority = -1 } -perf = { level = "warn", priority = -1 } -suspicious = { level = "warn", priority = -1 } - -# Additional strict lints -cargo = { level = "warn", priority = -1 } -pedantic = { level = "warn", priority = -1 } -nursery = { level = "warn", priority = -1 } - -# Specific lints we want to enforce -missing_errors_doc = "warn" -missing_panics_doc = "warn" -unwrap_used = "deny" -expect_used = "warn" -panic = "deny" -unimplemented = "deny" -unreachable = "deny" -todo = "warn" -dbg_macro = "warn" -# TLS and target-support compatibility crates intentionally coexist. -multiple_crate_versions = "allow" - -# Allow some pedantic lints that might be too noisy -module_name_repetitions = "allow" -similar_names = "allow" -too_many_lines = "allow" # We'll use the clippy.toml threshold instead +[lints] +workspace = true [dependencies] -anyhow = { workspace = true } -chrono = { workspace = true } -pico-args = { workspace = true } -serde = { workspace = true } -serde_json = { workspace = true } -serde-saphyr = { workspace = true } -tokio = { workspace = true } -askama = { workspace = true } -reqwest = { workspace = true } +anyhow.workspace = true +askama.workspace = true +chrono.workspace = true +pico-args.workspace = true +reqwest.workspace = true +serde.workspace = true +serde_json.workspace = true +serde-saphyr.workspace = true slug = "0.1.6" +tokio.workspace = true diff --git a/ci/render/src/bin/main.rs b/ci/render/src/bin/main.rs index 53021ebe24..b4c30e31ae 100644 --- a/ci/render/src/bin/main.rs +++ b/ci/render/src/bin/main.rs @@ -1,17 +1,18 @@ -use anyhow::{Context, Result}; +use anyhow::{Context, Result, ensure}; use askama::Template; use pico_args::Arguments; use render::types::{Collection, Entry, ParsedEntry, Tag, Tags, Type}; use render::{check_deprecated, create_api, create_catalog}; -use serde::de::DeserializeOwned; +use serde::{Deserialize, Serialize, de::DeserializeOwned}; use slug::slugify; use std::collections::BTreeMap; use std::env; use std::ffi::OsStr; use std::fs; use std::io; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; +#[derive(Debug)] struct Args { tags: PathBuf, tools: PathBuf, @@ -21,91 +22,100 @@ struct Args { skip_deprecated: bool, } +impl Args { + fn parse(mut args: Arguments) -> Result { + let parsed = Self { + tags: args.value_from_os_str("--tags", parse_path)?, + tools: args.value_from_os_str("--tools", parse_path)?, + collections: args.value_from_os_str("--collections", parse_path)?, + md_out: args.value_from_os_str("--md-out", parse_path)?, + json_out: args.value_from_os_str("--json-out", parse_path)?, + skip_deprecated: args.contains("--skip-deprecated"), + }; + let remaining = args.finish(); + ensure!(remaining.is_empty(), "Unexpected arguments: {remaining:?}"); + Ok(parsed) + } +} + // `pico_args::value_from_os_str` requires a fallible parser callback. #[allow(clippy::unnecessary_wraps)] fn parse_path(s: &OsStr) -> Result { Ok(s.into()) } -fn read_tags(path: PathBuf) -> Result { - let f = std::fs::File::open(path)?; - Ok(serde_saphyr::from_reader(f)?) +fn read_yaml(path: &Path) -> Result { + let file = fs::File::open(path).with_context(|| format!("Cannot open {}", path.display()))?; + serde_saphyr::from_reader(file).with_context(|| format!("Cannot parse {}", path.display())) } -fn read_entries(path: PathBuf) -> Result> { - let dir: std::fs::ReadDir = std::fs::read_dir(path)?; - - let files = dir - .map(|res| res.map(|e| e.path())) - .filter(|result| { - result - .as_ref() - .is_ok_and(|path| path.extension().and_then(OsStr::to_str) == Some("yml")) - }) - .collect::, io::Error>>()?; +fn read_entries(path: &Path) -> Result> { + let dir = fs::read_dir(path).with_context(|| format!("Cannot read {}", path.display()))?; + let mut files = dir + .map(|entry| entry.map(|entry| entry.path())) + .collect::>>() + .with_context(|| format!("Cannot list {}", path.display()))?; + files.retain(|path| path.extension().is_some_and(|extension| extension == "yml")); files .iter() - .inspect(|p| println!("Checking {}", p.display())) - .map(|p| { - let file = std::fs::File::open(p)?; - let entry = serde_saphyr::from_reader(file) - .with_context(|| format!("Cannot parse {}", p.display()))?; - Ok(entry) + .map(|path| { + println!("Checking {}", path.display()); + read_yaml(path) }) .collect() } -/// Backfills the deprecated field in the tools data from the old tools data. -fn backfill_deprecated(tools: &mut Vec) -> Result<()> { - let Ok(tools_raw) = fs::read_to_string("data/api/tools.json") else { - return Ok(()); // No old data to backfill from. Skip silently. - }; +#[derive(Deserialize)] +struct CachedTool { + deprecated: Option, +} - let old_tools_data: BTreeMap = serde_json::from_str(&tools_raw)?; +/// Reuses cached markers without overriding explicit YAML decisions. +fn backfill_deprecated(tools: &mut [Entry], path: &Path) -> Result<()> { + let file = match fs::File::open(path) { + Ok(file) => file, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error).with_context(|| format!("Cannot open {}", path.display())), + }; + let cached: BTreeMap = serde_json::from_reader(io::BufReader::new(file)) + .with_context(|| format!("Cannot parse {}", path.display()))?; + apply_deprecation_cache(tools, &cached); + Ok(()) +} - for tool in tools { - let id = slugify(&tool.name); - if let Some(old_tool) = old_tools_data.get(&id) { - // Only backfill deprecated if it's not already set - if tool.deprecated.is_none() { - tool.deprecated = old_tool - .get("deprecated") - .and_then(serde_json::Value::as_bool); - } - } +fn apply_deprecation_cache(tools: &mut [Entry], cached: &BTreeMap) { + for tool in tools.iter_mut().filter(|tool| tool.deprecated.is_none()) { + tool.deprecated = cached + .get(&slugify(&tool.name)) + .and_then(|old| old.deprecated); } - Ok(()) } -#[tokio::main] -async fn main() -> Result<()> { - let mut args = Arguments::from_env(); - let args = Args { - tags: args.value_from_os_str("--tags", parse_path)?, - tools: args.value_from_os_str("--tools", parse_path)?, - collections: args.value_from_os_str("--collections", parse_path)?, - md_out: args.value_from_os_str("--md-out", parse_path)?, - json_out: args.value_from_os_str("--json-out", parse_path)?, - skip_deprecated: args.contains("--skip-deprecated"), - }; +fn write_json(path: &Path, value: &impl Serialize) -> Result<()> { + let json = serde_json::to_vec_pretty(value)?; + fs::write(path, json).with_context(|| format!("Cannot write {}", path.display())) +} - let tags = read_tags(args.tags)?; +#[tokio::main(flavor = "current_thread")] +async fn main() -> Result<()> { + let args = Args::parse(Arguments::from_env())?; + let tags: Tags = read_yaml(&args.tags)?; - let mut collections: Vec = read_entries(args.collections)?; + let mut collections: Vec = read_entries(&args.collections)?; collections.sort_by_cached_key(|collection| collection.name.to_lowercase()); - let parsed_tools: Vec = read_entries(args.tools)?; - let tools: Result> = parsed_tools + let parsed_tools: Vec = read_entries(&args.tools)?; + let mut tools = parsed_tools .into_iter() - .map(|t| Entry::from_parsed(t, &tags)) - .collect(); - let mut tools = tools?; + .map(|tool| Entry::from_parsed(tool, &tags)) + .collect::>>()?; tools.sort(); let should_check_deprecation = !args.skip_deprecated; let github_token = env::var("GITHUB_TOKEN"); + let cache_path = Path::new("data/api/tools.json"); match (should_check_deprecation, github_token) { (true, Ok(token)) => { println!("Checking for deprecated entries on GitHub. This might take a while..."); @@ -113,56 +123,23 @@ async fn main() -> Result<()> { } (true, Err(_)) => { eprintln!("No GITHUB_TOKEN environment variable found. Reusing old deprecation data."); - backfill_deprecated(&mut tools)?; + backfill_deprecated(&mut tools, cache_path)?; } - (false, _) => backfill_deprecated(&mut tools)?, + (false, _) => backfill_deprecated(&mut tools, cache_path)?, } - let languages: Vec = tags - .clone() - .into_iter() - .filter(|t| t.kind == Type::Language) - .collect(); - - let other_tags: Vec = tags.into_iter().filter(|t| t.kind == Type::Other).collect(); + let (languages, other_tags): (Vec, Vec) = + tags.into_iter().partition(|tag| tag.kind == Type::Language); let catalog = create_catalog(&tools, &languages, &other_tags, collections); - fs::write(&args.md_out, catalog.render()?).context(format!( - "Cannot write Markdown output to {}", - args.md_out.display() - ))?; + fs::write(&args.md_out, catalog.render()?) + .with_context(|| format!("Cannot write Markdown output to {}", args.md_out.display()))?; let api = create_api(tools, &languages, &other_tags); - let json = serde_json::to_string_pretty(&api)?; - let tools_out = args.json_out.join("tools.json"); - fs::write(&tools_out, json).context(format!( - "Cannot write tools JSON output to {}", - args.json_out.display() - ))?; - - let mut tags_json = BTreeMap::new(); - tags_json.insert("languages", languages); - tags_json.insert("other", other_tags); - let json = serde_json::to_string_pretty(&tags_json)?; - - let tags_out = args.json_out.join("tags.json"); - fs::write(&tags_out, json).context(format!( - "Cannot write tags JSON output to {}", - args.json_out.display() - ))?; - - // let stats_raw = fs::read_to_string("data/api/stats_raw.json")?; - // let stats: StatsRaw = serde_json::from_str(&stats_raw)?; - - // let stats = format_stats(stats); - // let json = serde_json::to_string(&stats)?; - - // let stats_out = args.json_out.join("stats.json"); - // fs::write(&stats_out, json).context(format!( - // "Cannot write stats JSON output to {}", - // args.json_out.display() - // ))?; + write_json(&args.json_out.join("tools.json"), &api)?; + let tags_json = BTreeMap::from([("languages", languages), ("other", other_tags)]); + write_json(&args.json_out.join("tags.json"), &tags_json)?; Ok(()) } @@ -171,12 +148,85 @@ async fn main() -> Result<()> { mod tests { use super::*; + fn cli_args() -> Vec { + [ + "--tags", + "tags.yml", + "--tools", + "tools", + "--collections", + "collections", + "--md-out", + "README.md", + "--json-out", + "api", + ] + .into_iter() + .map(Into::into) + .collect() + } + + #[test] + fn parses_cli_and_rejects_unknown_or_missing_arguments() -> Result<()> { + let mut args = cli_args(); + args.push("--skip-deprecated".into()); + let parsed = Args::parse(Arguments::from_vec(args))?; + assert!(parsed.skip_deprecated); + assert_eq!(parsed.collections, Path::new("collections")); + + let mut args = cli_args(); + args.push("--skip-deprected".into()); + let error = Args::parse(Arguments::from_vec(args)) + .err() + .context("Expected invalid argument")?; + assert!(error.to_string().contains("--skip-deprected")); + assert!(Args::parse(Arguments::from_vec(vec![])).is_err()); + Ok(()) + } + + #[test] + fn cached_deprecation_never_overrides_explicit_markers() -> Result<()> { + let fixture = r#"{"name":"Example Tool","categories":[],"tags":[],"license":"MIT","types":[],"homepage":"https://example.com","description":"Example"}"#; + for cached_marker in [Some(true), Some(false), None] { + let cached = BTreeMap::from([( + "example-tool".into(), + CachedTool { + deprecated: cached_marker, + }, + )]); + for explicit in [Some(true), Some(false), None] { + let mut tool: Entry = serde_json::from_str(fixture)?; + tool.deprecated = explicit; + apply_deprecation_cache(std::slice::from_mut(&mut tool), &cached); + assert_eq!(tool.deprecated, explicit.or(cached_marker)); + } + } + let mut tool: Entry = serde_json::from_str(fixture)?; + apply_deprecation_cache(std::slice::from_mut(&mut tool), &BTreeMap::new()); + assert_eq!(tool.deprecated, None); + Ok(()) + } + + #[test] + fn file_errors_include_the_source_path() -> Result<()> { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("Cargo.toml"); + let error = read_entries::(&path) + .err() + .context("Expected directory error")?; + assert!(error.to_string().contains(&path.display().to_string())); + let error = backfill_deprecated(&mut [], &path) + .err() + .context("Expected JSON error")?; + assert!(error.to_string().contains(&path.display().to_string())); + Ok(()) + } + #[test] fn parses_catalog() -> Result<()> { let data = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../data"); - let tags = read_tags(data.join("tags.yml"))?; - let tools: Vec = read_entries(data.join("tools"))?; - let collections: Vec = read_entries(data.join("collections"))?; + let tags: Tags = read_yaml(&data.join("tags.yml"))?; + let tools: Vec = read_entries(&data.join("tools"))?; + let collections: Vec = read_entries(&data.join("collections"))?; assert!(!collections.is_empty()); let catalog = create_catalog(&[], &[], &[], collections); let markdown = catalog.render()?; diff --git a/ci/render/src/deprecation.rs b/ci/render/src/deprecation.rs new file mode 100644 index 0000000000..e8b81a8827 --- /dev/null +++ b/ci/render/src/deprecation.rs @@ -0,0 +1,174 @@ +use anyhow::{Context, Result}; +use chrono::{DateTime, Local, NaiveDate, Utc}; +use serde::Deserialize; + +use crate::types::Entry; + +#[derive(Deserialize)] +struct CommitResponse { + commit: Commit, +} + +#[derive(Deserialize)] +struct Commit { + author: CommitAuthor, +} + +#[derive(Deserialize)] +struct CommitAuthor { + date: DateTime, +} + +fn github_coordinates(source: &str) -> Option<(&str, &str)> { + let path = source + .strip_prefix("https://github.com/") + .or_else(|| source.strip_prefix("http://github.com/"))? + .trim_end_matches('/'); + let (owner, repo) = path.split_once('/')?; + (!owner.is_empty() && !repo.is_empty() && !repo.contains('/')).then_some((owner, repo)) +} + +async fn latest_commit_date( + client: &reqwest::Client, + token: &str, + owner: &str, + repo: &str, +) -> Result>> { + let url = format!("https://api.github.com/repos/{owner}/{repo}/commits?per_page=1"); + let response = client + .get(url) + .bearer_auth(token) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .send() + .await + .with_context(|| format!("Failed to fetch commits for {owner}/{repo}"))?; + + if matches!( + response.status(), + reqwest::StatusCode::NOT_FOUND | reqwest::StatusCode::CONFLICT + ) { + return Ok(None); + } + + let commits = response + .error_for_status() + .with_context(|| format!("GitHub rejected the commits request for {owner}/{repo}"))? + .json::>() + .await + .with_context(|| format!("Invalid commits response for {owner}/{repo}"))?; + + Ok(commits + .into_iter() + .next() + .map(|commit| commit.commit.author.date)) +} + +fn deprecation_marker(today: NaiveDate, last_commit: DateTime) -> Option { + // Preserve the existing calendar-date policy: local today versus the UTC + // author's date, not elapsed hours or the commit's committer date. + (today + .signed_duration_since(last_commit.date_naive()) + .num_days() + > 365) + .then_some(true) +} + +/// Refreshes deprecation markers using each GitHub repository's latest commit. +/// +/// Unavailable repositories and failed requests leave their markers unchanged. +/// Request failures are reported to stderr without stopping subsequent checks. +/// +/// # Errors +/// +/// Returns an error when the HTTP client cannot be created. +pub async fn check_deprecated(token: &str, entries: &mut [Entry]) -> Result<()> { + let client = reqwest::Client::builder() + .user_agent("analysis-tools-render/0.2") + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to build GitHub HTTP client")?; + + for entry in entries { + let Some((owner, repo)) = entry.source.as_deref().and_then(github_coordinates) else { + continue; + }; + let last_commit = match latest_commit_date(&client, token, owner, repo).await { + Ok(Some(date)) => date, + Ok(None) => continue, + Err(error) => { + eprintln!("Could not check {owner}/{repo} for deprecation: {error:#}"); + continue; + } + }; + + entry.deprecated = deprecation_marker(Local::now().date_naive(), last_commit); + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_github_repository_urls() { + for source in [ + "https://github.com/owner/repo", + "http://github.com/owner/repo/", + "https://github.com/owner/repo///", + ] { + assert_eq!(github_coordinates(source), Some(("owner", "repo"))); + } + for source in [ + "https://github.com/owner/repo/tree/main", + "https://gitlab.com/owner/repo", + "https://github.com//repo", + "https://github.com/owner/", + "https://github.com/owner", + ] { + assert_eq!(github_coordinates(source), None); + } + } + + #[test] + fn parses_github_author_date_not_committer_date() -> Result<()> { + let response: Vec = serde_json::from_str( + r#"[{"commit":{"author":{"date":"2026-08-01T12:34:56Z"},"committer":{"date":"2026-09-01T00:00:00Z"}}}]"#, + )?; + let date = response + .into_iter() + .next() + .map(|commit| commit.commit.author.date); + assert_eq!( + date.map(|value| value.to_rfc3339()), + Some("2026-08-01T12:34:56+00:00".into()) + ); + Ok(()) + } + + #[test] + fn deprecation_uses_calendar_dates_and_a_strict_365_day_cutoff() -> Result<()> { + let today = "2026-09-17".parse()?; + for (timestamp, expected) in [ + ("2025-09-16T23:59:59Z", Some(true)), + ("2025-09-17T00:00:00Z", None), + ("2025-09-18T00:00:00Z", None), + ("2026-09-17T23:59:59Z", None), + ("2026-09-18T00:00:00Z", None), + ("2025-09-17T00:30:00+01:00", Some(true)), + ] { + assert_eq!( + deprecation_marker(today, timestamp.parse()?), + expected, + "{timestamp}" + ); + } + assert_eq!( + deprecation_marker("2024-03-01".parse()?, "2023-03-01T00:00:00Z".parse()?), + Some(true) + ); + Ok(()) + } +} diff --git a/ci/render/src/lib.rs b/ci/render/src/lib.rs index c66589753b..37afa8f50a 100644 --- a/ci/render/src/lib.rs +++ b/ci/render/src/lib.rs @@ -1,115 +1,17 @@ -use anyhow::{Context, Result}; -use chrono::{DateTime, Local, Utc}; -use serde::Deserialize; use slug::slugify; use stats::StatsRaw; +use std::collections::BTreeMap; +use types::{Api, ApiEntry, Catalog, Collection, Entry, Tag, Type}; -/// Entry validation rules. +mod deprecation; mod lints; pub mod stats; pub mod types; -use std::collections::BTreeMap; -use types::{Api, ApiEntry, Catalog, Collection, Entry, ParsedEntry, Tag, Type}; - -fn valid(entry: &ParsedEntry, tags: &[Tag]) -> Result<()> { - let lints = [lints::name, lints::min_one_tag]; - lints.iter().try_for_each(|lint| lint(entry, tags)) -} - -#[derive(Deserialize)] -struct CommitResponse { - commit: Commit, -} - -#[derive(Deserialize)] -struct Commit { - author: CommitAuthor, -} - -#[derive(Deserialize)] -struct CommitAuthor { - date: DateTime, -} - -fn github_coordinates(source: &str) -> Option<(&str, &str)> { - let path = source - .strip_prefix("https://github.com/") - .or_else(|| source.strip_prefix("http://github.com/"))? - .trim_end_matches('/'); - let (owner, repo) = path.split_once('/')?; - (!owner.is_empty() && !repo.is_empty() && !repo.contains('/')).then_some((owner, repo)) -} - -async fn latest_commit_date( - client: &reqwest::Client, - token: &str, - owner: &str, - repo: &str, -) -> Result>> { - let url = format!("https://api.github.com/repos/{owner}/{repo}/commits?per_page=1"); - let response = client - .get(&url) - .bearer_auth(token) - .header("Accept", "application/vnd.github+json") - .header("X-GitHub-Api-Version", "2022-11-28") - .send() - .await - .with_context(|| format!("Failed to fetch commits for {owner}/{repo}"))?; - - if matches!( - response.status(), - reqwest::StatusCode::NOT_FOUND | reqwest::StatusCode::CONFLICT - ) { - return Ok(None); - } - - let commits = response - .error_for_status() - .with_context(|| format!("GitHub rejected the commits request for {owner}/{repo}"))? - .json::>() - .await - .with_context(|| format!("Invalid commits response for {owner}/{repo}"))?; - - Ok(commits - .into_iter() - .next() - .map(|commit| commit.commit.author.date)) -} - -/// Refreshes deprecation markers using each GitHub repository's latest commit. -/// -/// # Errors -/// -/// Returns an error when the HTTP client cannot be created or GitHub returns an -/// unexpected response. -pub async fn check_deprecated(token: &str, entries: &mut [Entry]) -> Result<()> { - let client = reqwest::Client::builder() - .user_agent("analysis-tools-render/0.2") - .build() - .context("Failed to build GitHub HTTP client")?; - - for entry in entries { - let Some((owner, repo)) = entry.source.as_deref().and_then(github_coordinates) else { - continue; - }; - let last_commit = match latest_commit_date(&client, token, owner, repo).await { - Ok(Some(date)) => date, - Ok(None) => continue, - Err(error) => { - eprintln!("Could not check {owner}/{repo} for deprecation: {error:#}"); - continue; - } - }; - - let duration = Local::now() - .date_naive() - .signed_duration_since(last_commit.date_naive()); - entry.deprecated = (duration.num_days() > 365).then_some(true); - } +pub use deprecation::check_deprecated; - Ok(()) -} +#[cfg(test)] +mod regression_tests; /// Groups normalized entries for the generated README. #[must_use] @@ -121,20 +23,20 @@ pub fn create_catalog( ) -> Catalog { // Multi-language tools get their own primary section instead of being repeated under // every language. They still belong in applicable non-language tag sections. - let (multi, single_language): (Vec, Vec) = - entries.iter().cloned().partition(|entry| { - let language_tags = entry - .tags - .iter() - .filter(|t| t.kind == Type::Language) - .count(); - language_tags > 1 && !entry.is_c_cpp() - }); + let (multi, single_language): (Vec<&Entry>, Vec<&Entry>) = entries.iter().partition(|entry| { + let language_tags = entry + .tags + .iter() + .filter(|t| t.kind == Type::Language) + .count(); + language_tags > 1 && !entry.is_c_cpp() + }); let mut linters = BTreeMap::new(); for language in languages { let list: Vec = single_language .iter() + .copied() .filter(|e| e.tags.contains(language)) .cloned() .collect(); @@ -145,16 +47,20 @@ pub fn create_catalog( let mut others = BTreeMap::new(); for other in other_tags { - let entries_for_tag: &[Entry] = if other.include_multi { + let list: Vec = if other.include_multi { entries + .iter() + .filter(|e| e.tags.contains(other)) + .cloned() + .collect() } else { - &single_language + single_language + .iter() + .copied() + .filter(|e| e.tags.contains(other)) + .cloned() + .collect() }; - let list: Vec = entries_for_tag - .iter() - .filter(|e| e.tags.contains(other)) - .cloned() - .collect(); if !list.is_empty() { others.insert(other.clone(), list); } @@ -163,7 +69,7 @@ pub fn create_catalog( Catalog { linters, others, - multi, + multi: multi.into_iter().cloned().collect(), collections, } } @@ -174,45 +80,27 @@ pub fn create_api(entries: Vec, languages: &[Tag], other_tags: &[Tag]) -> let mut api_entries = BTreeMap::new(); for entry in entries { - // Get the language data for the entry. We iterate over all languages - // and look up each language in the entry tags. This is an O(n) operation - // as we iterate over the language list only once while the lookup is an - // O(1) operation thanks to the tag set. + // Preserve configured tag order rather than the entry's set order. let entry_languages = languages .iter() - .filter_map(|lang| { - if entry.tags.contains(lang) { - entry.tags.get(lang).map(|tag| tag.value.clone()) - } else { - None - } - }) + .filter(|lang| entry.tags.contains(lang)) + .map(|lang| lang.value.clone()) .collect(); - // ...same for the non-language tags let entry_other = other_tags .iter() - .filter_map(|other| { - if entry.tags.contains(other) { - entry.tags.get(other).map(|tag| tag.value.clone()) - } else { - None - } - }) + .filter(|other| entry.tags.contains(other)) + .map(|other| other.value.clone()) .collect(); - // In the future we want to split up licenses in the YAML input files into a list. - // Emulate the future data format by creating a list from the current string. - // Note that this string could contain more than one license name for now, e.g. - // MIT / Apache License - let licenses = vec![entry.license]; - + let key = slugify(&entry.name); let api_entry = ApiEntry { - name: entry.name.clone(), + name: entry.name, categories: entry.categories, languages: entry_languages, other: entry_other, - licenses, + // Compound license strings remain a single API value. + licenses: vec![entry.license], types: entry.types, homepage: entry.homepage, source: entry.source, @@ -226,7 +114,7 @@ pub fn create_api(entries: Vec, languages: &[Tag], other_tags: &[Tag]) -> demos: entry.demos, wrapper: entry.wrapper, }; - api_entries.insert(slugify(&entry.name), api_entry); + api_entries.insert(key, api_entry); } api_entries @@ -251,6 +139,7 @@ pub fn format_stats(stats: StatsRaw) -> BTreeMap { #[cfg(test)] mod tests { use super::*; + use anyhow::{Context, Result}; use askama::Template; use std::collections::BTreeSet; @@ -379,39 +268,6 @@ mod tests { Ok(()) } - #[test] - fn parses_github_repository_urls() { - assert_eq!( - github_coordinates("https://github.com/owner/repo"), - Some(("owner", "repo")) - ); - assert_eq!( - github_coordinates("https://github.com/owner/repo/"), - Some(("owner", "repo")) - ); - assert_eq!( - github_coordinates("https://github.com/owner/repo/tree/main"), - None - ); - assert_eq!(github_coordinates("https://gitlab.com/owner/repo"), None); - } - - #[test] - fn parses_github_commit_response() -> Result<()> { - let response: Vec = - serde_json::from_str(r#"[{"commit":{"author":{"date":"2026-08-01T12:34:56Z"}}}]"#)?; - let date = response - .into_iter() - .next() - .map(|commit| commit.commit.author.date); - - assert_eq!( - date.map(|value| value.to_rfc3339()), - Some("2026-08-01T12:34:56+00:00".into()) - ); - Ok(()) - } - #[test] fn test_slugify() { assert_eq!(slugify("this is a test"), "this-is-a-test".to_string()); diff --git a/ci/render/src/lints.rs b/ci/render/src/lints.rs index d5e25b47b6..08376c29c4 100644 --- a/ci/render/src/lints.rs +++ b/ci/render/src/lints.rs @@ -1,27 +1,27 @@ -use anyhow::{Result, anyhow}; +use anyhow::{Result, ensure}; -use crate::types::ParsedEntry; -use crate::types::Tag; +use crate::types::{ParsedEntry, Tag}; + +pub fn validate(entry: &ParsedEntry, tags: &[Tag]) -> Result<()> { + name(entry, tags)?; + min_one_tag(entry, tags) +} pub fn name(entry: &ParsedEntry, _: &[Tag]) -> Result<()> { - if entry.name.len() <= 50 { - Ok(()) - } else { - Err(anyhow!( - "Name of entry may be at most 50 characters long, but {} is {} long", - entry.name, - entry.name.len() - )) - } + ensure!( + entry.name.len() <= 50, + "Name of entry may be at most 50 characters long, but {} is {} long", + entry.name, + entry.name.len() + ); + Ok(()) } pub fn min_one_tag(entry: &ParsedEntry, _: &[Tag]) -> Result<()> { - if entry.tags.is_empty() { - Err(anyhow!( - "{} must have at least one tag from `tags.yml`.", - entry.name - )) - } else { - Ok(()) - } + ensure!( + !entry.tags.is_empty(), + "{} must have at least one tag from `tags.yml`.", + entry.name + ); + Ok(()) } diff --git a/ci/render/src/regression_tests.rs b/ci/render/src/regression_tests.rs new file mode 100644 index 0000000000..8dd6209913 --- /dev/null +++ b/ci/render/src/regression_tests.rs @@ -0,0 +1,287 @@ +use anyhow::{Context, Result}; +use serde_json::json; +use std::collections::BTreeMap; + +use crate::types::{Catalog, Entry, ParsedEntry, Tag, ToolType, Type}; +use crate::{create_api, create_catalog, format_stats, stats}; + +fn tag(value: &str, kind: Type) -> Tag { + Tag { + name: value.into(), + value: value.into(), + kind, + include_multi: false, + } +} + +fn parsed() -> Result { + Ok(serde_json::from_value(json!({ + "name": "Example Tool", + "categories": ["linter"], + "tags": ["rust"], + "license": "MIT / Apache License", + "types": ["cli"], + "homepage": "https://example.com", + "description": "Example description" + }))?) +} + +#[test] +fn normalization_preserves_fields_and_uses_the_first_matching_tag() -> Result<()> { + let original = parsed()?; + let rust = tag("rust", Type::Language); + let mut duplicate = rust.clone(); + duplicate.name = "Different metadata".into(); + let normalized = Entry::from_parsed(original.clone(), &[rust.clone(), duplicate])?; + assert_eq!(normalized.tags, [rust].into()); + assert_eq!(normalized.types, [ToolType::Commandline].into()); + let mut expected = serde_json::to_value(original)?; + expected["tags"] = serde_json::to_value(&normalized.tags)?; + assert_eq!(serde_json::to_value(normalized)?, expected); + Ok(()) +} + +#[test] +fn unknown_tags_are_reported_together_before_invalid_tool_types() -> Result<()> { + let mut tool = parsed()?; + tool.tags = ["z-unknown".into(), "a-unknown".into(), "rust".into()].into(); + tool.types = ["invalid".into()].into(); + let error = Entry::from_parsed(tool, &[tag("rust", Type::Language)]) + .err() + .context("Unknown tags should be rejected")?; + assert_eq!( + error.to_string(), + "Tool 'Example Tool': Invalid tag: a-unknown\nInvalid tag: z-unknown\n File: data/tools/example-tool.yml" + ); + Ok(()) +} + +#[test] +fn tool_type_deserialization_matches_the_previous_json_conversion() -> Result<()> { + let tags = [tag("rust", Type::Language)]; + for value in ["cli", "gui", "service", "ide-plugin", "", "CLI", "unknown"] { + let mut tool = parsed()?; + tool.types = [value.into()].into(); + let previous = serde_json::from_value::(serde_json::to_value(value)?); + let current = Entry::from_parsed(tool, &tags).map(|entry| entry.types); + match previous { + Ok(kind) => assert_eq!(current?, [kind].into()), + Err(error) => assert_eq!( + current + .err() + .context("Invalid type should be rejected")? + .to_string(), + error.to_string() + ), + } + } + Ok(()) +} + +#[test] +fn validation_preserves_byte_length_limit_and_error_precedence() -> Result<()> { + let tags = [tag("rust", Type::Language)]; + for name in ["a".repeat(50), "é".repeat(25), String::new()] { + let mut tool = parsed()?; + tool.name = name; + Entry::from_parsed(tool, &tags)?; + } + let mut tool = parsed()?; + tool.name = "é".repeat(26); + tool.tags.clear(); + assert_eq!( + Entry::from_parsed(tool.clone(), &tags) + .err() + .context("Names over 50 bytes should be rejected")? + .to_string(), + format!( + "Name of entry may be at most 50 characters long, but {} is 52 long", + tool.name + ) + ); + tool.name = "Example Tool".into(); + assert_eq!( + Entry::from_parsed(tool, &tags) + .err() + .context("Empty tags should be rejected")? + .to_string(), + "Example Tool must have at least one tag from `tags.yml`." + ); + Ok(()) +} + +#[test] +fn api_preserves_configured_tag_order_duplicates_and_all_fields() -> Result<()> { + let rust = tag("rust", Type::Language); + let python = tag("python", Type::Language); + let security = tag("security", Type::Other); + let mut raw = parsed()?; + raw.tags = ["rust".into(), "python".into(), "security".into()].into(); + raw.source = Some("https://github.com/owner/repo".into()); + raw.pricing = Some("https://example.com/pricing".into()); + raw.plans = Some(BTreeMap::from([("free".into(), true)])); + raw.discussion = Some("https://example.com/discussion".into()); + raw.deprecated = Some(false); + raw.resources = Some(serde_json::from_value( + json!([{"title": "Docs", "url": "https://example.com/docs"}]), + )?); + raw.reviews = Some(["https://example.com/review".into()].into()); + raw.demos = Some(["https://example.com/demo".into()].into()); + raw.wrapper = Some(true); + let tool = Entry::from_parsed(raw, &[rust.clone(), python.clone(), security.clone()])?; + let mut different_metadata = rust.clone(); + different_metadata.name = "Not the same tag".into(); + let api = create_api( + vec![tool], + &[rust.clone(), python, rust, different_metadata], + &[security], + ); + assert_eq!( + serde_json::to_value(api)?, + json!({ + "example-tool": { + "name": "Example Tool", "categories": ["linter"], + "languages": ["rust", "python", "rust"], "other": ["security"], + "licenses": ["MIT / Apache License"], "types": ["cli"], + "homepage": "https://example.com", "description": "Example description", + "source": "https://github.com/owner/repo", "pricing": "https://example.com/pricing", + "plans": {"free": true}, "discussion": "https://example.com/discussion", + "deprecated": false, "resources": [{"title": "Docs", "url": "https://example.com/docs"}], + "reviews": ["https://example.com/review"], "demos": ["https://example.com/demo"], + "wrapper": true + } + }) + ); + Ok(()) +} + +#[test] +fn api_slug_collisions_keep_the_last_entry_and_missing_fields_stay_null() -> Result<()> { + let tool = Entry::from_parsed(parsed()?, &[tag("rust", Type::Language)])?; + let mut replacement = tool.clone(); + replacement.name = "Example-Tool".into(); + let api = create_api(vec![tool, replacement], &[], &[]); + assert_eq!(api.len(), 1); + assert_eq!(api["example-tool"].name, "Example-Tool"); + let encoded = serde_json::to_value(api)?; + for field in [ + "source", + "pricing", + "plans", + "discussion", + "deprecated", + "resources", + "reviews", + "demos", + "wrapper", + ] { + assert_eq!( + encoded["example-tool"].get(field), + Some(&serde_json::Value::Null) + ); + } + Ok(()) +} + +#[test] +fn catalog_preserves_input_order_and_omits_empty_sections() -> Result<()> { + let rust = tag("rust", Type::Language); + let python = tag("python", Type::Language); + let regular = tag("regular", Type::Other); + let mut inclusive = tag("inclusive", Type::Other); + inclusive.include_multi = true; + let tags = [ + rust.clone(), + python.clone(), + regular.clone(), + inclusive.clone(), + ]; + let mut raw = parsed()?; + raw.tags = ["rust".into(), "regular".into(), "inclusive".into()].into(); + raw.name = "Z Single".into(); + let single = Entry::from_parsed(raw.clone(), &tags)?; + raw.tags.insert("python".into()); + raw.name = "A Multi".into(); + let multi = Entry::from_parsed(raw, &tags)?; + let tools = [single.clone(), multi.clone()]; + let catalog = create_catalog( + &tools, + &[python, rust.clone()], + &[ + regular.clone(), + inclusive.clone(), + tag("absent", Type::Other), + ], + vec![], + ); + assert_eq!(catalog.multi, [multi]); + assert_eq!(catalog.linters.len(), 1); + assert_eq!(catalog.linters[&rust], std::slice::from_ref(&single)); + assert_eq!(catalog.others.len(), 2); + assert_eq!(catalog.others[®ular], [single]); + assert_eq!(catalog.others[&inclusive], tools); + Ok(()) +} + +#[test] +fn catalog_rows_keep_column_major_layout_for_partial_rows() { + for (count, expected) in [ + (0, vec![]), + (1, vec![vec!["0"]]), + (2, vec![vec!["0", "1"]]), + (3, vec![vec!["0", "1", "2"]]), + (4, vec![vec!["0", "2"], vec!["1", "3"]]), + (5, vec![vec!["0", "2", "4"], vec!["1", "3"]]), + (6, vec![vec!["0", "2", "4"], vec!["1", "3", "5"]]), + (7, vec![vec!["0", "3", "6"], vec!["1", "4"], vec!["2", "5"]]), + ] { + let map: BTreeMap<_, _> = (0..count) + .map(|i| (tag(&i.to_string(), Type::Language), vec![])) + .collect(); + let catalog = Catalog { + linters: map.clone(), + others: map, + multi: vec![], + collections: vec![], + }; + for rows in [catalog.linter_rows(), catalog.other_rows()] { + let names: Vec> = rows + .into_iter() + .map(|row| row.into_iter().map(|(tag, _)| tag.value.as_str()).collect()) + .collect(); + assert_eq!(names, expected); + } + } +} + +#[test] +fn stats_keep_last_duplicate_and_strip_repeated_leading_prefixes_only() { + let raw = stats::StatsRaw { + data: stats::Data { + result: [ + ("/tool/example", "1"), + ("/tool/example", "2"), + ("/tool//tool/repeated", "3"), + ("/other/tool/path", "4"), + ("/tool/", "5"), + ] + .into_iter() + .map(|(path, value)| stats::Result { + metric: stats::Metric { path: path.into() }, + value: (0.0, value.into()), + }) + .collect(), + ..Default::default() + }, + ..Default::default() + }; + assert_eq!( + format_stats(raw), + BTreeMap::from([ + ("example".into(), "2".into()), + ("repeated".into(), "3".into()), + ("/other/tool/path".into(), "4".into()), + (String::new(), "5".into()), + ]) + ); +} diff --git a/ci/render/src/types.rs b/ci/render/src/types.rs index bec2423b7d..acbef40cf8 100644 --- a/ci/render/src/types.rs +++ b/ci/render/src/types.rs @@ -1,10 +1,10 @@ use anyhow::{Result, bail}; use askama::Template; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Serialize, de::value::StrDeserializer}; use std::cmp::Ordering; use std::collections::{BTreeMap, BTreeSet}; -use crate::valid; +use crate::lints; #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash, Ord, PartialOrd)] pub enum Type { @@ -142,13 +142,17 @@ impl Entry { /// Returns an error when the entry fails validation or references an /// unknown tag or tool type. pub fn from_parsed(p: ParsedEntry, tags: &[Tag]) -> Result { - valid(&p, tags)?; - - let tag_results: Vec> = p.tags.iter().map(|t| get_tag(t, tags)).collect(); - let tag_errors: Vec = tag_results - .iter() - .filter_map(|r| r.as_ref().err().map(ToString::to_string)) - .collect(); + lints::validate(&p, tags)?; + + let mut entry_tags = BTreeSet::new(); + let mut tag_errors = Vec::new(); + for value in &p.tags { + if let Some(tag) = tags.iter().find(|tag| tag.value == *value) { + entry_tags.insert(tag.clone()); + } else { + tag_errors.push(format!("Invalid tag: {value}")); + } + } if !tag_errors.is_empty() { bail!( "Tool '{}': {}\n File: data/tools/{}.yml", @@ -157,23 +161,18 @@ impl Entry { p.name.to_lowercase().replace(' ', "-") ); } - let entry_tags: Result> = tag_results.into_iter().collect(); - - let types: Result> = p + let types = p .types .iter() - .map(|t| { - let value = serde_json::to_value(t)?; - serde_json::from_value::(value).map_err(Into::into) - }) - .collect(); + .map(|value| ToolType::deserialize(StrDeserializer::::new(value))) + .collect::>()?; Ok(Self { name: p.name, categories: p.categories, - tags: entry_tags?, + tags: entry_tags, license: p.license, - types: types?, + types, homepage: p.homepage, source: p.source, pricing: p.pricing, @@ -189,15 +188,6 @@ impl Entry { } } -fn get_tag(t: &str, tags: &[Tag]) -> Result { - for tag in tags { - if tag.value == t { - return Ok(tag.clone()); - } - } - bail!("Invalid tag: {t}") -} - impl PartialOrd for Entry { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -234,16 +224,16 @@ impl Catalog { /// Arranges a tag map into three visually balanced table columns. fn rows(map: &EntryMap) -> Vec)>> { let num_columns = 3; - let mut rows = Vec::new(); let items: Vec<_> = map.iter().collect(); let items_per_column = items.len().div_ceil(num_columns); + let mut rows = Vec::with_capacity(items_per_column); for i in 0..items_per_column { - let mut row = Vec::new(); + let mut row = Vec::with_capacity(num_columns); for col in 0..num_columns { let index = col * items_per_column + i; - if index < items.len() { - row.push(items[index]); + if let Some(&item) = items.get(index) { + row.push(item); } } rows.push(row); diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 4ecc04c729..b0cd3b63c5 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,4 +1,4 @@ [toolchain] -channel = "1.98.0" +channel = "1.98.1" components = ["clippy", "rustfmt"] profile = "minimal" From 96b4d2c2814a293c748eb9acd4366f2f62ce51bb Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 17:55:12 +0200 Subject: [PATCH 02/13] Use Clap for renderer and checker command-line arguments --- ci/Cargo.lock | 129 +++++++++++++++++++++++++++++++++++--- ci/Cargo.toml | 2 +- ci/pr-check/Cargo.toml | 2 +- ci/pr-check/src/main.rs | 52 ++++++++++++--- ci/pr-check/tests/cli.rs | 25 ++++++++ ci/render/Cargo.toml | 2 +- ci/render/src/bin/main.rs | 51 +++++++-------- 7 files changed, 214 insertions(+), 49 deletions(-) create mode 100644 ci/pr-check/tests/cli.rs diff --git a/ci/Cargo.lock b/ci/Cargo.lock index 7ec916a5e0..a3a12e0c27 100644 --- a/ci/Cargo.lock +++ b/ci/Cargo.lock @@ -22,12 +22,56 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "anstream" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "824a212faf96e9acacdbd09febd34438f8f711fb84e09a8916013cd7815ca28d" +dependencies = [ + "anstyle", + "anstyle-parse", + "anstyle-query", + "anstyle-wincon", + "colorchoice", + "is_terminal_polyfill", + "utf8parse", +] + [[package]] name = "anstyle" version = "1.0.14" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "940b3a0ca603d1eade50a4846a2afffd5ef57a9feac2c0e2ec2e14f9ead76000" +[[package]] +name = "anstyle-parse" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52ce7f38b242319f7cabaa6813055467063ecdc9d355bbb4ce0c68908cd8130e" +dependencies = [ + "utf8parse", +] + +[[package]] +name = "anstyle-query" +version = "1.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc" +dependencies = [ + "windows-sys 0.61.2", +] + +[[package]] +name = "anstyle-wincon" +version = "3.0.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d" +dependencies = [ + "anstyle", + "once_cell_polyfill", + "windows-sys 0.61.2", +] + [[package]] name = "anyhow" version = "1.0.104" @@ -216,6 +260,46 @@ dependencies = [ "windows-link", ] +[[package]] +name = "clap" +version = "4.6.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aa8876b300ab35ba921adea3dfd70157a46249b33f95c9084ae5709785478946" +dependencies = [ + "clap_builder", + "clap_derive", +] + +[[package]] +name = "clap_builder" +version = "4.6.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec0797fb7aeb1406c84efac526901f7ec3ead2124f946b494e72879d4b54704d" +dependencies = [ + "anstream", + "anstyle", + "clap_lex", + "strsim", +] + +[[package]] +name = "clap_derive" +version = "4.6.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f9c751b79415d4e559e3d1fcf128e09e720eb673a06d26cf6f392d37d75b66e0" +dependencies = [ + "heck", + "proc-macro2", + "quote", + "syn 3.0.6", +] + +[[package]] +name = "clap_lex" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1c133bc6a41be0d194c306b5506d15e6feeea7b1d6604bd3f8310dfb2ca96486" + [[package]] name = "cmake" version = "0.1.58" @@ -225,6 +309,12 @@ dependencies = [ "cc", ] +[[package]] +name = "colorchoice" +version = "1.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d07550c9036bf2ae0c684c4297d503f838287c83c53686d05370d0e139ae570" + [[package]] name = "combine" version = "4.6.8" @@ -420,6 +510,12 @@ dependencies = [ "smallvec", ] +[[package]] +name = "heck" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" + [[package]] name = "http" version = "1.5.0" @@ -653,6 +749,12 @@ version = "2.12.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "791930b43c0d5973160d90a8f3894509f2b273430f5c5c73b668636d0287c5c0" +[[package]] +name = "is_terminal_polyfill" +version = "1.70.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a6cb138bb79a146c1bd460005623e142ef0181e3d0219cb493e02f7d08a35695" + [[package]] name = "itoa" version = "1.0.18" @@ -812,6 +914,12 @@ version = "1.21.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" +[[package]] +name = "once_cell_polyfill" +version = "1.70.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" + [[package]] name = "openssl-probe" version = "0.2.1" @@ -824,12 +932,6 @@ version = "2.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" -[[package]] -name = "pico-args" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5be167a7af36ee22fe3115051bc51f6e6c7054c9348e28deb4f49bd6f705a315" - [[package]] name = "pin-project-lite" version = "0.2.17" @@ -858,6 +960,7 @@ dependencies = [ "anyhow", "askama", "chrono", + "clap", "reqwest", "serde", "serde-saphyr", @@ -978,7 +1081,7 @@ dependencies = [ "anyhow", "askama", "chrono", - "pico-args", + "clap", "reqwest", "serde", "serde-saphyr", @@ -1304,6 +1407,12 @@ version = "1.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" +[[package]] +name = "strsim" +version = "0.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" + [[package]] name = "subtle" version = "2.6.1" @@ -1551,6 +1660,12 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6c140620e7ffbb22c2dee59cafe6084a59b5ffc27a8859a5f0d494b5d52b6be" +[[package]] +name = "utf8parse" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" + [[package]] name = "walkdir" version = "2.5.0" diff --git a/ci/Cargo.toml b/ci/Cargo.toml index 72452219f3..8a63af670f 100644 --- a/ci/Cargo.toml +++ b/ci/Cargo.toml @@ -13,7 +13,7 @@ publish = false anyhow = "1.0.104" askama = "0.16" chrono = { version = "0.4.45", features = ["serde"] } -pico-args = "0.5" +clap = { version = "4.6.7", features = ["derive"] } reqwest = { version = "0.13.5", default-features = false, features = ["json", "rustls", "system-proxy"] } serde = { version = "1.0.229", features = ["derive"] } serde_json = "1.0.151" diff --git a/ci/pr-check/Cargo.toml b/ci/pr-check/Cargo.toml index e1c916a1ae..9b92c1c8ab 100644 --- a/ci/pr-check/Cargo.toml +++ b/ci/pr-check/Cargo.toml @@ -15,7 +15,7 @@ workspace = true anyhow.workspace = true askama.workspace = true chrono.workspace = true - +clap.workspace = true reqwest.workspace = true serde.workspace = true serde-saphyr.workspace = true diff --git a/ci/pr-check/src/main.rs b/ci/pr-check/src/main.rs index fedd79ba6f..71ec6d89e7 100644 --- a/ci/pr-check/src/main.rs +++ b/ci/pr-check/src/main.rs @@ -26,6 +26,7 @@ mod network; mod report; use anyhow::{Context, Result}; +use clap::Parser; use std::env; use std::path::{Path, PathBuf}; use std::process::ExitCode; @@ -34,6 +35,14 @@ use criteria::ToolEntry; use network::{GithubClient, check_tool}; use report::{render_comment, report_exit_code}; +#[derive(Debug, Parser)] +#[command(version, about)] +struct Args { + /// Changed files to check; only YAML files under data/tools are inspected. + #[arg(value_name = "FILE")] + files: Vec, +} + /// Reads and deserialises a single tool YAML file. /// /// # Errors @@ -54,20 +63,21 @@ fn is_tool_path(path: &Path) -> bool { #[tokio::main(flavor = "current_thread")] async fn main() -> Result { + let args = match Args::try_parse() { + Ok(args) => args, + Err(error) => { + // Exit code 2 tells the workflow to close the PR, so CLI errors must use 1. + let code = u8::from(error.use_stderr()); + error.print()?; + return Ok(ExitCode::from(code)); + } + }; let token = env::var("GITHUB_TOKEN").context("GITHUB_TOKEN not set")?; - // Remaining CLI arguments are the paths to check. - // Usage: pr-check data/tools/foo.yml data/tools/bar.yml - let tool_paths: Vec = env::args_os() - .skip(1) - .map(PathBuf::from) - .filter(|path| is_tool_path(path)) - .collect(); - let client = GithubClient::new(token)?; let mut reports = Vec::new(); - for path in &tool_paths { + for path in args.files.iter().filter(|path| is_tool_path(path)) { let tool = read_tool(path).with_context(|| format!("Failed to read {}", path.display()))?; eprintln!("Checking '{}'...", tool.name); let report = check_tool(&client, &tool).await?; @@ -105,6 +115,30 @@ async fn main() -> Result { mod tests { use super::*; + #[test] + fn parses_files_and_provides_help() -> Result<()> { + let args = Args::try_parse_from(["pr-check", "data/tools/example.yml", "README.md"])?; + assert_eq!( + args.files, + [ + PathBuf::from("data/tools/example.yml"), + PathBuf::from("README.md") + ] + ); + assert!(Args::try_parse_from(["pr-check"])?.files.is_empty()); + for (flag, kind) in [ + ("--help", clap::error::ErrorKind::DisplayHelp), + ("--version", clap::error::ErrorKind::DisplayVersion), + ("--unknown", clap::error::ErrorKind::UnknownArgument), + ] { + let error = Args::try_parse_from(["pr-check", flag]) + .err() + .context("Expected help or error")?; + assert_eq!(error.kind(), kind); + } + Ok(()) + } + #[test] fn parses_catalog() -> Result<()> { let tools = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../data/tools"); diff --git a/ci/pr-check/tests/cli.rs b/ci/pr-check/tests/cli.rs new file mode 100644 index 0000000000..5c3704b555 --- /dev/null +++ b/ci/pr-check/tests/cli.rs @@ -0,0 +1,25 @@ +use std::process::Command; + +#[test] +fn help_and_version_do_not_require_credentials() -> std::io::Result<()> { + for (flag, expected) in [("--help", "Usage:"), ("--version", "pr-check")] { + let output = Command::new(env!("CARGO_BIN_EXE_pr-check")) + .arg(flag) + .env_remove("GITHUB_TOKEN") + .output()?; + assert!(output.status.success()); + assert!(String::from_utf8_lossy(&output.stdout).contains(expected)); + } + Ok(()) +} + +#[test] +fn invalid_arguments_never_use_the_pr_closure_exit_code() -> std::io::Result<()> { + let output = Command::new(env!("CARGO_BIN_EXE_pr-check")) + .arg("--unknown") + .env_remove("GITHUB_TOKEN") + .output()?; + assert_eq!(output.status.code(), Some(1)); + assert!(String::from_utf8_lossy(&output.stderr).contains("--unknown")); + Ok(()) +} diff --git a/ci/render/Cargo.toml b/ci/render/Cargo.toml index 96575df036..cabcfe2a66 100644 --- a/ci/render/Cargo.toml +++ b/ci/render/Cargo.toml @@ -18,7 +18,7 @@ workspace = true anyhow.workspace = true askama.workspace = true chrono.workspace = true -pico-args.workspace = true +clap.workspace = true reqwest.workspace = true serde.workspace = true serde_json.workspace = true diff --git a/ci/render/src/bin/main.rs b/ci/render/src/bin/main.rs index b4c30e31ae..bde31c038c 100644 --- a/ci/render/src/bin/main.rs +++ b/ci/render/src/bin/main.rs @@ -1,49 +1,39 @@ -use anyhow::{Context, Result, ensure}; +use anyhow::{Context, Result}; use askama::Template; -use pico_args::Arguments; +use clap::Parser; use render::types::{Collection, Entry, ParsedEntry, Tag, Tags, Type}; use render::{check_deprecated, create_api, create_catalog}; use serde::{Deserialize, Serialize, de::DeserializeOwned}; use slug::slugify; use std::collections::BTreeMap; use std::env; -use std::ffi::OsStr; use std::fs; use std::io; use std::path::{Path, PathBuf}; -#[derive(Debug)] +#[derive(Debug, Parser)] +#[command(name = "render", version, about)] struct Args { + /// YAML file defining the available tags. + #[arg(long)] tags: PathBuf, + /// Directory containing tool YAML files. + #[arg(long)] tools: PathBuf, + /// Directory containing related collection YAML files. + #[arg(long)] collections: PathBuf, + /// Destination for the generated README. + #[arg(long)] md_out: PathBuf, + /// Existing directory for the generated JSON API files. + #[arg(long)] json_out: PathBuf, + /// Reuse cached deprecation data instead of querying GitHub. + #[arg(long)] skip_deprecated: bool, } -impl Args { - fn parse(mut args: Arguments) -> Result { - let parsed = Self { - tags: args.value_from_os_str("--tags", parse_path)?, - tools: args.value_from_os_str("--tools", parse_path)?, - collections: args.value_from_os_str("--collections", parse_path)?, - md_out: args.value_from_os_str("--md-out", parse_path)?, - json_out: args.value_from_os_str("--json-out", parse_path)?, - skip_deprecated: args.contains("--skip-deprecated"), - }; - let remaining = args.finish(); - ensure!(remaining.is_empty(), "Unexpected arguments: {remaining:?}"); - Ok(parsed) - } -} - -// `pico_args::value_from_os_str` requires a fallible parser callback. -#[allow(clippy::unnecessary_wraps)] -fn parse_path(s: &OsStr) -> Result { - Ok(s.into()) -} - fn read_yaml(path: &Path) -> Result { let file = fs::File::open(path).with_context(|| format!("Cannot open {}", path.display()))?; serde_saphyr::from_reader(file).with_context(|| format!("Cannot parse {}", path.display())) @@ -99,7 +89,7 @@ fn write_json(path: &Path, value: &impl Serialize) -> Result<()> { #[tokio::main(flavor = "current_thread")] async fn main() -> Result<()> { - let args = Args::parse(Arguments::from_env())?; + let args = Args::parse(); let tags: Tags = read_yaml(&args.tags)?; let mut collections: Vec = read_entries(&args.collections)?; @@ -150,6 +140,7 @@ mod tests { fn cli_args() -> Vec { [ + "render", "--tags", "tags.yml", "--tools", @@ -170,17 +161,17 @@ mod tests { fn parses_cli_and_rejects_unknown_or_missing_arguments() -> Result<()> { let mut args = cli_args(); args.push("--skip-deprecated".into()); - let parsed = Args::parse(Arguments::from_vec(args))?; + let parsed = Args::try_parse_from(args)?; assert!(parsed.skip_deprecated); assert_eq!(parsed.collections, Path::new("collections")); let mut args = cli_args(); args.push("--skip-deprected".into()); - let error = Args::parse(Arguments::from_vec(args)) + let error = Args::try_parse_from(args) .err() .context("Expected invalid argument")?; assert!(error.to_string().contains("--skip-deprected")); - assert!(Args::parse(Arguments::from_vec(vec![])).is_err()); + assert!(Args::try_parse_from(["render"]).is_err()); Ok(()) } From 650c5b080df4851755c943e43dc84982a197be9e Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 17:59:29 +0200 Subject: [PATCH 03/13] Group workspace crates and model GitHub repositories as a type --- .github/workflows/ci.yml | 2 +- .github/workflows/pr-check.yml | 2 +- .gitignore | 2 +- AGENTS.md | 2 +- CONTRIBUTING.md | 2 +- ci/Cargo.toml | 2 +- ci/{ => crates}/pr-check/Cargo.toml | 0 ci/{ => crates}/pr-check/src/criteria.rs | 89 +++++++++++++------ ci/{ => crates}/pr-check/src/main.rs | 2 +- ci/{ => crates}/pr-check/src/network.rs | 21 ++--- ci/{ => crates}/pr-check/src/report.rs | 0 ci/{ => crates}/pr-check/templates/comment.md | 0 ci/{ => crates}/pr-check/tests/cli.rs | 0 ci/{ => crates}/render/.gitignore | 0 ci/{ => crates}/render/Cargo.toml | 0 ci/{ => crates}/render/clippy.toml | 0 ci/{ => crates}/render/src/bin/main.rs | 2 +- ci/{ => crates}/render/src/deprecation.rs | 0 ci/{ => crates}/render/src/lib.rs | 0 ci/{ => crates}/render/src/lints.rs | 0 .../render/src/regression_tests.rs | 0 ci/{ => crates}/render/src/stats.rs | 0 ci/{ => crates}/render/src/types.rs | 0 ci/{ => crates}/render/templates/README.md | 0 24 files changed, 80 insertions(+), 46 deletions(-) rename ci/{ => crates}/pr-check/Cargo.toml (100%) rename ci/{ => crates}/pr-check/src/criteria.rs (87%) rename ci/{ => crates}/pr-check/src/main.rs (99%) rename ci/{ => crates}/pr-check/src/network.rs (91%) rename ci/{ => crates}/pr-check/src/report.rs (100%) rename ci/{ => crates}/pr-check/templates/comment.md (100%) rename ci/{ => crates}/pr-check/tests/cli.rs (100%) rename ci/{ => crates}/render/.gitignore (100%) rename ci/{ => crates}/render/Cargo.toml (100%) rename ci/{ => crates}/render/clippy.toml (100%) rename ci/{ => crates}/render/src/bin/main.rs (99%) rename ci/{ => crates}/render/src/deprecation.rs (100%) rename ci/{ => crates}/render/src/lib.rs (100%) rename ci/{ => crates}/render/src/lints.rs (100%) rename ci/{ => crates}/render/src/regression_tests.rs (100%) rename ci/{ => crates}/render/src/stats.rs (100%) rename ci/{ => crates}/render/src/types.rs (100%) rename ci/{ => crates}/render/templates/README.md (100%) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0c5e57b6a6..20209345c0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,7 +38,7 @@ jobs: REPO: ${{ github.repository }} run: | gh api "repos/$REPO/issues/$PR/comments" \ - -f body="README.md was edited directly. Please edit tool entries in \`data/tools/\`, related collections in \`data/collections/\`, or text and structure in \`ci/render/templates/README.md\` instead. Leave the generated README.md out of your pull request." \ + -f body="README.md was edited directly. Please edit tool entries in \`data/tools/\`, related collections in \`data/collections/\`, or text and structure in \`ci/crates/render/templates/README.md\` instead. Leave the generated README.md out of your pull request." \ --silent echo "README.md must not be edited directly." >&2 exit 1 diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index 3bed4cb94f..0083db4576 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -114,7 +114,7 @@ jobs: ## [FAIL] Generated README changed - `README.md` is generated and should not be included in tool submissions. Please remove its changes from this PR and submit tool entries under `data/tools/` instead. For changes to the README text or structure, edit `ci/render/templates/README.md` rather than the generated file. + `README.md` is generated and should not be included in tool submissions. Please remove its changes from this PR and submit tool entries under `data/tools/` instead. For changes to the README text or structure, edit `ci/crates/render/templates/README.md` rather than the generated file. This check will remain failed until the `README.md` changes are removed. README changes alone do not automatically close the PR. EOF diff --git a/.gitignore b/.gitignore index c11bed6903..df1b2ee035 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ logcli-linux-amd64 logcli.zip ci/target/ -ci/pr-check/target/ \ No newline at end of file +ci/crates/*/target/ \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 80fbfd70a6..4e29ba7364 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,6 +9,6 @@ Your goal is to help the user submit a high-quality pull request that aligns wit When the user asks you to add a new static analysis tool, please act as a helpful code reviewer: 1. **Verify the criteria:** Check the requirements in `CONTRIBUTING.md`: at least 20 GitHub stars, at least six months of history, and more than one human contributor. 2. **Wait until the tool qualifies:** If any criterion is not met, do not submit a pull request yet. Explain that the bot closes PRs with verified criteria failures and encourage the user to submit once all requirements are met. If a criterion cannot be verified automatically, provide evidence for manual review rather than claiming it passed. -3. **Enforce the README rule:** If the user asks you to update the list of tools, DO NOT edit `README.md`. Explain to the user that the list of tools in `README.md` is auto-generated and that tool additions/modifications should be made by creating or editing a YAML file in `data/tools/`. For changes to the README text or structure, edit `ci/render/templates/README.md`. Do not include generated `README.md` changes in a pull request; CI flags them as a failure. +3. **Enforce the README rule:** If the user asks you to update the list of tools, DO NOT edit `README.md`. Explain to the user that the list of tools in `README.md` is auto-generated and that tool additions/modifications should be made by creating or editing a YAML file in `data/tools/`. For changes to the README text or structure, edit `ci/crates/render/templates/README.md`. Do not include generated `README.md` changes in a pull request; CI flags them as a failure. Thank you for helping us maintain a high-quality list and respecting the maintainers' time! diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fa8c485f07..fb557a8972 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ provide evidence of that history. Missing registration data also requires review manually.** Leave generated `README.md` changes out of your pull request, even if you run `make render` locally. CI will flag them as a failure in the PR comment. For changes to the README text or structure, edit -`ci/render/templates/README.md` instead. +`ci/crates/render/templates/README.md` instead. To add a new tool, please create a file in the `data/tools` directory like `data/tools/.yml`. Feel free to check out a few other YAML files in diff --git a/ci/Cargo.toml b/ci/Cargo.toml index 8a63af670f..1748312e35 100644 --- a/ci/Cargo.toml +++ b/ci/Cargo.toml @@ -1,5 +1,5 @@ [workspace] -members = ["render", "pr-check"] +members = ["crates/*"] resolver = "3" [workspace.package] diff --git a/ci/pr-check/Cargo.toml b/ci/crates/pr-check/Cargo.toml similarity index 100% rename from ci/pr-check/Cargo.toml rename to ci/crates/pr-check/Cargo.toml diff --git a/ci/pr-check/src/criteria.rs b/ci/crates/pr-check/src/criteria.rs similarity index 87% rename from ci/pr-check/src/criteria.rs rename to ci/crates/pr-check/src/criteria.rs index ebc442ca35..a598fb1fc4 100644 --- a/ci/pr-check/src/criteria.rs +++ b/ci/crates/pr-check/src/criteria.rs @@ -1,6 +1,6 @@ //! Contribution criteria and URL classification, independent of network access. -use anyhow::{Context, Result}; +use anyhow::{Context, Result, ensure}; use chrono::{DateTime, Months, Utc}; use serde::Deserialize; @@ -47,20 +47,35 @@ const MIN_STARS: u64 = 20; const MIN_CONTRIBUTORS: usize = 2; const MIN_AGE_MONTHS: u32 = 6; -/// Parses `owner` and `repo` out of a GitHub URL like -/// `https://github.com/owner/repo` or `https://github.com/owner/repo/`. -/// Returns `None` for non-GitHub URLs or malformed paths. -pub fn parse_github_repo(url: &str) -> Option<(&str, &str)> { - let url = url.trim_end_matches('/'); - let without_scheme = url - .strip_prefix("https://github.com/") - .or_else(|| url.strip_prefix("http://github.com/"))?; - - let (owner, repo) = without_scheme.split_once('/')?; - if owner.is_empty() || repo.is_empty() || repo.contains('/') { - return None; +/// A repository parsed from a GitHub HTTP(S) URL, borrowing its owner and name. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct GithubRepo<'a> { + owner: &'a str, + name: &'a str, +} + +impl<'a> TryFrom<&'a str> for GithubRepo<'a> { + type Error = anyhow::Error; + + fn try_from(url: &'a str) -> Result { + let url = url.trim_end_matches('/'); + let path = url + .strip_prefix("https://github.com/") + .or_else(|| url.strip_prefix("http://github.com/")) + .context("Expected a GitHub HTTP(S) URL")?; + let (owner, name) = path.split_once('/').context("Expected owner/repository")?; + ensure!( + !owner.is_empty() && !name.is_empty() && !name.contains('/'), + "Expected a repository URL with no subpath" + ); + Ok(Self { owner, name }) + } +} + +impl std::fmt::Display for GithubRepo<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}/{}", self.owner, self.name) } - Some((owner, repo)) } /// Evaluate fetched metadata without performing I/O; unavailable checks require review. @@ -226,33 +241,45 @@ mod tests { } #[test] - fn parses_plain_github_url() { - let result = parse_github_repo("https://github.com/owner/repo"); - assert_eq!(result, Some(("owner", "repo"))); + fn parses_plain_github_url() -> Result<()> { + let repo = GithubRepo::try_from("https://github.com/owner/repo")?; + assert_eq!( + repo, + GithubRepo { + owner: "owner", + name: "repo" + } + ); + assert_eq!(repo.to_string(), "owner/repo"); + Ok(()) } #[test] - fn parses_trailing_slash() { - let result = parse_github_repo("https://github.com/owner/repo/"); - assert_eq!(result, Some(("owner", "repo"))); + fn parses_trailing_slash() -> Result<()> { + let repo = GithubRepo::try_from("https://github.com/owner/repo/")?; + assert_eq!( + repo, + GithubRepo { + owner: "owner", + name: "repo" + } + ); + Ok(()) } #[test] fn rejects_subpath() { - let result = parse_github_repo("https://github.com/owner/repo/tree/main/subdir"); - assert!(result.is_none()); + assert!(GithubRepo::try_from("https://github.com/owner/repo/tree/main/subdir").is_err()); } #[test] fn rejects_gitlab() { - let result = parse_github_repo("https://gitlab.com/owner/repo"); - assert!(result.is_none()); + assert!(GithubRepo::try_from("https://gitlab.com/owner/repo").is_err()); } #[test] fn rejects_missing_repo() { - let result = parse_github_repo("https://github.com/owner"); - assert!(result.is_none()); + assert!(GithubRepo::try_from("https://github.com/owner").is_err()); } #[test] @@ -455,7 +482,13 @@ mod tests { "http://github.com/owner/repo", "https://github.com/owner/repo///", ] { - assert_eq!(parse_github_repo(url), Some(("owner", "repo"))); + assert_eq!( + GithubRepo::try_from(url).ok(), + Some(GithubRepo { + owner: "owner", + name: "repo" + }) + ); } for url in [ "https://github.com/", @@ -465,7 +498,7 @@ mod tests { "https://github.com.evil/owner/repo", "git@github.com:owner/repo.git", ] { - assert_eq!(parse_github_repo(url), None, "{url}"); + assert!(GithubRepo::try_from(url).is_err(), "{url}"); } } } diff --git a/ci/pr-check/src/main.rs b/ci/crates/pr-check/src/main.rs similarity index 99% rename from ci/pr-check/src/main.rs rename to ci/crates/pr-check/src/main.rs index 71ec6d89e7..9778d93e76 100644 --- a/ci/pr-check/src/main.rs +++ b/ci/crates/pr-check/src/main.rs @@ -141,7 +141,7 @@ mod tests { #[test] fn parses_catalog() -> Result<()> { - let tools = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../data/tools"); + let tools = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../data/tools"); let mut count = 0; for entry in std::fs::read_dir(tools)? { let path = entry?.path(); diff --git a/ci/pr-check/src/network.rs b/ci/crates/pr-check/src/network.rs similarity index 91% rename from ci/pr-check/src/network.rs rename to ci/crates/pr-check/src/network.rs index a1e189033e..15423f918d 100644 --- a/ci/pr-check/src/network.rs +++ b/ci/crates/pr-check/src/network.rs @@ -5,7 +5,7 @@ use chrono::{DateTime, Utc}; use serde::{Deserialize, de::DeserializeOwned}; use crate::criteria::{ - Contributor, RepoInfo, ToolEntry, domain_age_result, homepage_domain, parse_github_repo, + Contributor, GithubRepo, RepoInfo, ToolEntry, domain_age_result, homepage_domain, repository_report, }; use crate::report::{CheckResult, ToolReport}; @@ -67,8 +67,8 @@ impl GithubClient { /// # Errors /// /// Returns an error if the API call fails. - async fn repo_info(&self, owner: &str, repo: &str) -> Result> { - let url = format!("https://api.github.com/repos/{owner}/{repo}"); + async fn repo_info(&self, repo: GithubRepo<'_>) -> Result> { + let url = format!("https://api.github.com/repos/{repo}"); self.get::(&url).await } @@ -77,9 +77,8 @@ impl GithubClient { /// # Errors /// /// Returns an error if the API call fails. - async fn contributor_count(&self, owner: &str, repo: &str) -> Result> { - let url = - format!("https://api.github.com/repos/{owner}/{repo}/contributors?per_page=100&anon=0"); + async fn contributor_count(&self, repo: GithubRepo<'_>) -> Result> { + let url = format!("https://api.github.com/repos/{repo}/contributors?per_page=100&anon=0"); Ok(self .get::>(&url) .await? @@ -96,11 +95,13 @@ impl GithubClient { pub async fn check_tool(client: &GithubClient, tool: &ToolEntry) -> Result { let source = &tool.source; - let gh_coords = source.as_deref().and_then(parse_github_repo); + let repo = source + .as_deref() + .and_then(|url| GithubRepo::try_from(url).ok()); - if let Some((owner, repo)) = gh_coords { - let repo_result = client.repo_info(owner, repo).await; - let contributors_result = client.contributor_count(owner, repo).await; + if let Some(repo) = repo { + let repo_result = client.repo_info(repo).await; + let contributors_result = client.contributor_count(repo).await; repository_report(tool, &repo_result, contributors_result, Utc::now()) } else { diff --git a/ci/pr-check/src/report.rs b/ci/crates/pr-check/src/report.rs similarity index 100% rename from ci/pr-check/src/report.rs rename to ci/crates/pr-check/src/report.rs diff --git a/ci/pr-check/templates/comment.md b/ci/crates/pr-check/templates/comment.md similarity index 100% rename from ci/pr-check/templates/comment.md rename to ci/crates/pr-check/templates/comment.md diff --git a/ci/pr-check/tests/cli.rs b/ci/crates/pr-check/tests/cli.rs similarity index 100% rename from ci/pr-check/tests/cli.rs rename to ci/crates/pr-check/tests/cli.rs diff --git a/ci/render/.gitignore b/ci/crates/render/.gitignore similarity index 100% rename from ci/render/.gitignore rename to ci/crates/render/.gitignore diff --git a/ci/render/Cargo.toml b/ci/crates/render/Cargo.toml similarity index 100% rename from ci/render/Cargo.toml rename to ci/crates/render/Cargo.toml diff --git a/ci/render/clippy.toml b/ci/crates/render/clippy.toml similarity index 100% rename from ci/render/clippy.toml rename to ci/crates/render/clippy.toml diff --git a/ci/render/src/bin/main.rs b/ci/crates/render/src/bin/main.rs similarity index 99% rename from ci/render/src/bin/main.rs rename to ci/crates/render/src/bin/main.rs index bde31c038c..f500de2417 100644 --- a/ci/render/src/bin/main.rs +++ b/ci/crates/render/src/bin/main.rs @@ -214,7 +214,7 @@ mod tests { #[test] fn parses_catalog() -> Result<()> { - let data = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../data"); + let data = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../../data"); let tags: Tags = read_yaml(&data.join("tags.yml"))?; let tools: Vec = read_entries(&data.join("tools"))?; let collections: Vec = read_entries(&data.join("collections"))?; diff --git a/ci/render/src/deprecation.rs b/ci/crates/render/src/deprecation.rs similarity index 100% rename from ci/render/src/deprecation.rs rename to ci/crates/render/src/deprecation.rs diff --git a/ci/render/src/lib.rs b/ci/crates/render/src/lib.rs similarity index 100% rename from ci/render/src/lib.rs rename to ci/crates/render/src/lib.rs diff --git a/ci/render/src/lints.rs b/ci/crates/render/src/lints.rs similarity index 100% rename from ci/render/src/lints.rs rename to ci/crates/render/src/lints.rs diff --git a/ci/render/src/regression_tests.rs b/ci/crates/render/src/regression_tests.rs similarity index 100% rename from ci/render/src/regression_tests.rs rename to ci/crates/render/src/regression_tests.rs diff --git a/ci/render/src/stats.rs b/ci/crates/render/src/stats.rs similarity index 100% rename from ci/render/src/stats.rs rename to ci/crates/render/src/stats.rs diff --git a/ci/render/src/types.rs b/ci/crates/render/src/types.rs similarity index 100% rename from ci/render/src/types.rs rename to ci/crates/render/src/types.rs diff --git a/ci/render/templates/README.md b/ci/crates/render/templates/README.md similarity index 100% rename from ci/render/templates/README.md rename to ci/crates/render/templates/README.md From 08ac720390e25606fa69441fc1349927aca4719e Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:06:00 +0200 Subject: [PATCH 04/13] Give contribution criteria a shared Check trait --- ci/crates/pr-check/src/checks.rs | 211 +++++++++++++++++++++++++++++ ci/crates/pr-check/src/criteria.rs | 146 ++++---------------- ci/crates/pr-check/src/main.rs | 1 + ci/crates/pr-check/src/network.rs | 24 ++-- 4 files changed, 250 insertions(+), 132 deletions(-) create mode 100644 ci/crates/pr-check/src/checks.rs diff --git a/ci/crates/pr-check/src/checks.rs b/ci/crates/pr-check/src/checks.rs new file mode 100644 index 0000000000..368b26ad65 --- /dev/null +++ b/ci/crates/pr-check/src/checks.rs @@ -0,0 +1,211 @@ +//! Individual contribution checks over already-fetched metadata. + +use anyhow::{Context, Result}; +use chrono::{DateTime, Months, Utc}; + +use crate::criteria::RepoInfo; +use crate::report::CheckResult; + +const MIN_STARS: u64 = 20; +const MIN_CONTRIBUTORS: usize = 2; +const MIN_AGE_MONTHS: u32 = 6; + +/// Evaluates a criterion without I/O. Unavailable evidence produces a skipped check. +pub trait Check { + /// Returns an error if evaluation cannot complete, such as invalid date arithmetic. + fn check(&self) -> Result; +} + +pub struct Stars<'a> { + pub repository: &'a Result>, +} + +impl Check for Stars<'_> { + fn check(&self) -> Result { + Ok(match self.repository { + Ok(Some(info)) => { + let stars = info.stargazers_count; + if stars >= MIN_STARS { + CheckResult::Pass(format!("{stars} stars")) + } else { + CheckResult::Fail(format!("{stars} stars (minimum is {MIN_STARS})")) + } + } + Ok(None) => CheckResult::Skip("repository not found".into()), + Err(error) => CheckResult::Skip(format!("Could not fetch repo info: {error}")), + }) + } +} + +pub struct Contributors<'a> { + pub count: &'a Result>, +} + +impl Check for Contributors<'_> { + fn check(&self) -> Result { + Ok(match self.count { + Ok(Some(count)) => { + if *count >= MIN_CONTRIBUTORS { + CheckResult::Pass(format!("{count} human contributors")) + } else { + CheckResult::Fail(format!( + "{count} human contributor(s) (minimum is {MIN_CONTRIBUTORS})" + )) + } + } + Ok(None) => CheckResult::Skip("repository not found".into()), + Err(error) => CheckResult::Skip(format!("Could not fetch contributors: {error}")), + }) + } +} + +pub struct RepositoryAge<'a> { + pub repository: &'a Result>, + pub now: DateTime, +} + +impl Check for RepositoryAge<'_> { + fn check(&self) -> Result { + Ok(match self.repository { + Ok(Some(info)) => { + let minimum_created_at = + self.now + .checked_sub_months(Months::new(MIN_AGE_MONTHS)) + .context("Current date cannot be shifted back by six months")?; + let days = self.now.signed_duration_since(info.created_at).num_days(); + if info.created_at <= minimum_created_at { + CheckResult::Pass(format!("created {days} days ago (at least 6 months)")) + } else { + let eligible_at = info.created_at + .checked_add_months(Months::new(MIN_AGE_MONTHS)) + .with_context(|| format!( + "Repository creation date {} cannot be shifted forward by {MIN_AGE_MONTHS} months", + info.created_at + ))?; + let remaining = eligible_at + .signed_duration_since(self.now) + .num_days() + .max(1); + CheckResult::Fail(format!( + "created {days} days ago, needs {remaining} more days to meet the 6-month minimum" + )) + } + } + Ok(None) => CheckResult::Skip("repository not found".into()), + Err(_) => CheckResult::Skip("Could not determine age (repo info unavailable)".into()), + }) + } +} + +pub struct DomainAge<'a> { + pub domain: &'a str, + pub registration: &'a Result>>, + pub now: DateTime, +} + +impl Check for DomainAge<'_> { + fn check(&self) -> Result { + let registered = match self.registration { + Ok(Some(registered)) => *registered, + Ok(None) => { + return Ok(CheckResult::Skip( + "Domain registration date unavailable; manual review required".into(), + )); + } + Err(error) => { + return Ok(CheckResult::Skip(format!( + "Could not check domain registration: {error}" + ))); + } + }; + let Some(eligible) = registered.checked_add_months(Months::new(MIN_AGE_MONTHS)) else { + return Ok(CheckResult::Skip("Invalid domain registration date".into())); + }; + if registered > self.now { + return Ok(CheckResult::Skip( + "Domain registration date is in the future; manual review required".into(), + )); + } + if self.now < eligible { + Ok(CheckResult::Fail(format!( + "The homepage domain `{}` was registered on {} and reaches the six-month minimum on {}.", + self.domain, + registered.format("%B %-d, %Y"), + eligible.format("%B %-d, %Y") + ))) + } else { + Ok(CheckResult::Pass(format!( + "Domain registered on {} (at least six months ago). Service age still requires manual review.", + registered.format("%B %-d, %Y") + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn domain_age_uses_six_calendar_months() -> Result<()> { + let registered = "2026-05-01T20:44:07Z".parse::>()?; + let before = "2026-11-01T20:44:06Z".parse::>()?; + let boundary = "2026-11-01T20:44:07Z".parse::>()?; + let check = |registered, now| { + DomainAge { + domain: "battletest.dev", + registration: &Ok(Some(registered)), + now, + } + .check() + }; + let result = check(registered, before)?; + assert!(result.is_fail()); + assert!(result.message().contains("registered on May 1, 2026")); + assert!(result.message().contains("minimum on November 1, 2026")); + assert!(check(registered, boundary)?.is_pass()); + assert!(matches!(check(boundary, registered)?, CheckResult::Skip(_))); + Ok(()) + } + + #[test] + fn all_checks_treat_unavailable_evidence_as_unverified() -> Result<()> { + let now = "2026-09-17T00:00:00Z".parse()?; + for unavailable in [false, true] { + let repository = if unavailable { + Err(anyhow::anyhow!("API unavailable")) + } else { + Ok(None) + }; + let count = if unavailable { + Err(anyhow::anyhow!("API unavailable")) + } else { + Ok(None) + }; + let registration = if unavailable { + Err(anyhow::anyhow!("RDAP unavailable")) + } else { + Ok(None) + }; + let checks: [&dyn Check; 4] = [ + &Stars { + repository: &repository, + }, + &Contributors { count: &count }, + &RepositoryAge { + repository: &repository, + now, + }, + &DomainAge { + domain: "example.com", + registration: ®istration, + now, + }, + ]; + for check in checks { + assert!(matches!(check.check()?, CheckResult::Skip(_))); + } + } + Ok(()) + } +} diff --git a/ci/crates/pr-check/src/criteria.rs b/ci/crates/pr-check/src/criteria.rs index a598fb1fc4..450bb020d2 100644 --- a/ci/crates/pr-check/src/criteria.rs +++ b/ci/crates/pr-check/src/criteria.rs @@ -1,10 +1,11 @@ //! Contribution criteria and URL classification, independent of network access. use anyhow::{Context, Result, ensure}; -use chrono::{DateTime, Months, Utc}; +use chrono::{DateTime, Utc}; use serde::Deserialize; -use crate::report::{CheckResult, ToolReport}; +use crate::checks::{Check, Contributors, RepositoryAge, Stars}; +use crate::report::ToolReport; /// A minimal tool entry parsed from `data/tools/.yml`. /// Only the fields needed for the contributing criteria check are required. @@ -43,10 +44,6 @@ impl Contributor { // Use exact logins rather than broad patterns that could exclude human contributors. const AUTOMATION_LOGINS: &[&str] = &["claude", "dependabot", "renovate-bot"]; -const MIN_STARS: u64 = 20; -const MIN_CONTRIBUTORS: usize = 2; -const MIN_AGE_MONTHS: u32 = 6; - /// A repository parsed from a GitHub HTTP(S) URL, borrowing its owner and name. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct GithubRepo<'a> { @@ -82,64 +79,22 @@ impl std::fmt::Display for GithubRepo<'_> { pub fn repository_report( tool: &ToolEntry, repo_result: &Result>, - contributors_result: Result>, + contributors_result: &Result>, now: DateTime, ) -> Result { - let stars_check = match repo_result { - Ok(Some(info)) => { - let s = info.stargazers_count; - if s >= MIN_STARS { - CheckResult::Pass(format!("{s} stars")) - } else { - CheckResult::Fail(format!("{s} stars (minimum is {MIN_STARS})")) - } - } - Ok(None) => CheckResult::Skip("repository not found".into()), - Err(e) => CheckResult::Skip(format!("Could not fetch repo info: {e}")), - }; - - let age_check = match repo_result { - Ok(Some(info)) => { - let minimum_created_at = now - .checked_sub_months(Months::new(MIN_AGE_MONTHS)) - .context("Current date cannot be shifted back by six months")?; - let days = now.signed_duration_since(info.created_at).num_days(); - - if info.created_at <= minimum_created_at { - CheckResult::Pass(format!("created {days} days ago (at least 6 months)")) - } else { - let eligible_at = info - .created_at - .checked_add_months(Months::new(MIN_AGE_MONTHS)) - .with_context(|| { - format!( - "Repository creation date {} cannot be shifted forward by {MIN_AGE_MONTHS} months", - info.created_at - ) - })?; - let remaining = eligible_at.signed_duration_since(now).num_days().max(1); - CheckResult::Fail(format!( - "created {days} days ago, needs {remaining} more days to meet the 6-month minimum" - )) - } - } - Ok(None) => CheckResult::Skip("repository not found".into()), - Err(_) => CheckResult::Skip("Could not determine age (repo info unavailable)".into()), - }; - - let contributors_check = match contributors_result { - Ok(Some(count)) => { - if count >= MIN_CONTRIBUTORS { - CheckResult::Pass(format!("{count} human contributors")) - } else { - CheckResult::Fail(format!( - "{count} human contributor(s) (minimum is {MIN_CONTRIBUTORS})" - )) - } - } - Ok(None) => CheckResult::Skip("repository not found".into()), - Err(e) => CheckResult::Skip(format!("Could not fetch contributors: {e}")), - }; + let stars = Stars { + repository: repo_result, + } + .check()?; + let age = RepositoryAge { + repository: repo_result, + now, + } + .check()?; + let contributors = Contributors { + count: contributors_result, + } + .check()?; let repo_not_found = matches!(repo_result, Ok(None)); let note = repo_not_found.then_some( @@ -149,9 +104,9 @@ pub fn repository_report( Ok(ToolReport { name: tool.name.clone(), source: tool.source.clone(), - stars: stars_check, - contributors: contributors_check, - age: age_check, + stars, + contributors, + age, domain: None, note: note.map(str::to_owned), }) @@ -167,34 +122,6 @@ pub fn homepage_domain(homepage: &str) -> Option { Some(domain.strip_prefix("www.").unwrap_or(domain).to_owned()) } -pub fn domain_age_result( - domain: &str, - registered: DateTime, - now: DateTime, -) -> CheckResult { - let Some(eligible) = registered.checked_add_months(Months::new(MIN_AGE_MONTHS)) else { - return CheckResult::Skip("Invalid domain registration date".into()); - }; - if registered > now { - return CheckResult::Skip( - "Domain registration date is in the future; manual review required".into(), - ); - } - let message = format!( - "The homepage domain `{domain}` was registered on {} and reaches the six-month minimum on {}.", - registered.format("%B %-d, %Y"), - eligible.format("%B %-d, %Y") - ); - if now < eligible { - CheckResult::Fail(message) - } else { - CheckResult::Pass(format!( - "Domain registered on {} (at least six months ago). Service age still requires manual review.", - registered.format("%B %-d, %Y") - )) - } -} - #[cfg(test)] mod tests { use super::*; @@ -235,8 +162,8 @@ mod tests { let count = |accounts: &[Contributor]| accounts.iter().filter(|c| c.counts_as_human()).count(); assert_eq!(count(&contributors[..4]), 1); - assert!(count(&contributors[..4]) < MIN_CONTRIBUTORS); - assert_eq!(count(&contributors), MIN_CONTRIBUTORS); + assert!(count(&contributors[..4]) < 2); + assert_eq!(count(&contributors), 2); Ok(()) } @@ -306,23 +233,6 @@ mod tests { } } - #[test] - fn domain_age_uses_six_calendar_months() -> Result<()> { - let registered = "2026-05-01T20:44:07Z".parse::>()?; - let before = "2026-11-01T20:44:06Z".parse::>()?; - let boundary = "2026-11-01T20:44:07Z".parse::>()?; - let result = domain_age_result("battletest.dev", registered, before); - assert!(result.is_fail()); - assert!(result.message().contains("registered on May 1, 2026")); - assert!(result.message().contains("minimum on November 1, 2026")); - assert!(domain_age_result("battletest.dev", registered, boundary).is_pass()); - assert!(matches!( - domain_age_result("battletest.dev", boundary, registered), - CheckResult::Skip(_) - )); - Ok(()) - } - #[test] fn excludes_automation_even_when_github_reports_a_user() { for login in [ @@ -367,7 +277,7 @@ mod tests { stargazers_count: stars, created_at, })), - Ok(Some(contributors)), + &Ok(Some(contributors)), boundary + chrono::Duration::seconds(seconds), )?; assert_eq!(report.stars.is_pass(), stars >= 20); @@ -400,7 +310,7 @@ mod tests { stargazers_count: 20, created_at: created.parse()?, })), - Ok(Some(2)), + &Ok(Some(2)), now, )?; assert_eq!(report.age.is_pass(), passes); @@ -411,7 +321,7 @@ mod tests { #[test] fn missing_or_unavailable_metadata_is_not_a_verified_failure() -> Result<()> { let now = "2026-09-01T12:00:00Z".parse::>()?; - let missing = repository_report(&example_tool(), &Ok(None), Ok(None), now)?; + let missing = repository_report(&example_tool(), &Ok(None), &Ok(None), now)?; assert_eq!(missing.status(), "REVIEW"); assert_eq!(missing.stars.message(), "repository not found"); assert_eq!(missing.age.message(), "repository not found"); @@ -426,7 +336,7 @@ mod tests { let unavailable = repository_report( &example_tool(), &Err(anyhow::anyhow!("rate limited")), - Err(anyhow::anyhow!("connection failed")), + &Err(anyhow::anyhow!("connection failed")), now, )?; assert_eq!(unavailable.status(), "REVIEW"); @@ -447,7 +357,7 @@ mod tests { let verified = repository_report( &example_tool(), &Err(anyhow::anyhow!("rate limited")), - Ok(Some(1)), + &Ok(Some(1)), now, )?; assert_eq!(verified.status(), "FAIL"); @@ -468,7 +378,7 @@ mod tests { stargazers_count: stars, created_at: created.parse()?, })), - Err(anyhow::anyhow!("API unavailable")), + &Err(anyhow::anyhow!("API unavailable")), now, )?; assert_eq!(report.status(), expected); diff --git a/ci/crates/pr-check/src/main.rs b/ci/crates/pr-check/src/main.rs index 9778d93e76..f3d41d5227 100644 --- a/ci/crates/pr-check/src/main.rs +++ b/ci/crates/pr-check/src/main.rs @@ -21,6 +21,7 @@ //! `GITHUB_TOKEN` - a token for reading public repository metadata //! `COMMENT_OUTPUT_FILE` - (optional) report output path; defaults to stdout +mod checks; mod criteria; mod network; mod report; diff --git a/ci/crates/pr-check/src/network.rs b/ci/crates/pr-check/src/network.rs index 15423f918d..c78e3b92a8 100644 --- a/ci/crates/pr-check/src/network.rs +++ b/ci/crates/pr-check/src/network.rs @@ -4,9 +4,9 @@ use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; use serde::{Deserialize, de::DeserializeOwned}; +use crate::checks::{Check, DomainAge}; use crate::criteria::{ - Contributor, GithubRepo, RepoInfo, ToolEntry, domain_age_result, homepage_domain, - repository_report, + Contributor, GithubRepo, RepoInfo, ToolEntry, homepage_domain, repository_report, }; use crate::report::{CheckResult, ToolReport}; @@ -103,7 +103,7 @@ pub async fn check_tool(client: &GithubClient, tool: &ToolEntry) -> Result Result, } -async fn check_domain_age(domain: &str) -> CheckResult { - match fetch_domain_registration(domain).await { - Ok(Some(registered)) => domain_age_result(domain, registered, Utc::now()), - Ok(None) => { - CheckResult::Skip("Domain registration date unavailable; manual review required".into()) - } - Err(error) => CheckResult::Skip(format!("Could not check domain registration: {error}")), - } -} - async fn fetch_domain_registration(domain: &str) -> Result>> { // RDAP requests must never carry the GitHub token. rdap.org redirects to the registry. let client = reqwest::Client::builder() From 13b13318a9a8df047a95eec95ad74899f4ea1470 Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:12:29 +0200 Subject: [PATCH 05/13] Use displayable comments and share GitHub repository parsing --- ci/Cargo.lock | 9 ++ ci/Cargo.toml | 1 + ci/crates/github-repo/Cargo.toml | 15 +++ ci/crates/github-repo/src/lib.rs | 160 ++++++++++++++++++++++++++++ ci/crates/pr-check/Cargo.toml | 1 + ci/crates/pr-check/src/criteria.rs | 101 +----------------- ci/crates/pr-check/src/main.rs | 8 +- ci/crates/pr-check/src/network.rs | 5 +- ci/crates/pr-check/src/report.rs | 45 ++++---- ci/crates/render/Cargo.toml | 1 + ci/crates/render/src/deprecation.rs | 51 +++------ 11 files changed, 229 insertions(+), 168 deletions(-) create mode 100644 ci/crates/github-repo/Cargo.toml create mode 100644 ci/crates/github-repo/src/lib.rs diff --git a/ci/Cargo.lock b/ci/Cargo.lock index a3a12e0c27..43017138ce 100644 --- a/ci/Cargo.lock +++ b/ci/Cargo.lock @@ -494,6 +494,13 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "github-repo" +version = "0.1.0" +dependencies = [ + "anyhow", +] + [[package]] name = "glob" version = "0.3.4" @@ -961,6 +968,7 @@ dependencies = [ "askama", "chrono", "clap", + "github-repo", "reqwest", "serde", "serde-saphyr", @@ -1082,6 +1090,7 @@ dependencies = [ "askama", "chrono", "clap", + "github-repo", "reqwest", "serde", "serde-saphyr", diff --git a/ci/Cargo.toml b/ci/Cargo.toml index 1748312e35..6612f3f0e4 100644 --- a/ci/Cargo.toml +++ b/ci/Cargo.toml @@ -14,6 +14,7 @@ anyhow = "1.0.104" askama = "0.16" chrono = { version = "0.4.45", features = ["serde"] } clap = { version = "4.6.7", features = ["derive"] } +github-repo = { path = "crates/github-repo" } reqwest = { version = "0.13.5", default-features = false, features = ["json", "rustls", "system-proxy"] } serde = { version = "1.0.229", features = ["derive"] } serde_json = "1.0.151" diff --git a/ci/crates/github-repo/Cargo.toml b/ci/crates/github-repo/Cargo.toml new file mode 100644 index 0000000000..9081275083 --- /dev/null +++ b/ci/crates/github-repo/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "github-repo" +version = "0.1.0" +edition.workspace = true +rust-version.workspace = true +description = "Shared GitHub repository URL parsing for catalog CI" +license.workspace = true +repository.workspace = true +publish.workspace = true + +[lints] +workspace = true + +[dependencies] +anyhow.workspace = true diff --git a/ci/crates/github-repo/src/lib.rs b/ci/crates/github-repo/src/lib.rs new file mode 100644 index 0000000000..c71bc4997c --- /dev/null +++ b/ci/crates/github-repo/src/lib.rs @@ -0,0 +1,160 @@ +//! Shared GitHub repository URL classification. + +use anyhow::{Context, Result, ensure}; + +/// A repository parsed from a GitHub HTTP(S) URL, borrowing its owner and name. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct GithubRepo<'a> { + owner: &'a str, + name: &'a str, +} + +impl<'a> TryFrom<&'a str> for GithubRepo<'a> { + type Error = anyhow::Error; + + /// Parses an HTTP(S) GitHub repository URL, ignoring trailing slashes. + /// + /// # Errors + /// + /// Returns an error for an unsupported prefix, missing owner or repository, + /// or a repository path containing a subpath. + fn try_from(url: &'a str) -> Result { + let url = url.trim_end_matches('/'); + let path = url + .strip_prefix("https://github.com/") + .or_else(|| url.strip_prefix("http://github.com/")) + .context("Expected a GitHub HTTP(S) URL")?; + let (owner, name) = path.split_once('/').context("Expected owner/repository")?; + ensure!( + !owner.is_empty() && !name.is_empty() && !name.contains('/'), + "Expected a repository URL with no subpath" + ); + Ok(Self { owner, name }) + } +} + +impl std::fmt::Display for GithubRepo<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}/{}", self.owner, self.name) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_plain_github_url() -> Result<()> { + let repo = GithubRepo::try_from("https://github.com/owner/repo")?; + assert_eq!( + repo, + GithubRepo { + owner: "owner", + name: "repo" + } + ); + assert_eq!(repo.to_string(), "owner/repo"); + Ok(()) + } + + #[test] + fn parses_trailing_slash() -> Result<()> { + let repo = GithubRepo::try_from("https://github.com/owner/repo/")?; + assert_eq!( + repo, + GithubRepo { + owner: "owner", + name: "repo" + } + ); + Ok(()) + } + + #[test] + fn preserves_repository_text_without_url_normalization() -> Result<()> { + for path in [ + "Owner/Repo.git", + "owner/repo?tab=readme", + "owner/repo#readme", + "owner/repo%2Ftree", + "owner/repo ", + ] { + let url = format!("https://github.com/{path}"); + assert_eq!(GithubRepo::try_from(url.as_str())?.to_string(), path); + } + Ok(()) + } + + #[test] + fn preserves_parse_error_messages() { + for (url, expected) in [ + ( + "https://gitlab.com/owner/repo", + "Expected a GitHub HTTP(S) URL", + ), + ("https://github.com/", "Expected a GitHub HTTP(S) URL"), + ("https://github.com/owner/", "Expected owner/repository"), + ( + "https://github.com//repo", + "Expected a repository URL with no subpath", + ), + ( + "https://github.com/owner/repo/tree/main", + "Expected a repository URL with no subpath", + ), + ] { + assert_eq!( + GithubRepo::try_from(url) + .err() + .map(|error| error.to_string()), + Some(expected.to_owned()), + "{url}" + ); + } + } + + #[test] + fn rejects_subpath() { + assert!(GithubRepo::try_from("https://github.com/owner/repo/tree/main/subdir").is_err()); + } + + #[test] + fn rejects_gitlab() { + assert!(GithubRepo::try_from("https://gitlab.com/owner/repo").is_err()); + } + + #[test] + fn rejects_missing_repo() { + assert!(GithubRepo::try_from("https://github.com/owner").is_err()); + } + + #[test] + fn github_url_classification_preserves_supported_forms() { + for url in [ + "http://github.com/owner/repo", + "http://github.com/owner/repo/", + "https://github.com/owner/repo///", + ] { + assert_eq!( + GithubRepo::try_from(url).ok(), + Some(GithubRepo { + owner: "owner", + name: "repo" + }) + ); + } + for url in [ + "https://github.com/", + "https://github.com//repo", + "https://github.com/owner/", + "https://github.com/owner/repo//tree/main", + "https://github.com/owner/repo/tree/main", + "https://GitHub.com/owner/repo", + " https://github.com/owner/repo", + "https://github.com.evil/owner/repo", + "git@github.com:owner/repo.git", + ] { + assert!(GithubRepo::try_from(url).is_err(), "{url}"); + } + } +} diff --git a/ci/crates/pr-check/Cargo.toml b/ci/crates/pr-check/Cargo.toml index 9b92c1c8ab..ff1a48e61a 100644 --- a/ci/crates/pr-check/Cargo.toml +++ b/ci/crates/pr-check/Cargo.toml @@ -16,6 +16,7 @@ anyhow.workspace = true askama.workspace = true chrono.workspace = true clap.workspace = true +github-repo.workspace = true reqwest.workspace = true serde.workspace = true serde-saphyr.workspace = true diff --git a/ci/crates/pr-check/src/criteria.rs b/ci/crates/pr-check/src/criteria.rs index 450bb020d2..12a7bcc7b8 100644 --- a/ci/crates/pr-check/src/criteria.rs +++ b/ci/crates/pr-check/src/criteria.rs @@ -1,6 +1,6 @@ //! Contribution criteria and URL classification, independent of network access. -use anyhow::{Context, Result, ensure}; +use anyhow::Result; use chrono::{DateTime, Utc}; use serde::Deserialize; @@ -44,37 +44,6 @@ impl Contributor { // Use exact logins rather than broad patterns that could exclude human contributors. const AUTOMATION_LOGINS: &[&str] = &["claude", "dependabot", "renovate-bot"]; -/// A repository parsed from a GitHub HTTP(S) URL, borrowing its owner and name. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct GithubRepo<'a> { - owner: &'a str, - name: &'a str, -} - -impl<'a> TryFrom<&'a str> for GithubRepo<'a> { - type Error = anyhow::Error; - - fn try_from(url: &'a str) -> Result { - let url = url.trim_end_matches('/'); - let path = url - .strip_prefix("https://github.com/") - .or_else(|| url.strip_prefix("http://github.com/")) - .context("Expected a GitHub HTTP(S) URL")?; - let (owner, name) = path.split_once('/').context("Expected owner/repository")?; - ensure!( - !owner.is_empty() && !name.is_empty() && !name.contains('/'), - "Expected a repository URL with no subpath" - ); - Ok(Self { owner, name }) - } -} - -impl std::fmt::Display for GithubRepo<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}/{}", self.owner, self.name) - } -} - /// Evaluate fetched metadata without performing I/O; unavailable checks require review. pub fn repository_report( tool: &ToolEntry, @@ -167,48 +136,6 @@ mod tests { Ok(()) } - #[test] - fn parses_plain_github_url() -> Result<()> { - let repo = GithubRepo::try_from("https://github.com/owner/repo")?; - assert_eq!( - repo, - GithubRepo { - owner: "owner", - name: "repo" - } - ); - assert_eq!(repo.to_string(), "owner/repo"); - Ok(()) - } - - #[test] - fn parses_trailing_slash() -> Result<()> { - let repo = GithubRepo::try_from("https://github.com/owner/repo/")?; - assert_eq!( - repo, - GithubRepo { - owner: "owner", - name: "repo" - } - ); - Ok(()) - } - - #[test] - fn rejects_subpath() { - assert!(GithubRepo::try_from("https://github.com/owner/repo/tree/main/subdir").is_err()); - } - - #[test] - fn rejects_gitlab() { - assert!(GithubRepo::try_from("https://gitlab.com/owner/repo").is_err()); - } - - #[test] - fn rejects_missing_repo() { - assert!(GithubRepo::try_from("https://github.com/owner").is_err()); - } - #[test] fn homepage_domains_are_not_reduced_to_hosting_providers() { assert_eq!( @@ -385,30 +312,4 @@ mod tests { } Ok(()) } - - #[test] - fn github_url_classification_preserves_supported_forms() { - for url in [ - "http://github.com/owner/repo", - "https://github.com/owner/repo///", - ] { - assert_eq!( - GithubRepo::try_from(url).ok(), - Some(GithubRepo { - owner: "owner", - name: "repo" - }) - ); - } - for url in [ - "https://github.com/", - "https://github.com//repo", - "https://github.com/owner/", - "https://github.com/owner/repo//tree/main", - "https://github.com.evil/owner/repo", - "git@github.com:owner/repo.git", - ] { - assert!(GithubRepo::try_from(url).is_err(), "{url}"); - } - } } diff --git a/ci/crates/pr-check/src/main.rs b/ci/crates/pr-check/src/main.rs index f3d41d5227..c29dcf091f 100644 --- a/ci/crates/pr-check/src/main.rs +++ b/ci/crates/pr-check/src/main.rs @@ -34,7 +34,7 @@ use std::process::ExitCode; use criteria::ToolEntry; use network::{GithubClient, check_tool}; -use report::{render_comment, report_exit_code}; +use report::{Comment, report_exit_code}; #[derive(Debug, Parser)] #[command(version, about)] @@ -85,7 +85,7 @@ async fn main() -> Result { reports.push(report); } - let comment_body = render_comment(&reports)?; + let comment = Comment::from(reports.as_slice()); if let Some(output_file) = env::var("COMMENT_OUTPUT_FILE") .ok() @@ -95,11 +95,11 @@ async fn main() -> Result { std::fs::create_dir_all(parent) .with_context(|| format!("Failed to create directory for {output_file}"))?; } - std::fs::write(&output_file, &comment_body) + std::fs::write(&output_file, comment.to_string()) .with_context(|| format!("Failed to write comment to {output_file}"))?; eprintln!("Comment written to {output_file}"); } else { - println!("{comment_body}"); + println!("{comment}"); } let exit_code = report_exit_code(&reports); diff --git a/ci/crates/pr-check/src/network.rs b/ci/crates/pr-check/src/network.rs index c78e3b92a8..91f22b5920 100644 --- a/ci/crates/pr-check/src/network.rs +++ b/ci/crates/pr-check/src/network.rs @@ -2,12 +2,11 @@ use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; +use github_repo::GithubRepo; use serde::{Deserialize, de::DeserializeOwned}; use crate::checks::{Check, DomainAge}; -use crate::criteria::{ - Contributor, GithubRepo, RepoInfo, ToolEntry, homepage_domain, repository_report, -}; +use crate::criteria::{Contributor, RepoInfo, ToolEntry, homepage_domain, repository_report}; use crate::report::{CheckResult, ToolReport}; pub struct GithubClient { diff --git a/ci/crates/pr-check/src/report.rs b/ci/crates/pr-check/src/report.rs index ea7207ab75..86c642edae 100644 --- a/ci/crates/pr-check/src/report.rs +++ b/ci/crates/pr-check/src/report.rs @@ -1,6 +1,5 @@ //! Report status, Markdown rendering, and workflow exit-code contract. -use anyhow::{Context, Result}; use askama::Template; // Identifies the report as output from the contribution checker. @@ -73,30 +72,25 @@ impl ToolReport { } } +/// A Markdown comment; Askama derives its `Display` implementation. #[derive(Template)] #[template(path = "comment.md")] -struct CommentTemplate<'a> { +pub struct Comment<'a> { marker: &'a str, reports: &'a [ToolReport], any_failures: bool, should_close: bool, } -/// Renders all tool reports into a Markdown comment body. -/// -/// # Errors -/// -/// Returns an error if the template fails to render. -pub fn render_comment(reports: &[ToolReport]) -> Result { - let any_failures = reports.iter().any(ToolReport::has_nonpassing_checks); - CommentTemplate { - marker: COMMENT_MARKER, - reports, - any_failures, - should_close: reports.iter().any(ToolReport::should_close), +impl<'a> From<&'a [ToolReport]> for Comment<'a> { + fn from(reports: &'a [ToolReport]) -> Self { + Self { + marker: COMMENT_MARKER, + reports, + any_failures: reports.iter().any(ToolReport::has_nonpassing_checks), + should_close: reports.iter().any(ToolReport::should_close), + } } - .render() - .context("Failed to render comment template") } pub fn report_exit_code(reports: &[ToolReport]) -> u8 { @@ -110,6 +104,7 @@ pub fn report_exit_code(reports: &[ToolReport]) -> u8 { #[cfg(test)] mod tests { use super::*; + use anyhow::Result; fn passing_report() -> ToolReport { ToolReport { @@ -127,7 +122,7 @@ mod tests { fn passing_tools_do_not_close_pr() -> Result<()> { let reports = [passing_report()]; assert_eq!(report_exit_code(&reports), 0); - let comment = render_comment(&reports)?; + let comment = Comment::from(reports.as_slice()).render()?; assert!(comment.contains("All tool eligibility criteria passed")); assert!(!comment.contains("closing this pull request")); assert_eq!(report_exit_code(&[]), 0); @@ -147,7 +142,7 @@ mod tests { assert_eq!(report.status(), "FAIL"); let reports = [passing_report(), report]; assert_eq!(report_exit_code(&reports), 2); - let comment = render_comment(&reports)?; + let comment = Comment::from(reports.as_slice()).render()?; assert!(comment.contains("closing this pull request")); assert!(comment.contains("submit a new pull request once all criteria are met")); } @@ -164,7 +159,7 @@ mod tests { assert_eq!(report.status(), "REVIEW"); let reports = [report]; assert_eq!(report_exit_code(&reports), 1); - let comment = render_comment(&reports)?; + let comment = Comment::from(reports.as_slice()).render()?; assert!(comment.contains("needs manual review")); assert!(!comment.contains("closing this pull request")); } @@ -196,7 +191,7 @@ mod tests { assert_eq!(report.status(), "REVIEW"); let reports = [report]; assert_eq!(report_exit_code(&reports), 1); - let comment = render_comment(&reports)?; + let comment = Comment::from(reports.as_slice()).render()?; assert!(comment.contains("Homepage domain age")); assert!(comment.contains("https://rdap.org/domain/battletest.dev")); assert!(!comment.contains("closing this pull request")); @@ -206,14 +201,16 @@ mod tests { } #[test] fn render_comment_no_files() -> Result<()> { - let comment = render_comment(&[])?; + let template = Comment::from([].as_slice()); + let comment = template.render()?; + assert_eq!(template.to_string(), comment); assert!(comment.contains("No new tool files detected")); Ok(()) } #[test] fn render_comment_contains_marker() -> Result<()> { - let comment = render_comment(&[])?; + let comment = Comment::from([].as_slice()).render()?; assert!(comment.contains(COMMENT_MARKER)); Ok(()) } @@ -251,7 +248,9 @@ mod tests { assert_eq!(report.status(), status); let reports = [passing_report(), report]; assert_eq!(report_exit_code(&reports), exit); - let comment = render_comment(&reports)?; + let template = Comment::from(reports.as_slice()); + let comment = template.render()?; + assert_eq!(template.to_string(), comment); assert!(comment.starts_with(COMMENT_MARKER)); assert_eq!(comment.contains("closing this pull request"), close); assert_eq!(comment.contains("needs manual review"), review && !close); diff --git a/ci/crates/render/Cargo.toml b/ci/crates/render/Cargo.toml index cabcfe2a66..efe44c1709 100644 --- a/ci/crates/render/Cargo.toml +++ b/ci/crates/render/Cargo.toml @@ -19,6 +19,7 @@ anyhow.workspace = true askama.workspace = true chrono.workspace = true clap.workspace = true +github-repo.workspace = true reqwest.workspace = true serde.workspace = true serde_json.workspace = true diff --git a/ci/crates/render/src/deprecation.rs b/ci/crates/render/src/deprecation.rs index e8b81a8827..5894f4ec8d 100644 --- a/ci/crates/render/src/deprecation.rs +++ b/ci/crates/render/src/deprecation.rs @@ -1,5 +1,6 @@ use anyhow::{Context, Result}; use chrono::{DateTime, Local, NaiveDate, Utc}; +use github_repo::GithubRepo; use serde::Deserialize; use crate::types::Entry; @@ -19,22 +20,12 @@ struct CommitAuthor { date: DateTime, } -fn github_coordinates(source: &str) -> Option<(&str, &str)> { - let path = source - .strip_prefix("https://github.com/") - .or_else(|| source.strip_prefix("http://github.com/"))? - .trim_end_matches('/'); - let (owner, repo) = path.split_once('/')?; - (!owner.is_empty() && !repo.is_empty() && !repo.contains('/')).then_some((owner, repo)) -} - async fn latest_commit_date( client: &reqwest::Client, token: &str, - owner: &str, - repo: &str, + repo: GithubRepo<'_>, ) -> Result>> { - let url = format!("https://api.github.com/repos/{owner}/{repo}/commits?per_page=1"); + let url = format!("https://api.github.com/repos/{repo}/commits?per_page=1"); let response = client .get(url) .bearer_auth(token) @@ -42,7 +33,7 @@ async fn latest_commit_date( .header("X-GitHub-Api-Version", "2022-11-28") .send() .await - .with_context(|| format!("Failed to fetch commits for {owner}/{repo}"))?; + .with_context(|| format!("Failed to fetch commits for {repo}"))?; if matches!( response.status(), @@ -53,10 +44,10 @@ async fn latest_commit_date( let commits = response .error_for_status() - .with_context(|| format!("GitHub rejected the commits request for {owner}/{repo}"))? + .with_context(|| format!("GitHub rejected the commits request for {repo}"))? .json::>() .await - .with_context(|| format!("Invalid commits response for {owner}/{repo}"))?; + .with_context(|| format!("Invalid commits response for {repo}"))?; Ok(commits .into_iter() @@ -90,14 +81,18 @@ pub async fn check_deprecated(token: &str, entries: &mut [Entry]) -> Result<()> .context("Failed to build GitHub HTTP client")?; for entry in entries { - let Some((owner, repo)) = entry.source.as_deref().and_then(github_coordinates) else { + let Some(repo) = entry + .source + .as_deref() + .and_then(|url| GithubRepo::try_from(url).ok()) + else { continue; }; - let last_commit = match latest_commit_date(&client, token, owner, repo).await { + let last_commit = match latest_commit_date(&client, token, repo).await { Ok(Some(date)) => date, Ok(None) => continue, Err(error) => { - eprintln!("Could not check {owner}/{repo} for deprecation: {error:#}"); + eprintln!("Could not check {repo} for deprecation: {error:#}"); continue; } }; @@ -112,26 +107,6 @@ pub async fn check_deprecated(token: &str, entries: &mut [Entry]) -> Result<()> mod tests { use super::*; - #[test] - fn parses_github_repository_urls() { - for source in [ - "https://github.com/owner/repo", - "http://github.com/owner/repo/", - "https://github.com/owner/repo///", - ] { - assert_eq!(github_coordinates(source), Some(("owner", "repo"))); - } - for source in [ - "https://github.com/owner/repo/tree/main", - "https://gitlab.com/owner/repo", - "https://github.com//repo", - "https://github.com/owner/", - "https://github.com/owner", - ] { - assert_eq!(github_coordinates(source), None); - } - } - #[test] fn parses_github_author_date_not_committer_date() -> Result<()> { let response: Vec = serde_json::from_str( From c480971eebe61b4951629bc6b7354e5e439bc0d6 Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:13:58 +0200 Subject: [PATCH 06/13] Encapsulate renderer GitHub requests in a client type --- ci/crates/render/src/deprecation.rs | 86 ++++++++++++++++------------- 1 file changed, 49 insertions(+), 37 deletions(-) diff --git a/ci/crates/render/src/deprecation.rs b/ci/crates/render/src/deprecation.rs index 5894f4ec8d..c267bd9ba8 100644 --- a/ci/crates/render/src/deprecation.rs +++ b/ci/crates/render/src/deprecation.rs @@ -20,39 +20,55 @@ struct CommitAuthor { date: DateTime, } -async fn latest_commit_date( - client: &reqwest::Client, - token: &str, - repo: GithubRepo<'_>, -) -> Result>> { - let url = format!("https://api.github.com/repos/{repo}/commits?per_page=1"); - let response = client - .get(url) - .bearer_auth(token) - .header("Accept", "application/vnd.github+json") - .header("X-GitHub-Api-Version", "2022-11-28") - .send() - .await - .with_context(|| format!("Failed to fetch commits for {repo}"))?; - - if matches!( - response.status(), - reqwest::StatusCode::NOT_FOUND | reqwest::StatusCode::CONFLICT - ) { - return Ok(None); +struct GithubClient { + client: reqwest::Client, + token: String, +} + +impl GithubClient { + fn new(token: &str) -> Result { + let client = reqwest::Client::builder() + .user_agent("analysis-tools-render/0.2") + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to build GitHub HTTP client")?; + Ok(Self { + client, + token: token.to_owned(), + }) } - let commits = response - .error_for_status() - .with_context(|| format!("GitHub rejected the commits request for {repo}"))? - .json::>() - .await - .with_context(|| format!("Invalid commits response for {repo}"))?; - - Ok(commits - .into_iter() - .next() - .map(|commit| commit.commit.author.date)) + async fn latest_commit_date(&self, repo: GithubRepo<'_>) -> Result>> { + let url = format!("https://api.github.com/repos/{repo}/commits?per_page=1"); + let response = self + .client + .get(url) + .bearer_auth(&self.token) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .send() + .await + .with_context(|| format!("Failed to fetch commits for {repo}"))?; + + if matches!( + response.status(), + reqwest::StatusCode::NOT_FOUND | reqwest::StatusCode::CONFLICT + ) { + return Ok(None); + } + + let commits = response + .error_for_status() + .with_context(|| format!("GitHub rejected the commits request for {repo}"))? + .json::>() + .await + .with_context(|| format!("Invalid commits response for {repo}"))?; + + Ok(commits + .into_iter() + .next() + .map(|commit| commit.commit.author.date)) + } } fn deprecation_marker(today: NaiveDate, last_commit: DateTime) -> Option { @@ -74,11 +90,7 @@ fn deprecation_marker(today: NaiveDate, last_commit: DateTime) -> Option Result<()> { - let client = reqwest::Client::builder() - .user_agent("analysis-tools-render/0.2") - .timeout(std::time::Duration::from_secs(30)) - .build() - .context("Failed to build GitHub HTTP client")?; + let client = GithubClient::new(token)?; for entry in entries { let Some(repo) = entry @@ -88,7 +100,7 @@ pub async fn check_deprecated(token: &str, entries: &mut [Entry]) -> Result<()> else { continue; }; - let last_commit = match latest_commit_date(&client, token, repo).await { + let last_commit = match client.latest_commit_date(repo).await { Ok(Some(date)) => date, Ok(None) => continue, Err(error) => { From b3375bd428924989c13aa7673295b31747fd1578 Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:14:43 +0200 Subject: [PATCH 07/13] Use crate version in renderer user agent --- ci/crates/render/src/deprecation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/crates/render/src/deprecation.rs b/ci/crates/render/src/deprecation.rs index c267bd9ba8..5bf2f4e708 100644 --- a/ci/crates/render/src/deprecation.rs +++ b/ci/crates/render/src/deprecation.rs @@ -28,7 +28,7 @@ struct GithubClient { impl GithubClient { fn new(token: &str) -> Result { let client = reqwest::Client::builder() - .user_agent("analysis-tools-render/0.2") + .user_agent(concat!("analysis-tools-render/", env!("CARGO_PKG_VERSION"))) .timeout(std::time::Duration::from_secs(30)) .build() .context("Failed to build GitHub HTTP client")?; From 0753fc231df9ac16fbed296166c757ff63dbfc50 Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:24:11 +0200 Subject: [PATCH 08/13] Enforce entry name and tag invariants with validated types --- CONTRIBUTING.md | 4 +- ci/crates/render/src/bin/main.rs | 2 +- ci/crates/render/src/lib.rs | 40 +++--- ci/crates/render/src/lints.rs | 27 ---- ci/crates/render/src/regression_tests.rs | 114 +++++++++++----- ci/crates/render/src/types.rs | 16 +-- ci/crates/render/src/validated.rs | 165 +++++++++++++++++++++++ 7 files changed, 275 insertions(+), 93 deletions(-) delete mode 100644 ci/crates/render/src/lints.rs create mode 100644 ci/crates/render/src/validated.rs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fb557a8972..aa105a660e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -43,10 +43,12 @@ To add a new tool, please create a file in the `data/tools` directory like `data/tools/.yml`. Feel free to check out a few other YAML files in that directory to see how it should look like. +- Use a nonblank tool name of at most **50 UTF-8 bytes** (non-ASCII characters + can take more than one byte). - Make each tool description as precise as possible. Please limit the description to **500 characters**. - Add a license. If it's a proprietary tool, use `license: proprietary`. -- Please add as many tags as possible. You can choose from the tags in +- Add at least one tag, and include as many relevant tags as possible. Choose from `data/tags.yml`. If a tool does not match any existing tag, feel free to add a new tag but also add it to `data/tags.yml`. - For AI-related tools, add `ai-generated-code` if the tool analyzes diff --git a/ci/crates/render/src/bin/main.rs b/ci/crates/render/src/bin/main.rs index f500de2417..c2630cb792 100644 --- a/ci/crates/render/src/bin/main.rs +++ b/ci/crates/render/src/bin/main.rs @@ -177,7 +177,7 @@ mod tests { #[test] fn cached_deprecation_never_overrides_explicit_markers() -> Result<()> { - let fixture = r#"{"name":"Example Tool","categories":[],"tags":[],"license":"MIT","types":[],"homepage":"https://example.com","description":"Example"}"#; + let fixture = r#"{"name":"Example Tool","categories":[],"tags":[{"name":"Rust","value":"rust","tag_type":"language"}],"license":"MIT","types":[],"homepage":"https://example.com","description":"Example"}"#; for cached_marker in [Some(true), Some(false), None] { let cached = BTreeMap::from([( "example-tool".into(), diff --git a/ci/crates/render/src/lib.rs b/ci/crates/render/src/lib.rs index 37afa8f50a..180a0ac343 100644 --- a/ci/crates/render/src/lib.rs +++ b/ci/crates/render/src/lib.rs @@ -4,9 +4,9 @@ use std::collections::BTreeMap; use types::{Api, ApiEntry, Catalog, Collection, Entry, Tag, Type}; mod deprecation; -mod lints; pub mod stats; pub mod types; +mod validated; pub use deprecation::check_deprecated; @@ -95,7 +95,7 @@ pub fn create_api(entries: Vec, languages: &[Tag], other_tags: &[Tag]) -> let key = slugify(&entry.name); let api_entry = ApiEntry { - name: entry.name, + name: entry.name.into(), categories: entry.categories, languages: entry_languages, other: entry_other, @@ -152,11 +152,11 @@ mod tests { } } - fn entry(tags: &[Tag]) -> Entry { - Entry { - name: "Multi Tool".into(), + fn entry(tags: &[Tag]) -> Result { + Ok(Entry { + name: String::from("Multi Tool").try_into()?, categories: BTreeSet::new(), - tags: tags.iter().cloned().collect(), + tags: tags.iter().cloned().collect::>().try_into()?, license: "MIT".into(), types: BTreeSet::new(), homepage: "https://example.com".into(), @@ -170,15 +170,16 @@ mod tests { reviews: None, demos: None, wrapper: None, - } + }) } #[test] fn deprecated_tools_are_collapsed_in_every_section() -> Result<()> { - let mut active = entry(&[]); - active.name = "Active Tool".into(); - let mut deprecated = entry(&[]); - deprecated.name = "Deprecated Tool".into(); + let tags = [tag("Rust", "rust", Type::Language)]; + let mut active = entry(&tags)?; + active.name = String::from("Active Tool").try_into()?; + let mut deprecated = entry(&tags)?; + deprecated.name = String::from("Deprecated Tool").try_into()?; deprecated.deprecated = Some(true); deprecated.license = "proprietary".into(); deprecated.discussion = Some("https://example.com/discussion".into()); @@ -232,9 +233,10 @@ mod tests { #[test] fn no_empty_deprecated_sections_are_rendered() -> Result<()> { - let mut explicitly_active = entry(&[]); + let tags = [tag("Rust", "rust", Type::Language)]; + let mut explicitly_active = entry(&tags)?; explicitly_active.deprecated = Some(false); - for tools in [vec![], vec![entry(&[])], vec![explicitly_active]] { + for tools in [vec![], vec![entry(&tags)?], vec![explicitly_active]] { let markdown = Catalog { linters: BTreeMap::new(), others: BTreeMap::new(), @@ -249,7 +251,7 @@ mod tests { #[test] fn deprecated_only_sections_keep_their_entries() -> Result<()> { - let mut tool = entry(&[]); + let mut tool = entry(&[tag("Rust", "rust", Type::Language)])?; tool.deprecated = Some(true); let markdown = Catalog { linters: BTreeMap::new(), @@ -283,12 +285,12 @@ mod tests { } #[test] - fn multi_language_tools_remain_visible_in_other_sections_and_api() { + fn multi_language_tools_remain_visible_in_other_sections_and_api() -> Result<()> { let python = tag("Python", "python", Type::Language); let rust = tag("Rust", "rust", Type::Language); let mut ai_generated = tag("AI-generated code", "ai-generated-code", Type::Other); ai_generated.include_multi = true; - let tool = entry(&[python.clone(), rust.clone(), ai_generated.clone()]); + let tool = entry(&[python.clone(), rust.clone(), ai_generated.clone()])?; let languages = [python, rust]; let other_tags = [ai_generated.clone()]; @@ -303,14 +305,15 @@ mod tests { let api = create_api(vec![tool], &languages, &other_tags); assert_eq!(api["multi-tool"].languages, ["python", "rust"]); assert_eq!(api["multi-tool"].other, ["ai-generated-code"]); + Ok(()) } #[test] - fn c_and_cpp_tools_stay_in_language_sections_when_they_have_other_tags() { + fn c_and_cpp_tools_stay_in_language_sections_when_they_have_other_tags() -> Result<()> { let c = tag("C", "c", Type::Language); let cpp = tag("C++", "cpp", Type::Language); let security = tag("Security/SAST", "security", Type::Other); - let tool = entry(&[c.clone(), cpp.clone(), security.clone()]); + let tool = entry(&[c.clone(), cpp.clone(), security.clone()])?; let catalog = create_catalog( std::slice::from_ref(&tool), @@ -326,5 +329,6 @@ mod tests { assert_eq!(catalog.linters[&cpp][0], tool); assert_eq!(catalog.others[&security].len(), 1); assert_eq!(catalog.others[&security][0], tool); + Ok(()) } } diff --git a/ci/crates/render/src/lints.rs b/ci/crates/render/src/lints.rs deleted file mode 100644 index 08376c29c4..0000000000 --- a/ci/crates/render/src/lints.rs +++ /dev/null @@ -1,27 +0,0 @@ -use anyhow::{Result, ensure}; - -use crate::types::{ParsedEntry, Tag}; - -pub fn validate(entry: &ParsedEntry, tags: &[Tag]) -> Result<()> { - name(entry, tags)?; - min_one_tag(entry, tags) -} - -pub fn name(entry: &ParsedEntry, _: &[Tag]) -> Result<()> { - ensure!( - entry.name.len() <= 50, - "Name of entry may be at most 50 characters long, but {} is {} long", - entry.name, - entry.name.len() - ); - Ok(()) -} - -pub fn min_one_tag(entry: &ParsedEntry, _: &[Tag]) -> Result<()> { - ensure!( - !entry.tags.is_empty(), - "{} must have at least one tag from `tags.yml`.", - entry.name - ); - Ok(()) -} diff --git a/ci/crates/render/src/regression_tests.rs b/ci/crates/render/src/regression_tests.rs index 8dd6209913..5969602f8d 100644 --- a/ci/crates/render/src/regression_tests.rs +++ b/ci/crates/render/src/regression_tests.rs @@ -1,6 +1,6 @@ use anyhow::{Context, Result}; use serde_json::json; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use crate::types::{Catalog, Entry, ParsedEntry, Tag, ToolType, Type}; use crate::{create_api, create_catalog, format_stats, stats}; @@ -33,7 +33,7 @@ fn normalization_preserves_fields_and_uses_the_first_matching_tag() -> Result<() let mut duplicate = rust.clone(); duplicate.name = "Different metadata".into(); let normalized = Entry::from_parsed(original.clone(), &[rust.clone(), duplicate])?; - assert_eq!(normalized.tags, [rust].into()); + assert_eq!(normalized.tags.iter().collect::>(), [&rust]); assert_eq!(normalized.types, [ToolType::Commandline].into()); let mut expected = serde_json::to_value(original)?; expected["tags"] = serde_json::to_value(&normalized.tags)?; @@ -44,7 +44,8 @@ fn normalization_preserves_fields_and_uses_the_first_matching_tag() -> Result<() #[test] fn unknown_tags_are_reported_together_before_invalid_tool_types() -> Result<()> { let mut tool = parsed()?; - tool.tags = ["z-unknown".into(), "a-unknown".into(), "rust".into()].into(); + tool.tags = + BTreeSet::from(["z-unknown".into(), "a-unknown".into(), "rust".into()]).try_into()?; tool.types = ["invalid".into()].into(); let error = Entry::from_parsed(tool, &[tag("rust", Type::Language)]) .err() @@ -79,34 +80,69 @@ fn tool_type_deserialization_matches_the_previous_json_conversion() -> Result<() } #[test] -fn validation_preserves_byte_length_limit_and_error_precedence() -> Result<()> { - let tags = [tag("rust", Type::Language)]; - for name in ["a".repeat(50), "é".repeat(25), String::new()] { - let mut tool = parsed()?; - tool.name = name; - Entry::from_parsed(tool, &tags)?; +fn parsed_and_normalized_deserialization_enforce_invariants() -> Result<()> { + let parsed = serde_json::to_value(parsed()?)?; + let normalized = serde_json::to_value(Entry::from_parsed( + serde_json::from_value(parsed.clone())?, + &[tag("rust", Type::Language)], + )?)?; + for (original, is_normalized) in [(parsed, false), (normalized, true)] { + let rejects = |value| { + if is_normalized { + serde_json::from_value::(value).is_err() + } else { + serde_json::from_value::(value).is_err() + } + }; + for name in [ + String::new(), + " \t\n\u{2003}".into(), + "a".repeat(51), + "é".repeat(26), + ] { + let mut invalid = original.clone(); + invalid["name"] = json!(name); + assert!(rejects(invalid)); + } + for name in ["a".repeat(50), "é".repeat(25), " Astrée ".into()] { + let mut valid = original.clone(); + valid["name"] = json!(name); + let encoded = if is_normalized { + serde_json::to_value(serde_json::from_value::(valid.clone())?)? + } else { + serde_json::to_value(serde_json::from_value::(valid.clone())?)? + }; + assert_eq!(encoded, valid); + } + let mut invalid = original.clone(); + invalid["tags"] = json!([]); + assert!(rejects(invalid)); + for field in ["name", "tags"] { + let mut invalid = original.clone(); + invalid[field] = serde_json::Value::Null; + assert!(rejects(invalid)); + } } - let mut tool = parsed()?; - tool.name = "é".repeat(26); - tool.tags.clear(); - assert_eq!( - Entry::from_parsed(tool.clone(), &tags) - .err() - .context("Names over 50 bytes should be rejected")? - .to_string(), - format!( - "Name of entry may be at most 50 characters long, but {} is 52 long", - tool.name - ) - ); - tool.name = "Example Tool".into(); - assert_eq!( - Entry::from_parsed(tool, &tags) - .err() - .context("Empty tags should be rejected")? - .to_string(), - "Example Tool must have at least one tag from `tags.yml`." - ); + Ok(()) +} + +#[test] +fn normalized_tags_deserialize_as_a_sorted_nonempty_set() -> Result<()> { + let rust = tag("rust", Type::Language); + let cpp = tag("cpp", Type::Language); + let entry = Entry::from_parsed(parsed()?, std::slice::from_ref(&rust))?; + let mut value = serde_json::to_value(entry)?; + value["tags"] = json!([rust, cpp, rust]); + let encoded = serde_json::to_string(&value)?; + for entry in [ + serde_json::from_str::(&encoded)?, + serde_saphyr::from_str::(&encoded)?, + ] { + assert_eq!(entry.tags.iter().collect::>(), [&cpp, &rust]); + assert_eq!(serde_json::to_value(entry.tags)?, json!([cpp, rust])); + } + value["tags"] = json!([]); + assert!(serde_saphyr::from_str::(&serde_json::to_string(&value)?).is_err()); Ok(()) } @@ -116,7 +152,7 @@ fn api_preserves_configured_tag_order_duplicates_and_all_fields() -> Result<()> let python = tag("python", Type::Language); let security = tag("security", Type::Other); let mut raw = parsed()?; - raw.tags = ["rust".into(), "python".into(), "security".into()].into(); + raw.tags = BTreeSet::from(["rust".into(), "python".into(), "security".into()]).try_into()?; raw.source = Some("https://github.com/owner/repo".into()); raw.pricing = Some("https://example.com/pricing".into()); raw.plans = Some(BTreeMap::from([("free".into(), true)])); @@ -159,7 +195,7 @@ fn api_preserves_configured_tag_order_duplicates_and_all_fields() -> Result<()> fn api_slug_collisions_keep_the_last_entry_and_missing_fields_stay_null() -> Result<()> { let tool = Entry::from_parsed(parsed()?, &[tag("rust", Type::Language)])?; let mut replacement = tool.clone(); - replacement.name = "Example-Tool".into(); + replacement.name = String::from("Example-Tool").try_into()?; let api = create_api(vec![tool, replacement], &[], &[]); assert_eq!(api.len(), 1); assert_eq!(api["example-tool"].name, "Example-Tool"); @@ -197,11 +233,17 @@ fn catalog_preserves_input_order_and_omits_empty_sections() -> Result<()> { inclusive.clone(), ]; let mut raw = parsed()?; - raw.tags = ["rust".into(), "regular".into(), "inclusive".into()].into(); - raw.name = "Z Single".into(); + raw.tags = BTreeSet::from(["rust".into(), "regular".into(), "inclusive".into()]).try_into()?; + raw.name = String::from("Z Single").try_into()?; let single = Entry::from_parsed(raw.clone(), &tags)?; - raw.tags.insert("python".into()); - raw.name = "A Multi".into(); + raw.tags = raw + .tags + .iter() + .cloned() + .chain(["python".into()]) + .collect::>() + .try_into()?; + raw.name = String::from("A Multi").try_into()?; let multi = Entry::from_parsed(raw, &tags)?; let tools = [single.clone(), multi.clone()]; let catalog = create_catalog( diff --git a/ci/crates/render/src/types.rs b/ci/crates/render/src/types.rs index acbef40cf8..9ae47a6e74 100644 --- a/ci/crates/render/src/types.rs +++ b/ci/crates/render/src/types.rs @@ -4,7 +4,7 @@ use serde::{Deserialize, Serialize, de::value::StrDeserializer}; use std::cmp::Ordering; use std::collections::{BTreeMap, BTreeSet}; -use crate::lints; +pub use crate::validated::{EntryName, EntryTags}; #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash, Ord, PartialOrd)] pub enum Type { @@ -29,8 +29,6 @@ pub struct Tag { // `BTreeSet` so renders preserve the configured order. pub type Tags = Vec; -pub type EntryTags = BTreeSet; - #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct Resource { title: String, @@ -62,9 +60,9 @@ pub enum Category { #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct ParsedEntry { - pub name: String, + pub name: EntryName, pub categories: BTreeSet, - pub tags: BTreeSet, + pub tags: EntryTags, pub license: String, pub types: BTreeSet, pub homepage: String, @@ -94,9 +92,9 @@ pub enum ToolType { #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct Entry { - pub name: String, + pub name: EntryName, pub categories: BTreeSet, - pub tags: BTreeSet, + pub tags: EntryTags, pub license: String, pub types: BTreeSet, pub homepage: String, @@ -142,8 +140,6 @@ impl Entry { /// Returns an error when the entry fails validation or references an /// unknown tag or tool type. pub fn from_parsed(p: ParsedEntry, tags: &[Tag]) -> Result { - lints::validate(&p, tags)?; - let mut entry_tags = BTreeSet::new(); let mut tag_errors = Vec::new(); for value in &p.tags { @@ -170,7 +166,7 @@ impl Entry { Ok(Self { name: p.name, categories: p.categories, - tags: entry_tags, + tags: entry_tags.try_into()?, license: p.license, types, homepage: p.homepage, diff --git a/ci/crates/render/src/validated.rs b/ci/crates/render/src/validated.rs new file mode 100644 index 0000000000..4d8ace879b --- /dev/null +++ b/ci/crates/render/src/validated.rs @@ -0,0 +1,165 @@ +use anyhow::{Result, ensure}; +use serde::{Deserialize, Serialize}; +use std::collections::{BTreeSet, btree_set}; +use std::fmt; +use std::ops::Deref; + +/// A nonblank entry name of at most 50 UTF-8 bytes, stored without trimming. +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] +#[serde(try_from = "String")] +pub struct EntryName(String); + +impl TryFrom for EntryName { + type Error = anyhow::Error; + + fn try_from(name: String) -> Result { + ensure!(!name.trim().is_empty(), "Name of entry must not be blank"); + ensure!( + name.len() <= 50, + "Name of entry may be at most 50 UTF-8 bytes long, but {} is {} bytes long", + name, + name.len() + ); + Ok(Self(name)) + } +} + +impl AsRef for EntryName { + fn as_ref(&self) -> &str { + &self.0 + } +} + +impl Deref for EntryName { + type Target = str; + + fn deref(&self) -> &Self::Target { + self.as_ref() + } +} + +impl fmt::Display for EntryName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.as_ref()) + } +} + +impl From for String { + fn from(name: EntryName) -> Self { + name.0 + } +} + +/// A nonempty, sorted set of entry tags, with no mutable access to its contents. +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] +#[serde(try_from = "BTreeSet")] +pub struct EntryTags(BTreeSet); + +impl TryFrom> for EntryTags { + type Error = anyhow::Error; + + fn try_from(tags: BTreeSet) -> Result { + ensure!( + !tags.is_empty(), + "Entry must have at least one tag from `tags.yml`." + ); + Ok(Self(tags)) + } +} + +impl EntryTags { + /// Iterates over the tags in sorted order. + pub fn iter(&self) -> btree_set::Iter<'_, T> { + self.0.iter() + } + + /// Whether this entry has the given tag. + #[must_use] + pub fn contains(&self, tag: &T) -> bool { + self.0.contains(tag) + } +} + +impl<'a, T: Ord> IntoIterator for &'a EntryTags { + type Item = &'a T; + type IntoIter = btree_set::Iter<'a, T>; + + fn into_iter(self) -> Self::IntoIter { + self.iter() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + #[test] + fn name_construction_and_serde_reject_blank_and_overlong_names() -> Result<()> { + for name in [ + String::new(), + " \t\r\n".into(), + "\u{2003}\u{a0}".into(), + "a".repeat(51), + "é".repeat(26), + ] { + assert!(EntryName::try_from(name.clone()).is_err()); + assert!(serde_json::from_value::(json!(name)).is_err()); + assert!(serde_saphyr::from_str::(&serde_json::to_string(&name)?).is_err()); + } + Ok(()) + } + + #[test] + fn names_preserve_whitespace_and_utf8_bytes_through_serde() -> Result<()> { + for original in ["a".repeat(50), "é".repeat(25), " Astrée \t".into()] { + let name = EntryName::try_from(original.clone())?; + assert_eq!(name.as_ref(), original); + assert_eq!(name.to_string(), original); + let encoded = serde_json::to_string(&name)?; + assert_eq!(serde_json::from_str::(&encoded)?, name); + assert_eq!(serde_saphyr::from_str::(&encoded)?, name); + assert_eq!(serde_json::to_value(&name)?, json!(original)); + assert_eq!(String::from(name), original); + } + Ok(()) + } + + #[test] + fn tags_reject_empty_sets_in_constructors_and_serde() { + assert!(EntryTags::::try_from(BTreeSet::new()).is_err()); + assert!(serde_json::from_str::("[]").is_err()); + assert!(serde_saphyr::from_str::("[]").is_err()); + } + + #[test] + fn tags_are_sorted_and_deduplicated_in_constructors_and_serde() -> Result<()> { + let tags = EntryTags::try_from(BTreeSet::from([ + String::from("rust"), + String::from("cpp"), + String::from("rust"), + ]))?; + assert!(tags.contains(&String::from("rust"))); + assert!(!tags.contains(&String::from("python"))); + assert_eq!( + tags.iter().map(String::as_str).collect::>(), + ["cpp", "rust"] + ); + assert_eq!(serde_json::to_value(&tags)?, json!(["cpp", "rust"])); + assert_eq!( + serde_json::from_value::(json!(["rust", "cpp", "rust"]))?, + tags + ); + assert_eq!( + serde_saphyr::from_str::("[rust, cpp, rust]")?, + tags + ); + assert_eq!( + serde_json::from_value::(json!(["rust", "rust"]))? + .iter() + .count(), + 1 + ); + Ok(()) + } +} From bc487438fccc983dce3150a255bf962929ba12ed Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:25:35 +0200 Subject: [PATCH 09/13] Move renderer regression coverage to integration tests --- ci/crates/render/src/lib.rs | 3 --- .../render/{src/regression_tests.rs => tests/regression.rs} | 4 ++-- 2 files changed, 2 insertions(+), 5 deletions(-) rename ci/crates/render/{src/regression_tests.rs => tests/regression.rs} (98%) diff --git a/ci/crates/render/src/lib.rs b/ci/crates/render/src/lib.rs index 180a0ac343..d33045fc08 100644 --- a/ci/crates/render/src/lib.rs +++ b/ci/crates/render/src/lib.rs @@ -10,9 +10,6 @@ mod validated; pub use deprecation::check_deprecated; -#[cfg(test)] -mod regression_tests; - /// Groups normalized entries for the generated README. #[must_use] pub fn create_catalog( diff --git a/ci/crates/render/src/regression_tests.rs b/ci/crates/render/tests/regression.rs similarity index 98% rename from ci/crates/render/src/regression_tests.rs rename to ci/crates/render/tests/regression.rs index 5969602f8d..b097b98047 100644 --- a/ci/crates/render/src/regression_tests.rs +++ b/ci/crates/render/tests/regression.rs @@ -2,8 +2,8 @@ use anyhow::{Context, Result}; use serde_json::json; use std::collections::{BTreeMap, BTreeSet}; -use crate::types::{Catalog, Entry, ParsedEntry, Tag, ToolType, Type}; -use crate::{create_api, create_catalog, format_stats, stats}; +use render::types::{Catalog, Entry, ParsedEntry, Tag, ToolType, Type}; +use render::{create_api, create_catalog, format_stats, stats}; fn tag(value: &str, kind: Type) -> Tag { Tag { From c7211fbed6f87a55b3d3ac830b468b5a82ff474e Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:33:47 +0200 Subject: [PATCH 10/13] Parse catalog paths into ToolPath and load entries through ToolEntry --- ci/crates/pr-check/src/criteria.rs | 10 +-- ci/crates/pr-check/src/input.rs | 117 +++++++++++++++++++++++++++++ ci/crates/pr-check/src/main.rs | 79 +++---------------- ci/crates/pr-check/src/network.rs | 3 +- ci/crates/pr-check/tests/cli.rs | 26 +++++++ 5 files changed, 155 insertions(+), 80 deletions(-) create mode 100644 ci/crates/pr-check/src/input.rs diff --git a/ci/crates/pr-check/src/criteria.rs b/ci/crates/pr-check/src/criteria.rs index 12a7bcc7b8..50e0e8728a 100644 --- a/ci/crates/pr-check/src/criteria.rs +++ b/ci/crates/pr-check/src/criteria.rs @@ -5,17 +5,9 @@ use chrono::{DateTime, Utc}; use serde::Deserialize; use crate::checks::{Check, Contributors, RepositoryAge, Stars}; +use crate::input::ToolEntry; use crate::report::ToolReport; -/// A minimal tool entry parsed from `data/tools/.yml`. -/// Only the fields needed for the contributing criteria check are required. -#[derive(Debug, Deserialize)] -pub struct ToolEntry { - pub name: String, - pub source: Option, - pub homepage: Option, -} - /// Response from `GET /repos/{owner}/{repo}`. #[derive(Debug, Deserialize)] pub struct RepoInfo { diff --git a/ci/crates/pr-check/src/input.rs b/ci/crates/pr-check/src/input.rs new file mode 100644 index 0000000000..af5d2074fe --- /dev/null +++ b/ci/crates/pr-check/src/input.rs @@ -0,0 +1,117 @@ +//! Catalog input paths and tool loading. + +use anyhow::{Context, Result, ensure}; +use serde::Deserialize; +use std::fmt; +use std::path::{Component, PathBuf}; + +/// A relative YAML path under `data/tools`, with no parent-directory traversal. +/// This classifies a catalog path; it does not resolve symlinks or guarantee existence. +#[derive(Debug)] +pub struct ToolPath(PathBuf); + +impl TryFrom for ToolPath { + type Error = anyhow::Error; + + fn try_from(path: PathBuf) -> Result { + ensure!( + path.starts_with("data/tools") + && !path + .components() + .any(|component| component == Component::ParentDir) + && matches!( + path.extension().and_then(|ext| ext.to_str()), + Some("yml" | "yaml") + ), + "Expected a relative YAML path under data/tools: {}", + path.display() + ); + Ok(Self(path)) + } +} + +impl fmt::Display for ToolPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.display().fmt(f) + } +} + +/// The tool fields required by the contribution checks. +#[derive(Debug, Deserialize)] +pub struct ToolEntry { + pub name: String, + pub source: Option, + pub homepage: Option, +} + +impl ToolEntry { + /// Reads a catalog entry relative to the current working directory. + /// + /// # Errors + /// + /// Returns an error if the file cannot be opened or parsed. + pub fn read(path: &ToolPath) -> Result { + let file = std::fs::File::open(&path.0).with_context(|| format!("Cannot open {path}"))?; + serde_saphyr::from_reader(file).with_context(|| format!("Cannot parse {path}")) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + #[test] + fn parses_only_catalog_yaml_paths() -> Result<()> { + for path in [ + "data/tools/tool.yml", + "data/tools/tool.yaml", + "data/tools/nested/tool.yml", + ] { + let parsed = ToolPath::try_from(PathBuf::from(path))?; + assert_eq!(parsed.to_string(), path); + } + for path in [ + "README.md", + "data/tags.yml", + "data/tools-extra/tool.yml", + "data/tools/tool.YML", + "data/tools/tool.json", + "/data/tools/tool.yml", + "data/tools/../outside.yml", + "data/tools/nested/../../outside.yml", + ] { + assert!(ToolPath::try_from(PathBuf::from(path)).is_err(), "{path}"); + } + Ok(()) + } + + #[test] + fn parses_catalog() -> Result<()> { + let tools = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../data/tools"); + let mut count = 0; + for entry in std::fs::read_dir(tools)? { + let entry = entry?; + let relative = Path::new("data/tools").join(entry.file_name()); + if ToolPath::try_from(relative).is_ok() { + let tool: ToolEntry = + serde_saphyr::from_reader(std::fs::File::open(entry.path())?)?; + assert!(!tool.name.is_empty(), "{}", entry.path().display()); + count += 1; + } + } + assert!(count > 0); + Ok(()) + } + + #[test] + fn tool_yaml_keeps_optional_fields_and_ignores_unrelated_metadata() -> Result<()> { + let tool: ToolEntry = + serde_saphyr::from_str("name: Example\nlicense: MIT\ntags: [rust]\n")?; + assert_eq!(tool.name, "Example"); + assert!(tool.source.is_none()); + assert!(tool.homepage.is_none()); + assert!(serde_saphyr::from_str::("source: https://example.com").is_err()); + Ok(()) + } +} diff --git a/ci/crates/pr-check/src/main.rs b/ci/crates/pr-check/src/main.rs index c29dcf091f..e8ddb59d0a 100644 --- a/ci/crates/pr-check/src/main.rs +++ b/ci/crates/pr-check/src/main.rs @@ -23,16 +23,17 @@ mod checks; mod criteria; +mod input; mod network; mod report; use anyhow::{Context, Result}; use clap::Parser; use std::env; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::process::ExitCode; -use criteria::ToolEntry; +use input::{ToolEntry, ToolPath}; use network::{GithubClient, check_tool}; use report::{Comment, report_exit_code}; @@ -44,24 +45,6 @@ struct Args { files: Vec, } -/// Reads and deserialises a single tool YAML file. -/// -/// # Errors -/// -/// Returns an error if the file cannot be read or parsed. -fn read_tool(path: &Path) -> Result { - let f = std::fs::File::open(path).with_context(|| format!("Cannot open {}", path.display()))?; - serde_saphyr::from_reader(f).with_context(|| format!("Cannot parse {}", path.display())) -} - -fn is_tool_path(path: &Path) -> bool { - path.starts_with("data/tools") - && matches!( - path.extension().and_then(|extension| extension.to_str()), - Some("yml" | "yaml") - ) -} - #[tokio::main(flavor = "current_thread")] async fn main() -> Result { let args = match Args::try_parse() { @@ -78,8 +61,12 @@ async fn main() -> Result { let client = GithubClient::new(token)?; let mut reports = Vec::new(); - for path in args.files.iter().filter(|path| is_tool_path(path)) { - let tool = read_tool(path).with_context(|| format!("Failed to read {}", path.display()))?; + for path in args + .files + .into_iter() + .filter_map(|path| ToolPath::try_from(path).ok()) + { + let tool = ToolEntry::read(&path)?; eprintln!("Checking '{}'...", tool.name); let report = check_tool(&client, &tool).await?; reports.push(report); @@ -139,52 +126,4 @@ mod tests { } Ok(()) } - - #[test] - fn parses_catalog() -> Result<()> { - let tools = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../data/tools"); - let mut count = 0; - for entry in std::fs::read_dir(tools)? { - let path = entry?.path(); - if path.extension().is_some_and(|ext| ext == "yml") { - let tool = read_tool(&path)?; - assert!(!tool.name.is_empty(), "{}", path.display()); - count += 1; - } - } - assert!(count > 0); - Ok(()) - } - - #[test] - fn tool_path_filter_accepts_only_catalog_yaml_arguments() { - for path in [ - "data/tools/tool.yml", - "data/tools/tool.yaml", - "data/tools/nested/tool.yml", - ] { - assert!(is_tool_path(Path::new(path)), "{path}"); - } - for path in [ - "README.md", - "data/tags.yml", - "data/tools-extra/tool.yml", - "data/tools/tool.YML", - "data/tools/tool.json", - "/data/tools/tool.yml", - ] { - assert!(!is_tool_path(Path::new(path)), "{path}"); - } - } - - #[test] - fn tool_yaml_keeps_optional_fields_and_ignores_unrelated_metadata() -> Result<()> { - let tool: ToolEntry = - serde_saphyr::from_str("name: Example\nlicense: MIT\ntags: [rust]\n")?; - assert_eq!(tool.name, "Example"); - assert!(tool.source.is_none()); - assert!(tool.homepage.is_none()); - assert!(serde_saphyr::from_str::("source: https://example.com").is_err()); - Ok(()) - } } diff --git a/ci/crates/pr-check/src/network.rs b/ci/crates/pr-check/src/network.rs index 91f22b5920..46695043f2 100644 --- a/ci/crates/pr-check/src/network.rs +++ b/ci/crates/pr-check/src/network.rs @@ -6,7 +6,8 @@ use github_repo::GithubRepo; use serde::{Deserialize, de::DeserializeOwned}; use crate::checks::{Check, DomainAge}; -use crate::criteria::{Contributor, RepoInfo, ToolEntry, homepage_domain, repository_report}; +use crate::criteria::{Contributor, RepoInfo, homepage_domain, repository_report}; +use crate::input::ToolEntry; use crate::report::{CheckResult, ToolReport}; pub struct GithubClient { diff --git a/ci/crates/pr-check/tests/cli.rs b/ci/crates/pr-check/tests/cli.rs index 5c3704b555..2e5e111ce4 100644 --- a/ci/crates/pr-check/tests/cli.rs +++ b/ci/crates/pr-check/tests/cli.rs @@ -1,5 +1,31 @@ use std::process::Command; +#[test] +fn unrelated_and_traversing_paths_are_not_loaded() -> std::io::Result<()> { + let output = Command::new(env!("CARGO_BIN_EXE_pr-check")) + .args(["README.md", "data/tools/../outside.yml"]) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .env("GITHUB_TOKEN", "unused-test-token") + .env_remove("COMMENT_OUTPUT_FILE") + .output()?; + assert!(output.status.success()); + assert!(String::from_utf8_lossy(&output.stdout).contains("Nothing to check")); + Ok(()) +} + +#[test] +fn missing_catalog_file_reports_its_path_without_closing() -> std::io::Result<()> { + let output = Command::new(env!("CARGO_BIN_EXE_pr-check")) + .arg("data/tools/missing.yml") + .current_dir(env!("CARGO_MANIFEST_DIR")) + .env("GITHUB_TOKEN", "unused-test-token") + .env_remove("COMMENT_OUTPUT_FILE") + .output()?; + assert_eq!(output.status.code(), Some(1)); + assert!(String::from_utf8_lossy(&output.stderr).contains("Cannot open data/tools/missing.yml")); + Ok(()) +} + #[test] fn help_and_version_do_not_require_credentials() -> std::io::Result<()> { for (flag, expected) in [("--help", "Usage:"), ("--version", "pr-check")] { From 623def87a109d90ce1f0dd594a813abaea07476c Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:39:27 +0200 Subject: [PATCH 11/13] Collect tool outcomes into a Reports type --- ci/Cargo.lock | 1 + ci/Cargo.toml | 1 + ci/crates/pr-check/Cargo.toml | 1 + ci/crates/pr-check/src/main.rs | 26 +++---- ci/crates/pr-check/src/report.rs | 115 ++++++++++++++++++++++--------- 5 files changed, 100 insertions(+), 44 deletions(-) diff --git a/ci/Cargo.lock b/ci/Cargo.lock index 43017138ce..b69c1deb66 100644 --- a/ci/Cargo.lock +++ b/ci/Cargo.lock @@ -968,6 +968,7 @@ dependencies = [ "askama", "chrono", "clap", + "futures-util", "github-repo", "reqwest", "serde", diff --git a/ci/Cargo.toml b/ci/Cargo.toml index 6612f3f0e4..098363c98a 100644 --- a/ci/Cargo.toml +++ b/ci/Cargo.toml @@ -15,6 +15,7 @@ askama = "0.16" chrono = { version = "0.4.45", features = ["serde"] } clap = { version = "4.6.7", features = ["derive"] } github-repo = { path = "crates/github-repo" } +futures-util = { version = "0.3.34", default-features = false, features = ["std"] } reqwest = { version = "0.13.5", default-features = false, features = ["json", "rustls", "system-proxy"] } serde = { version = "1.0.229", features = ["derive"] } serde_json = "1.0.151" diff --git a/ci/crates/pr-check/Cargo.toml b/ci/crates/pr-check/Cargo.toml index ff1a48e61a..589b5f2067 100644 --- a/ci/crates/pr-check/Cargo.toml +++ b/ci/crates/pr-check/Cargo.toml @@ -17,6 +17,7 @@ askama.workspace = true chrono.workspace = true clap.workspace = true github-repo.workspace = true +futures-util.workspace = true reqwest.workspace = true serde.workspace = true serde-saphyr.workspace = true diff --git a/ci/crates/pr-check/src/main.rs b/ci/crates/pr-check/src/main.rs index e8ddb59d0a..3893d190d7 100644 --- a/ci/crates/pr-check/src/main.rs +++ b/ci/crates/pr-check/src/main.rs @@ -29,13 +29,14 @@ mod report; use anyhow::{Context, Result}; use clap::Parser; +use futures_util::{StreamExt, TryStreamExt, stream}; use std::env; use std::path::PathBuf; use std::process::ExitCode; use input::{ToolEntry, ToolPath}; use network::{GithubClient, check_tool}; -use report::{Comment, report_exit_code}; +use report::{Comment, Reports}; #[derive(Debug, Parser)] #[command(version, about)] @@ -60,19 +61,20 @@ async fn main() -> Result { let client = GithubClient::new(token)?; - let mut reports = Vec::new(); - for path in args + let paths = args .files .into_iter() - .filter_map(|path| ToolPath::try_from(path).ok()) - { - let tool = ToolEntry::read(&path)?; - eprintln!("Checking '{}'...", tool.name); - let report = check_tool(&client, &tool).await?; - reports.push(report); - } + .filter_map(|path| ToolPath::try_from(path).ok()); + let reports: Reports = stream::iter(paths) + .then(async |path| { + let tool = ToolEntry::read(&path)?; + eprintln!("Checking '{}'...", tool.name); + check_tool(&client, &tool).await + }) + .try_collect() + .await?; - let comment = Comment::from(reports.as_slice()); + let comment = Comment::from(&reports); if let Some(output_file) = env::var("COMMENT_OUTPUT_FILE") .ok() @@ -89,7 +91,7 @@ async fn main() -> Result { println!("{comment}"); } - let exit_code = report_exit_code(&reports); + let exit_code = reports.exit_code(); if exit_code != 0 { eprintln!( "One or more tools failed or require manual review of the contributing criteria." diff --git a/ci/crates/pr-check/src/report.rs b/ci/crates/pr-check/src/report.rs index 86c642edae..7306d0ad85 100644 --- a/ci/crates/pr-check/src/report.rs +++ b/ci/crates/pr-check/src/report.rs @@ -72,6 +72,32 @@ impl ToolReport { } } +/// The ordered outcomes of all tool checks. +#[derive(Debug, Default)] +pub struct Reports(Vec); + +impl Reports { + pub fn exit_code(&self) -> u8 { + if self.0.iter().any(ToolReport::should_close) { + 2 + } else { + u8::from(self.0.iter().any(ToolReport::has_nonpassing_checks)) + } + } +} + +impl Extend for Reports { + fn extend>(&mut self, iter: T) { + self.0.extend(iter); + } +} + +impl FromIterator for Reports { + fn from_iter>(iter: T) -> Self { + Self(iter.into_iter().collect()) + } +} + /// A Markdown comment; Askama derives its `Display` implementation. #[derive(Template)] #[template(path = "comment.md")] @@ -82,25 +108,17 @@ pub struct Comment<'a> { should_close: bool, } -impl<'a> From<&'a [ToolReport]> for Comment<'a> { - fn from(reports: &'a [ToolReport]) -> Self { +impl<'a> From<&'a Reports> for Comment<'a> { + fn from(reports: &'a Reports) -> Self { Self { marker: COMMENT_MARKER, - reports, - any_failures: reports.iter().any(ToolReport::has_nonpassing_checks), - should_close: reports.iter().any(ToolReport::should_close), + reports: &reports.0, + any_failures: reports.0.iter().any(ToolReport::has_nonpassing_checks), + should_close: reports.0.iter().any(ToolReport::should_close), } } } -pub fn report_exit_code(reports: &[ToolReport]) -> u8 { - if reports.iter().any(ToolReport::should_close) { - 2 - } else { - u8::from(reports.iter().any(ToolReport::has_nonpassing_checks)) - } -} - #[cfg(test)] mod tests { use super::*; @@ -118,14 +136,46 @@ mod tests { } } + #[test] + fn collect_and_extend_preserve_input_order() { + let report = |name: &str| ToolReport { + name: name.into(), + ..passing_report() + }; + let mut reports: Reports = [report("First"), report("Second")].into_iter().collect(); + reports.extend([report("Third"), report("Fourth")]); + reports.extend([]); + let comment = Comment::from(&reports); + let names: Vec<_> = comment + .reports + .iter() + .map(|report| report.name.as_str()) + .collect(); + assert_eq!(names, ["First", "Second", "Third", "Fourth"]); + assert_eq!(reports.exit_code(), 0); + } + + #[test] + fn empty_reports_have_no_failures() -> Result<()> { + for mut reports in [Reports::default(), std::iter::empty().collect()] { + reports.extend([]); + assert_eq!(reports.exit_code(), 0); + let comment = Comment::from(&reports); + assert!(comment.reports.is_empty()); + assert!(!comment.any_failures); + assert!(!comment.should_close); + assert!(comment.render()?.contains("No new tool files detected")); + } + Ok(()) + } + #[test] fn passing_tools_do_not_close_pr() -> Result<()> { - let reports = [passing_report()]; - assert_eq!(report_exit_code(&reports), 0); - let comment = Comment::from(reports.as_slice()).render()?; + let reports: Reports = std::iter::once(passing_report()).collect(); + assert_eq!(reports.exit_code(), 0); + let comment = Comment::from(&reports).render()?; assert!(comment.contains("All tool eligibility criteria passed")); assert!(!comment.contains("closing this pull request")); - assert_eq!(report_exit_code(&[]), 0); Ok(()) } @@ -140,9 +190,9 @@ mod tests { }; *check = CheckResult::Fail("below minimum".into()); assert_eq!(report.status(), "FAIL"); - let reports = [passing_report(), report]; - assert_eq!(report_exit_code(&reports), 2); - let comment = Comment::from(reports.as_slice()).render()?; + let reports: Reports = [passing_report(), report].into_iter().collect(); + assert_eq!(reports.exit_code(), 2); + let comment = Comment::from(&reports).render()?; assert!(comment.contains("closing this pull request")); assert!(comment.contains("submit a new pull request once all criteria are met")); } @@ -157,9 +207,9 @@ mod tests { report.contributors = CheckResult::Skip(reason.into()); report.age = CheckResult::Skip(reason.into()); assert_eq!(report.status(), "REVIEW"); - let reports = [report]; - assert_eq!(report_exit_code(&reports), 1); - let comment = Comment::from(reports.as_slice()).render()?; + let reports = Reports::from_iter([report]); + assert_eq!(reports.exit_code(), 1); + let comment = Comment::from(&reports).render()?; assert!(comment.contains("needs manual review")); assert!(!comment.contains("closing this pull request")); } @@ -171,7 +221,7 @@ mod tests { let mut report = passing_report(); report.stars = CheckResult::Skip("GitHub API unavailable".into()); report.contributors = CheckResult::Fail("1 contributor".into()); - assert_eq!(report_exit_code(&[report]), 2); + assert_eq!(Reports::from_iter([report]).exit_code(), 2); } #[test] @@ -189,9 +239,9 @@ mod tests { report.age = age; assert!(!report.should_close()); assert_eq!(report.status(), "REVIEW"); - let reports = [report]; - assert_eq!(report_exit_code(&reports), 1); - let comment = Comment::from(reports.as_slice()).render()?; + let reports = Reports::from_iter([report]); + assert_eq!(reports.exit_code(), 1); + let comment = Comment::from(&reports).render()?; assert!(comment.contains("Homepage domain age")); assert!(comment.contains("https://rdap.org/domain/battletest.dev")); assert!(!comment.contains("closing this pull request")); @@ -201,7 +251,8 @@ mod tests { } #[test] fn render_comment_no_files() -> Result<()> { - let template = Comment::from([].as_slice()); + let reports = Reports::from_iter([]); + let template = Comment::from(&reports); let comment = template.render()?; assert_eq!(template.to_string(), comment); assert!(comment.contains("No new tool files detected")); @@ -210,7 +261,7 @@ mod tests { #[test] fn render_comment_contains_marker() -> Result<()> { - let comment = Comment::from([].as_slice()).render()?; + let comment = Comment::from(&Reports::from_iter([])).render()?; assert!(comment.contains(COMMENT_MARKER)); Ok(()) } @@ -246,9 +297,9 @@ mod tests { ("PASS", 0) }; assert_eq!(report.status(), status); - let reports = [passing_report(), report]; - assert_eq!(report_exit_code(&reports), exit); - let template = Comment::from(reports.as_slice()); + let reports: Reports = [passing_report(), report].into_iter().collect(); + assert_eq!(reports.exit_code(), exit); + let template = Comment::from(&reports); let comment = template.render()?; assert_eq!(template.to_string(), comment); assert!(comment.starts_with(COMMENT_MARKER)); From db36f640aeb6e71df8af5d6b193aae7b7e63bb50 Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:40:07 +0200 Subject: [PATCH 12/13] Import filesystem and path names in checker CLI --- ci/crates/pr-check/src/main.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ci/crates/pr-check/src/main.rs b/ci/crates/pr-check/src/main.rs index 3893d190d7..917ceae1cf 100644 --- a/ci/crates/pr-check/src/main.rs +++ b/ci/crates/pr-check/src/main.rs @@ -30,9 +30,9 @@ mod report; use anyhow::{Context, Result}; use clap::Parser; use futures_util::{StreamExt, TryStreamExt, stream}; -use std::env; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::ExitCode; +use std::{env, fs}; use input::{ToolEntry, ToolPath}; use network::{GithubClient, check_tool}; @@ -80,11 +80,11 @@ async fn main() -> Result { .ok() .filter(|s| !s.is_empty()) { - if let Some(parent) = std::path::Path::new(&output_file).parent() { - std::fs::create_dir_all(parent) + if let Some(parent) = Path::new(&output_file).parent() { + fs::create_dir_all(parent) .with_context(|| format!("Failed to create directory for {output_file}"))?; } - std::fs::write(&output_file, comment.to_string()) + fs::write(&output_file, comment.to_string()) .with_context(|| format!("Failed to write comment to {output_file}"))?; eprintln!("Comment written to {output_file}"); } else { From c9db148f94aec49945bfe01920c31db445f1f894 Mon Sep 17 00:00:00 2001 From: Matthias Date: Thu, 17 Sep 2026 18:41:14 +0200 Subject: [PATCH 13/13] Return typed ExitCode values from report outcomes --- ci/crates/pr-check/src/main.rs | 12 ++++++++---- ci/crates/pr-check/src/report.rs | 26 +++++++++++++++----------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/ci/crates/pr-check/src/main.rs b/ci/crates/pr-check/src/main.rs index 917ceae1cf..c4b8521c4b 100644 --- a/ci/crates/pr-check/src/main.rs +++ b/ci/crates/pr-check/src/main.rs @@ -52,9 +52,13 @@ async fn main() -> Result { Ok(args) => args, Err(error) => { // Exit code 2 tells the workflow to close the PR, so CLI errors must use 1. - let code = u8::from(error.use_stderr()); + let code = if error.use_stderr() { + ExitCode::FAILURE + } else { + ExitCode::SUCCESS + }; error.print()?; - return Ok(ExitCode::from(code)); + return Ok(code); } }; let token = env::var("GITHUB_TOKEN").context("GITHUB_TOKEN not set")?; @@ -92,13 +96,13 @@ async fn main() -> Result { } let exit_code = reports.exit_code(); - if exit_code != 0 { + if exit_code != ExitCode::SUCCESS { eprintln!( "One or more tools failed or require manual review of the contributing criteria." ); } - Ok(ExitCode::from(exit_code)) + Ok(exit_code) } #[cfg(test)] diff --git a/ci/crates/pr-check/src/report.rs b/ci/crates/pr-check/src/report.rs index 7306d0ad85..fc40af8a53 100644 --- a/ci/crates/pr-check/src/report.rs +++ b/ci/crates/pr-check/src/report.rs @@ -1,6 +1,7 @@ //! Report status, Markdown rendering, and workflow exit-code contract. use askama::Template; +use std::process::ExitCode; // Identifies the report as output from the contribution checker. const COMMENT_MARKER: &str = ""; @@ -77,11 +78,14 @@ impl ToolReport { pub struct Reports(Vec); impl Reports { - pub fn exit_code(&self) -> u8 { + pub fn exit_code(&self) -> ExitCode { if self.0.iter().any(ToolReport::should_close) { - 2 + // The workflow reserves code 2 for verified failures that close the PR. + ExitCode::from(2) + } else if self.0.iter().any(ToolReport::has_nonpassing_checks) { + ExitCode::FAILURE } else { - u8::from(self.0.iter().any(ToolReport::has_nonpassing_checks)) + ExitCode::SUCCESS } } } @@ -152,14 +156,14 @@ mod tests { .map(|report| report.name.as_str()) .collect(); assert_eq!(names, ["First", "Second", "Third", "Fourth"]); - assert_eq!(reports.exit_code(), 0); + assert_eq!(reports.exit_code(), ExitCode::SUCCESS); } #[test] fn empty_reports_have_no_failures() -> Result<()> { for mut reports in [Reports::default(), std::iter::empty().collect()] { reports.extend([]); - assert_eq!(reports.exit_code(), 0); + assert_eq!(reports.exit_code(), ExitCode::SUCCESS); let comment = Comment::from(&reports); assert!(comment.reports.is_empty()); assert!(!comment.any_failures); @@ -172,7 +176,7 @@ mod tests { #[test] fn passing_tools_do_not_close_pr() -> Result<()> { let reports: Reports = std::iter::once(passing_report()).collect(); - assert_eq!(reports.exit_code(), 0); + assert_eq!(reports.exit_code(), ExitCode::SUCCESS); let comment = Comment::from(&reports).render()?; assert!(comment.contains("All tool eligibility criteria passed")); assert!(!comment.contains("closing this pull request")); @@ -191,7 +195,7 @@ mod tests { *check = CheckResult::Fail("below minimum".into()); assert_eq!(report.status(), "FAIL"); let reports: Reports = [passing_report(), report].into_iter().collect(); - assert_eq!(reports.exit_code(), 2); + assert_eq!(reports.exit_code(), ExitCode::from(2)); let comment = Comment::from(&reports).render()?; assert!(comment.contains("closing this pull request")); assert!(comment.contains("submit a new pull request once all criteria are met")); @@ -208,7 +212,7 @@ mod tests { report.age = CheckResult::Skip(reason.into()); assert_eq!(report.status(), "REVIEW"); let reports = Reports::from_iter([report]); - assert_eq!(reports.exit_code(), 1); + assert_eq!(reports.exit_code(), ExitCode::FAILURE); let comment = Comment::from(&reports).render()?; assert!(comment.contains("needs manual review")); assert!(!comment.contains("closing this pull request")); @@ -221,7 +225,7 @@ mod tests { let mut report = passing_report(); report.stars = CheckResult::Skip("GitHub API unavailable".into()); report.contributors = CheckResult::Fail("1 contributor".into()); - assert_eq!(Reports::from_iter([report]).exit_code(), 2); + assert_eq!(Reports::from_iter([report]).exit_code(), ExitCode::from(2)); } #[test] @@ -240,7 +244,7 @@ mod tests { assert!(!report.should_close()); assert_eq!(report.status(), "REVIEW"); let reports = Reports::from_iter([report]); - assert_eq!(reports.exit_code(), 1); + assert_eq!(reports.exit_code(), ExitCode::FAILURE); let comment = Comment::from(&reports).render()?; assert!(comment.contains("Homepage domain age")); assert!(comment.contains("https://rdap.org/domain/battletest.dev")); @@ -298,7 +302,7 @@ mod tests { }; assert_eq!(report.status(), status); let reports: Reports = [passing_report(), report].into_iter().collect(); - assert_eq!(reports.exit_code(), exit); + assert_eq!(reports.exit_code(), ExitCode::from(exit)); let template = Comment::from(&reports); let comment = template.render()?; assert_eq!(template.to_string(), comment);