From 458f720357ec30115034ab802963904ebf329cec Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 9 Aug 2026 14:27:53 -0500 Subject: [PATCH 1/2] ci: run chat-delegate and web-container tests, guard the gap class The chat-delegate's 40 unit tests never ran in CI. Makefile.toml defines test-chat-delegate and rolls it into `cargo make test`, but no workflow invoked it, so the tests gated only a developer's local run. Same for web-container-contract + web-container-tool (9 tests). Adds a cargo test step for each, plus scripts/check-ci-test-coverage.sh, which fails CI when any [workspace] member has no `cargo test -p ` step in build.yml. That converts a recurring per-crate oversight into a CI failure. Closes #614 Refs #612, freenet/freenet-core#2776 [AI-assisted - Claude] --- .github/workflows/build.yml | 45 +++++++++++++++ scripts/check-ci-test-coverage.sh | 91 +++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+) create mode 100755 scripts/check-ci-test-coverage.sh diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a414d9283..3655efab6 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -100,6 +100,14 @@ jobs: - name: Build Project run: cargo make build + - name: Check every workspace member has a test step + # Guards the CLASS of gap that the per-crate steps below have each closed + # one instance of (freenet/river#614). A crate added to [workspace] + # members without a `cargo test -p ` step here now fails CI, + # instead of shipping tests that quietly never execute — which is what + # the chat-delegate did for its entire existence. + run: ./scripts/check-ci-test-coverage.sh + - name: Test room-contract env: RUST_MIN_STACK: 8388608 @@ -199,6 +207,43 @@ jobs: # windowing Playwright specs would pass CI silently. run: cargo test -p river-ui --bins --features example-data,no-sync + - name: Test chat-delegate (delegate dispatch, CAS envelopes, room subscription) + env: + RUST_MIN_STACK: 8388608 + CARGO_TARGET_DIR: ${{ github.workspace }}/target + # 40 unit tests that had never run in CI (freenet/river#614) — the + # `Makefile.toml` `test-chat-delegate` task existed, but no workflow + # invoked it, so they gated only a developer's local `cargo make test`. + # That mattered more here than for a normal crate: this is where River's + # secret storage and its delegate-re-key migration path live, and the + # delegate is re-keyed roughly weekly (see freenet/river#612). + # + # No `--target x86_64-unknown-linux-gnu` pin, unlike the Makefile task. + # The pin there pairs with `--target-dir target/native` to keep a local + # host build from clobbering the wasm artifacts that the UI pulls in via + # `include_bytes!`. CI has no such conflict — `cargo make build` has + # already produced the wasm, and every test step above likewise builds + # for the host into the same CARGO_TARGET_DIR. The crate's default + # target is only wasm by convention of how it is built, not by a + # `[build] target` setting, so a bare host `cargo test` is correct. + # + # Scope caveat, so this is not over-read: on native targets + # `DelegateCtx`'s `set_secret` is a no-op and `get_secret` always returns + # `None` (freenet-stdlib's stub — see the cfg at handlers.rs:190). These + # tests cover dispatch and pure-value logic, NOT storage round-trips. + run: cargo test -p chat-delegate + + - name: Test web-container-contract + web-container-tool + env: + RUST_MIN_STACK: 8388608 + CARGO_TARGET_DIR: ${{ github.workspace }}/target + # Same gap as the chat-delegate, found while fixing it: 9 tests + # (6 lib + 2 integration + 1 in the tool) that `Makefile.toml` knows how + # to run and no workflow ever did. Includes + # `test_tool_and_contract_compatibility`, which pins that the signing + # tool and the verifying contract agree on the parameter encoding. + run: cargo test -p web-container-contract -p web-container-tool + ui-playwright-tests: runs-on: freenet-default-runner diff --git a/scripts/check-ci-test-coverage.sh b/scripts/check-ci-test-coverage.sh new file mode 100755 index 000000000..c4151d120 --- /dev/null +++ b/scripts/check-ci-test-coverage.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +# +# Fail if any workspace member has no `cargo test` step in build.yml. +# +# Why this exists (freenet/river#614): a test that no CI job runs is +# indistinguishable from a test that passes. This repo has hit that failure at +# least five times — river-ui's unit tests, riverctl's unit tests, the bulk of +# river-core's lib tests, the nine common/tests files that an allowlist of +# `--test ` steps silently skipped, and finally the chat-delegate's 40 +# tests, which never ran from the crate's creation until #614. Each time the +# fix was "add the missing step", which fixes the instance and not the class. +# +# The class fix is this script: adding a workspace member without wiring its +# tests into CI now fails CI. See the long-form comments in +# .github/workflows/build.yml for the individual incidents. +# +# A member with genuinely no tests still needs a step — `cargo test -p ` +# on a testless crate is fast and passes, and it means the day someone adds the +# first test to that crate, it runs. + +set -euo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +workflow="$repo_root/.github/workflows/build.yml" +root_manifest="$repo_root/Cargo.toml" + +[[ -f "$workflow" ]] || { echo "ERROR: $workflow not found" >&2; exit 1; } +[[ -f "$root_manifest" ]] || { echo "ERROR: $root_manifest not found" >&2; exit 1; } + +# `run:` lines that invoke cargo test. Restricting to these means a package +# named only in a build step or a comment does not count as covered. +test_invocations="$(grep -E '^\s*(run:|-)?\s*cargo test ' "$workflow" || true)" + +if [[ -z "$test_invocations" ]]; then + echo "ERROR: build.yml contains no 'cargo test' invocations at all." >&2 + exit 1 +fi + +members="$(sed -n '/^members = \[/,/^]/p' "$root_manifest" \ + | grep -oE '"[^"]+"' | tr -d '"')" + +if [[ -z "$members" ]]; then + echo "ERROR: could not parse [workspace] members from $root_manifest" >&2 + exit 1 +fi + +missing=0 +for member in $members; do + manifest="$repo_root/$member/Cargo.toml" + if [[ ! -f "$manifest" ]]; then + echo "ERROR: workspace member '$member' has no Cargo.toml" >&2 + missing=1 + continue + fi + + pkg="$(grep -m1 -E '^name\s*=' "$manifest" | sed -E 's/^name\s*=\s*"(.*)".*/\1/')" + if [[ -z "$pkg" ]]; then + echo "ERROR: could not read package name from $manifest" >&2 + missing=1 + continue + fi + + # Match `-p ` or `--package `, requiring a word boundary so + # `-p web-container-contract` does not satisfy `web-container-tool`. + if grep -qE "(-p|--package)[= ]$pkg([[:space:]]|$)" <<<"$test_invocations"; then + echo "ok: $pkg ($member)" + else + echo "MISSING: $pkg ($member) has no 'cargo test -p $pkg' step in build.yml" >&2 + missing=1 + fi +done + +if (( missing )); then + cat >&2 <<'EOF' + +Every workspace member must have a `cargo test` step in +.github/workflows/build.yml. Add one next to the existing per-crate steps: + + - name: Test + env: + RUST_MIN_STACK: 8388608 + CARGO_TARGET_DIR: ${{ github.workspace }}/target + run: cargo test -p + +Do not silence this check by deleting the member from the list it reads — +it reads [workspace] members directly, which is the point (freenet/river#614). +EOF + exit 1 +fi + +echo "All workspace members have a cargo test step in build.yml." From 7af7aef8feec9137beec285c184b14502ef7e557 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 9 Aug 2026 14:44:16 -0500 Subject: [PATCH 2/2] =?UTF-8?q?ci:=20address=20review=20=E2=80=94=20scope?= =?UTF-8?q?=20the=20guard's=20claim,=20POSIX=20regexes,=20point=20to=20the?= =?UTF-8?q?=20real=20migration=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review fixes: 1. The guard's header cited five prior incidents as the class it fixes, but two of them (river-core's bulk lib tests, the nine common/tests allowlist files) happened while river-core DID have -p test steps — a configuration the guard passes. It checks presence, not adequacy. Says so now, since a guard overclaiming its coverage is the failure mode this PR is about. 2. `\s` is GNU-only, so the script failed spuriously on macOS/BSD. Uses [[:space:]] throughout. 3. The build.yml caveat said what the delegate tests don't cover but not where that coverage lives. Now points at the #613 pin in `cargo test -p river-ui --bins`, and cites handlers.rs:191-198 (191 is the cfg arm; 190 was the prose comment). Also parses `members = [...]` by accumulating to the closing bracket, so a single-line reformat is handled instead of running to EOF and reporting "member '2' has no Cargo.toml" (it had picked up `resolver = "2"`). [AI-assisted - Claude] --- .github/workflows/build.yml | 13 ++++++++-- scripts/check-ci-test-coverage.sh | 40 +++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 3655efab6..cde3d4a8f 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -229,8 +229,17 @@ jobs: # # Scope caveat, so this is not over-read: on native targets # `DelegateCtx`'s `set_secret` is a no-op and `get_secret` always returns - # `None` (freenet-stdlib's stub — see the cfg at handlers.rs:190). These - # tests cover dispatch and pure-value logic, NOT storage round-trips. + # `None` (freenet-stdlib's stub — see the `cfg(not(target_family = + # "wasm"))` arm at handlers.rs:191-198). These tests cover dispatch and + # pure-value logic, NOT storage round-trips. Measured, not assumed: + # deleting the `set_key_index(...)` call from `handle_store_request` — a + # regression that would stop every room migrating — leaves all 40 of + # these tests passing. + # + # The migration behaviour is gated elsewhere: the #613 source-scrape pin + # `only_the_indexed_delegate_paths_register_keys_for_migration`, which + # runs in the `cargo test -p river-ui --bins` step above and does catch + # that deletion. Trust that step, not this one, for freenet/river#612. run: cargo test -p chat-delegate - name: Test web-container-contract + web-container-tool diff --git a/scripts/check-ci-test-coverage.sh b/scripts/check-ci-test-coverage.sh index c4151d120..7e13a14cc 100755 --- a/scripts/check-ci-test-coverage.sh +++ b/scripts/check-ci-test-coverage.sh @@ -14,6 +14,18 @@ # tests into CI now fails CI. See the long-form comments in # .github/workflows/build.yml for the individual incidents. # +# SCOPE — this catches the ZERO-STEP class only, which is narrower than the +# list above may suggest. Two of those five incidents (river-core's bulk lib +# tests, the nine common/tests files behind a `--test ` allowlist) +# happened while river-core DID have `cargo test -p river-core --test X` steps +# — a configuration this script passes. It checks that a member has SOME test +# step, not that the step is ADEQUATE: a partial `--test ` allowlist +# still satisfies it, and a file missing from such an allowlist is neither run +# nor even compiled. Guarding that class needs a catch-all step instead — see +# the `cargo test -p river-core --tests` comment in build.yml. Stating this +# plainly because a guard that overclaims its own coverage is precisely the +# failure mode this PR is about. +# # A member with genuinely no tests still needs a step — `cargo test -p ` # on a testless crate is fast and passes, and it means the day someone adds the # first test to that crate, it runs. @@ -29,18 +41,35 @@ root_manifest="$repo_root/Cargo.toml" # `run:` lines that invoke cargo test. Restricting to these means a package # named only in a build step or a comment does not count as covered. -test_invocations="$(grep -E '^\s*(run:|-)?\s*cargo test ' "$workflow" || true)" +# POSIX classes, not `\s` — `\s` is a GNU extension, so a dev running this on +# macOS/BSD would otherwise get a spurious parse failure. CI is Linux, but this +# script is meant to be runnable locally. +test_invocations="$(grep -E '^[[:space:]]*(run:|-)?[[:space:]]*cargo test ' "$workflow" || true)" if [[ -z "$test_invocations" ]]; then echo "ERROR: build.yml contains no 'cargo test' invocations at all." >&2 exit 1 fi -members="$(sed -n '/^members = \[/,/^]/p' "$root_manifest" \ - | grep -oE '"[^"]+"' | tr -d '"')" +# Capture the `members = [ ... ]` array, stopping at the closing bracket. +# Accumulating until `]` handles both the multi-line form and a single-line +# reformat; an earlier line-range version ran to EOF on the single-line form +# and swept up unrelated values (it reported "member '2' has no Cargo.toml", +# having picked up `resolver = "2"`). It failed closed, but unreadably. +members_block="$(awk ' + /^members[[:space:]]*=[[:space:]]*\[/ { inblock = 1 } + inblock { buf = buf $0; if (index($0, "]") > 0) { print buf; exit } } +' "$root_manifest")" + +if [[ -z "$members_block" ]]; then + echo "ERROR: could not locate a 'members = [ ... ]' array in $root_manifest" >&2 + exit 1 +fi + +members="$(grep -oE '"[^"]+"' <<<"$members_block" | tr -d '"')" if [[ -z "$members" ]]; then - echo "ERROR: could not parse [workspace] members from $root_manifest" >&2 + echo "ERROR: [workspace] members in $root_manifest parsed as empty" >&2 exit 1 fi @@ -53,7 +82,8 @@ for member in $members; do continue fi - pkg="$(grep -m1 -E '^name\s*=' "$manifest" | sed -E 's/^name\s*=\s*"(.*)".*/\1/')" + pkg="$(grep -m1 -E '^name[[:space:]]*=' "$manifest" \ + | sed -E 's/^name[[:space:]]*=[[:space:]]*"(.*)".*/\1/')" if [[ -z "$pkg" ]]; then echo "ERROR: could not read package name from $manifest" >&2 missing=1