From 190a3adf9c28b7e88299b2d2a774a864f845684a Mon Sep 17 00:00:00 2001 From: hushen <190065939+918154429@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:06:28 +0800 Subject: [PATCH] fix(ci): guard each live Postgres contract invocation --- CHANGELOG.md | 8 + docs/developer-guide.md | 2 + scripts/ci-local.sh | 20 +- tests/release/postgres-contract-guard.test.sh | 303 ++++++++++++++---- 4 files changed, 272 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a8b8a47..8df09449 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,14 @@ Releases before `0.2.5` predate the public launch; their notes live in the ## [Unreleased] +### Fixed + +- Guard each live PostgreSQL test invocation independently, including the CLI + anchor exit-code contract, and run that CLI contract in both local CI paths. + Missing ignore flags, unresolved test filters, missing contract targets and + disabled required-database settings now fail the guard. Quoted step labels + cannot substitute for the arguments that actually execute (#362). + ## [0.13.1] — 2026-09-05 The last digit moves. No shipped code changed: `crates/**`, `apps/*/src/**` and diff --git a/docs/developer-guide.md b/docs/developer-guide.md index fde3a280..db1bff7b 100644 --- a/docs/developer-guide.md +++ b/docs/developer-guide.md @@ -139,6 +139,8 @@ podman run -d --name sysknife-pg -p 55987:5432 \ export SYSKNIFE_TEST_POSTGRES_URL=postgres://sysknife:sysknife@127.0.0.1:55987/sysknife_test export SYSKNIFE_REQUIRE_POSTGRES=1 cargo test -p sysknife-daemon --test postgres_store --locked -- --include-ignored +cargo test -p sysknife-cli --test cli_smoke --locked \ + audit_verify_exits_with_code_1_when_anchor_is_truncated -- --ignored --exact podman rm -f sysknife-pg ``` diff --git a/scripts/ci-local.sh b/scripts/ci-local.sh index 40771ee0..cd16a733 100755 --- a/scripts/ci-local.sh +++ b/scripts/ci-local.sh @@ -334,7 +334,7 @@ run_security_group() { run_postgres_contract_group() { printf '\n### postgres-contract (optional)\n' - local label="postgres-contract: cargo test -p sysknife-daemon --test postgres_store --locked -- --include-ignored" + local label="postgres-contract: live Postgres contract (store + CLI anchor exit code)" if [[ "$run_postgres" != true ]]; then record SKIP "${label} (--no-postgres)" @@ -343,7 +343,12 @@ run_postgres_contract_group() { if [[ -n "${SYSKNIFE_TEST_POSTGRES_URL:-}" ]]; then SYSKNIFE_REQUIRE_POSTGRES=1 \ - run_step "$label" cargo test -p sysknife-daemon --test postgres_store --locked -- --include-ignored + run_step "postgres-contract: cargo test -p sysknife-daemon --test postgres_store --locked -- --include-ignored" \ + cargo test -p sysknife-daemon --test postgres_store --locked -- --include-ignored + SYSKNIFE_REQUIRE_POSTGRES=1 \ + run_step "postgres-contract: cargo test -p sysknife-cli --test cli_smoke --locked audit_verify_exits_with_code_1_when_anchor_is_truncated -- --ignored --exact" \ + cargo test -p sysknife-cli --test cli_smoke --locked \ + audit_verify_exits_with_code_1_when_anchor_is_truncated -- --ignored --exact return fi @@ -384,9 +389,16 @@ run_postgres_contract_group() { sleep "$POSTGRES_HEALTH_INTERVAL_SECS" done - SYSKNIFE_TEST_POSTGRES_URL="postgres://sysknife:sysknife@127.0.0.1:${POSTGRES_HOST_PORT}/sysknife_test?sslmode=disable" \ + local container_url="postgres://sysknife:sysknife@127.0.0.1:${POSTGRES_HOST_PORT}/sysknife_test?sslmode=disable" + SYSKNIFE_TEST_POSTGRES_URL="$container_url" \ + SYSKNIFE_REQUIRE_POSTGRES=1 \ + run_step "postgres-contract: cargo test -p sysknife-daemon --test postgres_store --locked -- --include-ignored" \ + cargo test -p sysknife-daemon --test postgres_store --locked -- --include-ignored + SYSKNIFE_TEST_POSTGRES_URL="$container_url" \ SYSKNIFE_REQUIRE_POSTGRES=1 \ - run_step "$label" cargo test -p sysknife-daemon --test postgres_store --locked -- --include-ignored + run_step "postgres-contract: cargo test -p sysknife-cli --test cli_smoke --locked audit_verify_exits_with_code_1_when_anchor_is_truncated -- --ignored --exact" \ + cargo test -p sysknife-cli --test cli_smoke --locked \ + audit_verify_exits_with_code_1_when_anchor_is_truncated -- --ignored --exact "$runtime" rm -f "$POSTGRES_CONTAINER_NAME" >/dev/null 2>&1 || true } diff --git a/tests/release/postgres-contract-guard.test.sh b/tests/release/postgres-contract-guard.test.sh index 5a54abac..d0f3ea08 100755 --- a/tests/release/postgres-contract-guard.test.sh +++ b/tests/release/postgres-contract-guard.test.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # -# postgres-contract-guard.test.sh — both halves of the live Postgres contract -# must stay on the job that claims to run it. +# postgres-contract-guard.test.sh — every live Postgres contract must stay on +# the job that claims to run it, and on the local mirror of that job. # # #313 closed the case where postgres-contract reported success with no # database configured. The job now sets SYSKNIFE_REQUIRE_POSTGRES and runs @@ -10,24 +10,32 @@ # "6 passed; 5 ignored" and exits 0 — #294's shape with a different cause. # See issue #315. # -# This check reads the job and fails when either token is missing. It does -# not assert a test count: that would break the next time anyone adds a -# store test and teach whoever hit it to edit the number rather than read -# the check. +# The job later grew a second live step (#340, the CLI anchor exit-code +# contract) and this check did not see it, because it looked for its two +# tokens anywhere in the job rather than on each command. Three mutations of +# that step left the guard green: dropping --ignored (cargo reports +# "1 ignored", exit 0), misspelling the test name by one character (cargo +# reports "running 0 tests", exit 0), and deleting the step outright. So the +# check now reads each cargo invocation separately. # -# Two design rules, both from tests/e2e/provider-parity.test.sh: +# Design rules, all three inherited from tests/e2e/provider-parity.test.sh: # -# * The tokens are derived from postgres_store.rs, never restated as a -# count, so renaming the env var or dropping #[ignore] fails here -# until the job catches up. +# * Everything is derived from the Rust sources, never restated as a +# count or a list. A test file that names SYSKNIFE_REQUIRE_POSTGRES is +# a live-Postgres contract file, so some step has to run it; renaming +# the env var or dropping #[ignore] fails here until the job catches up. # * It covers every entry point, not just CI. ci-local.sh carries the -# pair in two branches (URL already exported, and the path that starts -# a container first). A guard that watches CI and ignores the local -# mirror leaves half the surface open. +# contract in two branches (URL already exported, and the path that +# starts a container first). A guard that watches CI and ignores the +# local mirror leaves half the surface open. +# * No test count is asserted. That would break the next time anyone adds +# a store test and would teach whoever hit it to edit the number rather +# than read the check. # # A pattern that matched nothing would make every assertion below vacuously -# true, which is the exact failure this suite exists to stop. Missing files -# and an empty job extract fail rather than pass over nothing to read. +# true, which is the exact failure this suite exists to stop. Missing files, +# an empty job extract and an empty marker set all fail rather than pass over +# nothing to read. # # Host-side only: greps files, needs no VM, no daemon, no network. # Wired into docs-and-hygiene and scripts/ci-local.sh; a test nothing @@ -37,41 +45,142 @@ set -euo pipefail repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" ci_yml="$repo_root/.github/workflows/ci.yml" ci_local="$repo_root/scripts/ci-local.sh" -store="$repo_root/crates/sysknife-daemon/tests/postgres_store.rs" -for f in "$ci_yml" "$ci_local" "$store"; do +for f in "$ci_yml" "$ci_local"; do [ -f "$f" ] || { printf 'FAIL: missing file: %s\n' "$f" >&2; exit 1; } done -# Fail-closed env var, taken from the store test the job runs. -# sort -u consumes every grep hit so a later head cannot SIGPIPE the pipeline -# under `set -o pipefail`. -require_token="$(grep -o 'SYSKNIFE_REQUIRE_POSTGRES' "$store" | sort -u || true)" -if [ -z "$require_token" ]; then - printf 'FAIL: %s no longer names SYSKNIFE_REQUIRE_POSTGRES; the job token cannot be derived\n' \ - "$store" >&2 - exit 1 -fi +failures=0 +report() { + printf 'FAIL %s\n' "$1" >&2 + failures=$((failures + 1)) +} -# Live tests carry #[ignore]; without --include-ignored cargo never runs them -# and still exits 0. Demand the attribute rather than trusting a count. -if ! grep -Eq '#\[ignore' "$store"; then - printf 'FAIL: %s has no #[ignore] tests; --include-ignored would not run the live contract\n' \ - "$store" >&2 +# -------------------------------------------------------------------------- +# The marker set: which test files are live Postgres contracts. +# -------------------------------------------------------------------------- +# The fail-closed env var is the marker. A file that reads it refuses to pass +# quietly without a database (#313), which is precisely what makes it a +# contract someone has to run. Deriving the set means a new live test file is +# covered the day it lands, with no list here to forget to update. +# +# `|| true` keeps an empty result reachable: grep exiting 1 on no match would +# otherwise kill the script at the assignment under `set -o pipefail`, and the +# diagnostic below would never print. That shape cost five guards their error +# messages (#347). +require_token='SYSKNIFE_REQUIRE_POSTGRES' +marker_files="$( + grep -rl --include='*.rs' -F "$require_token" \ + "$repo_root/crates" "$repo_root/apps" 2>/dev/null \ + | grep '/tests/' | sort || true +)" +if [ -z "$marker_files" ]; then + printf 'FAIL: no test file under crates/ or apps/ names %s; the contract set cannot be derived\n' \ + "$require_token" >&2 exit 1 fi -# cargo's flag for #[ignore] tests. Named here once; the rust file proves -# those tests are ignored, the job must still pass the flag. -ignore_token='--include-ignored' -tokens=("$require_token" "$ignore_token") +# stem -> path, and the ordered list of stems. +declare -A marker_path=() +marker_stems=() +while IFS= read -r path; do + [ -n "$path" ] || continue + stem="$(basename "$path" .rs)" + if [ -n "${marker_path[$stem]:-}" ]; then + report "duplicate live test target $stem; qualify the guard by package before adding this target" + fi + marker_path["$stem"]="$path" + marker_stems+=("$stem") + # Live tests carry #[ignore]; without an ignore flag cargo never runs them + # and still exits 0. Demand the attribute rather than trusting a count. + if ! grep -Eq '#\[ignore' "$path"; then + report "${path#"$repo_root"/} names $require_token but has no #[ignore] test; no ignore flag would run its contract" + fi +done <<<"$marker_files" -failures=0 -report() { - printf 'FAIL %s\n' "$1" >&2 - failures=$((failures + 1)) +# cargo's two flags for #[ignore] tests. `--ignored` runs only them, +# `--include-ignored` runs them alongside the rest; either is acceptable, and +# neither is optional. +ignore_flag_re='(^|[[:space:]])(--include-ignored|--ignored)([[:space:]]|$)' + +# -------------------------------------------------------------------------- +# Shared assertions for one cargo invocation. +# -------------------------------------------------------------------------- +# `where` is a human-readable location for the diagnostic; `cmd` is the whole +# invocation on one line with runs of whitespace squashed. +check_invocation() { + local where="$1" cmd="$2" + + local harness_args="${cmd#* -- }" + if [[ "$harness_args" == "$cmd" ]] || ! [[ "$harness_args" =~ $ignore_flag_re ]]; then + report "$where runs cargo test without --ignored or --include-ignored, so its #[ignore] contract would be skipped and still exit 0" + fi + + local target="" + if [[ "$cmd" =~ --test[[:space:]]+([A-Za-z0-9_]+) ]]; then + target="${BASH_REMATCH[1]}" + else + report "$where runs cargo test with no --test target, so which contract it covers cannot be derived" + return + fi + + # Any bare word left after the options and before the `--` separator is a + # test-name filter. `cargo test NAME -- --exact` matching nothing prints + # "running 0 tests" and exits 0, so a filter that no longer resolves is a + # green step that tests nothing. + local head="${cmd%% -- *}" + local skip=0 tok filters=() + for tok in $head; do + if [ "$skip" = 1 ]; then skip=0; continue; fi + case "$tok" in + cargo | test) ;; + # A shell line continuation survives the whitespace squash. + '\') ;; + -p | --package | --test | --features | --manifest-path) skip=1 ;; + -* | *=*) ;; + *) filters+=("$tok") ;; + esac + done + + local file="${marker_path[$target]:-}" + if [ -z "$file" ]; then + # A --test target with no marker file is either a target that has no + # live contract, or one whose file has gone. Distinguish them: cargo + # errors on a missing target, but a green guard that names a target + # nothing can build is a guard reporting on nothing. + if ! compgen -G "$repo_root/*/*/tests/$target.rs" >/dev/null; then + report "$where targets --test $target, and no crates/*/tests/$target.rs or apps/*/tests/$target.rs exists to run" + fi + # Otherwise the ignore-flag check above still applies; there is no + # source of truth here to resolve a name against. + return + fi + + for tok in ${filters+"${filters[@]}"}; do + # The filter has to name an #[ignore]d test in the file this step + # targets. Walk up from the fn to the nearest attribute block so a + # non-ignored test cannot satisfy an --ignored run. + if ! awk -v name="$tok" ' + /^[[:space:]]*#\[ignore/ { ignored = 1 } + /^[[:space:]]*#\[(tokio::)?test\]/ { test = 1 } + /^[[:space:]]*(async[[:space:]]+)?fn[[:space:]]/ { + if ($0 ~ ("fn[[:space:]]+" name "[[:space:]]*\\(") && ignored && test) { + found = 1 + exit + } + ignored = 0 + test = 0 + } + END { exit found ? 0 : 1 } + ' "$file"; then + report "$where filters on '$tok', which is not an #[ignore] test in ${file#"$repo_root"/}; the step would run 0 tests and exit 0" + fi + done } +# -------------------------------------------------------------------------- +# .github/workflows/ci.yml +# -------------------------------------------------------------------------- # Top-level GitHub Actions job. Empty extract is a failure: grepping nothing # would report every token present. extract_job() { @@ -84,49 +193,129 @@ extract_job() { ' "$file" } +# One record per `- name:` step, folded scalars joined, whitespace squashed, so +# a flag on one step cannot satisfy an assertion about another. That blindness +# is what let #340's step regress unnoticed. +extract_steps() { + awk ' + /^ - name:/ { + if (buf != "") printf "%s\x1e", buf + buf = $0 + next + } + buf != "" { buf = buf " " $0 } + END { if (buf != "") printf "%s\x1e", buf } + ' <<<"$1" | tr -s '[:space:]' ' ' | tr '\036' '\n' +} + job="$(extract_job "$ci_yml" "postgres-contract")" if [ -z "$job" ]; then report "$ci_yml has no postgres-contract job to read" else - for token in "${tokens[@]}"; do - grep -Fq -- "$token" <<<"$job" \ - || report "$ci_yml postgres-contract job is missing $token" + grep -Eq "^[[:space:]]+$require_token: [\"']?1[\"']?([[:space:]]|$)" <<<"$job" \ + || report "$ci_yml postgres-contract job is missing $require_token" + + ci_targets="" + while IFS= read -r step; do + [ -n "$step" ] || continue + case "$step" in + *"cargo test"*) ;; + *) continue ;; + esac + step_name="$(sed -E 's/^ *- name: *//; s/ run:.*//' <<<"$step")" + # Drop the step's name and the `run: >-` preamble; the words in a step + # title are not arguments, and reading them as such reports a filter + # per word. + step="${step#* run: }" + step="${step#>- }" + step="${step#> }" + step="${step#| }" + if [[ "$step" != cargo\ test\ * ]]; then + report "$ci_yml step '$step_name' must directly run its cargo test command" + continue + fi + check_invocation "$ci_yml postgres-contract step '$step_name'" "$step" + if [[ "$step" =~ --test[[:space:]]+([A-Za-z0-9_]+) ]]; then + ci_targets="$ci_targets ${BASH_REMATCH[1]}" + fi + done < <(extract_steps "$job") + + # Every live-contract file needs a step. Deleting #340's step, which this + # check used to allow, fails here. + for stem in "${marker_stems[@]}"; do + case " $ci_targets " in + *" $stem "*) ;; + *) report "$ci_yml postgres-contract job runs no step against --test $stem, whose ${marker_path[$stem]##*/} is a live $require_token contract" ;; + esac done fi -# Each cargo invocation of postgres_store, including the env lines that +# -------------------------------------------------------------------------- +# scripts/ci-local.sh +# -------------------------------------------------------------------------- +# Each cargo invocation reached through run_step, including the env lines that # continue onto it. The label string also names the command, so only -# statements that actually call run_step count as a branch. +# statements that actually call run_step count. extract_invocations() { awk ' { if (buf != "") buf = buf "\n" $0 else buf = $0 if ($0 ~ /\\$/) next - if (buf ~ /run_step/ && buf ~ /postgres_store/) printf "%s\x1e", buf + if (buf ~ /run_step/ && buf ~ /cargo test/) printf "%s\x1e", buf buf = "" } ' "$1" } -branch_count=0 +declare -A local_target_count=() +invocation=0 while IFS= read -r -d $'\x1e' stmt; do - [ -z "$stmt" ] && continue - branch_count=$((branch_count + 1)) - for token in "${tokens[@]}"; do - grep -Fq -- "$token" <<<"$stmt" \ - || report "$ci_local postgres-contract branch $branch_count is missing $token" - done + [ -n "$stmt" ] || continue + invocation=$((invocation + 1)) + squashed="$(tr -s '[:space:]' ' ' <<<"$stmt")" + # The label repeats the command verbatim, so strip it before parsing or the + # quoted copy is read as a second invocation. + parsed="${squashed#*run_step }" + # Remove exactly one quoted label. Searching for the first 'cargo test' + # instead reads the label, allowing the actual command to silently drift. + label_re='^"[^"]*"[[:space:]]+(.*)$' + if [[ "$parsed" =~ $label_re ]]; then + parsed="${BASH_REMATCH[1]}" + parsed="${parsed#\\ }" + else + report "$ci_local invocation $invocation must have one quoted run_step label" + continue + fi + if [[ "$parsed" != cargo\ test\ * ]]; then + report "$ci_local invocation $invocation does not run cargo test" + continue + fi + environment="${squashed%%run_step*}" + if ! [[ "$environment" =~ (^|[[:space:]])SYSKNIFE_REQUIRE_POSTGRES=1([[:space:]]|$) ]]; then + report "$ci_local invocation $invocation must set SYSKNIFE_REQUIRE_POSTGRES=1" + fi + check_invocation "$ci_local invocation $invocation" "$parsed" + if [[ "$parsed" =~ --test[[:space:]]+([A-Za-z0-9_]+) ]]; then + target="${BASH_REMATCH[1]}" + local_target_count["$target"]=$(( ${local_target_count["$target"]:-0} + 1 )) + fi done < <(extract_invocations "$ci_local") -if [ "$branch_count" -lt 2 ]; then - report "$ci_local has $branch_count postgres-contract run_step branch(es); both the exported-URL path and the container-start path must carry the pair" -fi +# Both entry paths, for every contract. ci-local.sh reaches the database two +# ways (URL already exported, container started here) and a contributor who +# takes either one has to run the same set CI does. +for stem in "${marker_stems[@]}"; do + count="${local_target_count[$stem]:-0}" + if [ "$count" -lt 2 ]; then + report "$ci_local runs --test $stem in $count branch(es); both the exported-URL path and the container-start path must run it" + fi +done if [ "$failures" -ne 0 ]; then printf '\n%d postgres-contract guard failure(s).\n' "$failures" >&2 exit 1 fi -printf 'postgres-contract guard passed: %s and %s present on the CI job and %d ci-local.sh branches.\n' \ - "$require_token" "$ignore_token" "$branch_count" +printf 'postgres-contract guard passed: %d live contract file(s) (%s), each run with an ignore flag by ci.yml and by both ci-local.sh branches.\n' \ + "${#marker_stems[@]}" "${marker_stems[*]}"