refactor(scripts): unify test-harness scaffolding in scripts/lib/test-harness.sh

[kyle-sexton]

Closes #3160

Summary

The shell suites under scripts/ re-declared PASS/FAIL counters, ok()/fail(), and a trailing summary in eight spellings. The copies were free to disagree on the exit contract: a suite that printed failures could still exit 0, and CI would treat that as green.

Fix

scripts/lib/test-harness.sh is a sourced-only library (no shebang, mode 100644) that owns ok, fail, test_harness::report, and the exit status. The shape is called, not an EXIT trap — bash has one trap slot, and the suites with fixture cleanup already use it. Each migrated suite's last line is test_harness::report.

This pass migrates the 23 suites that already shared the ok: / FAIL: / PASS/FAIL dialect. TAP-ish suites stay on their own dialect; incremental coexistence is in the brief. Plugin *.test.sh helpers stay duplicated on purpose (docs/conventions/shell-test-helpers/README.md).

scripts/lib/test-harness.test.sh asserts that a recorded failure plus report cannot exit 0, and that every suite sourcing the harness ends with the report call. The suite is wired into plugin-gate next to the other scripts/lib tests.

Fixture stage_libs helpers are unchanged: the gates they copy do not source the harness.

Verification

• bash scripts/lib/test-harness.test.sh
• Migrated suites keep their existing assertion messages; only the scaffolding and the collapsed summary line change.
• shellcheck --rcfile=.shellcheckrc -x and shfmt -d clean on the new library and its suite.
• bash -n clean across every migrated file.

Related

• Refs coupling: route-lane findings from /coupling:reduce dry-run over scripts/ #2914 — parent umbrella, finding 6 of 7.
• Refs refactor(scripts): extract the shared changed-file and awk-operand seams #3144 — stage_libs fixture pattern this pass does not widen.
• Refs ci: hoist the fetch-depth and base-fetch incantation into a checkout-with-base composite action #3158 ci: hoist docs_only scope resolution into one job instead of 46 references #3159 — sibling coupling findings, not touched here.

[claude]

Claude finished @kyle-sexton's task in 1m 2s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Run /review:security-review criteria against the diff
• Post findings

No security issues found.

This PR is a pure test-scaffolding refactor: it consolidates duplicated PASS/FAIL counters and ok()/fail() helpers across 23 scripts/*.test.sh suites into a shared, sourced-only library (scripts/lib/test-harness.sh), plus its self-test and a doc update. Reviewed for the criteria in scope (logic/trust-boundary flaws, injection surfaces, credential handling, and GitHub Actions risk):

• scripts/lib/test-harness.sh — no eval, no unsanitized interpolation into a command context; ok/fail only printf their arguments as data. The sourced-only guard ([[ "${BASH_SOURCE[0]}" == "$0" ]]) is a benign misuse check, not a security boundary.
• scripts/lib/test-harness.test.sh — bash -c "$body" interpolates only $HARNESS, a path derived from $(cd ... && pwd) in the same script, not from any external/attacker-controlled input.
• .github/workflows/ci.yml — adds one run: bash scripts/lib/test-harness.test.sh step gated the same way as its neighbors (needs.scope.outputs.run_full == 'true'). No permissions: widening, no trigger changes, no use of pull_request_target/workflow_run, no new secrets exposure.
• The other 22 migrated *.test.sh files are mechanical replacements of local PASS=0/FAIL=0/ok()/fail() blocks with a . "$SELF_DIR/lib/test-harness.sh" source line and a trailing test_harness::report call — same exit-status semantics, no new external inputs or privilege changes introduced.

No CI-internal, non-production test tooling with no attacker-controlled inputs and no workflow permission/trigger changes — nothing in this diff crosses a trust boundary.

[github-actions]

Last security-reviewed head: d90836c27f9cb901f724f8a4e14dfefb6a0681b7. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

Claude finished @kyle-sexton's task in 3m 47s —— View job

---

Code review complete

• Gather context (PR diff, CLAUDE.md/AGENTS.md, REVIEW.md)
• Review scripts/lib/test-harness.sh and its test suite
• Review migrated suites for correctness of the migration
• Review CI wiring and docs updates
• Post findings

No blocking findings. This is a clean, mechanical refactor.

What I checked

• scripts/lib/test-harness.sh (view): sourced-only (mode 100644, no shebang, guarded by the BASH_SOURCE[0] == $0 check), matches the file-mode convention of sibling libraries (read-list.sh, changed-files.sh). test_harness::report is a plain function call (not an EXIT trap), so it correctly coexists with the two suites that already own the trap slot for fixture cleanup (check-skill-portability.test.sh, check-silent-revert.test.sh — verified both still carry their own trap ... EXIT untouched).
• Exit-status equivalence: diffed all 23 migrated suites against their pre-migration versions. Every removed tail ([[ "$FAIL" -eq 0 ]], ((FAIL == 0)), ((FAIL == 0)) || exit 1, etc.) was semantically boolean-equivalent to the new test_harness::report's return 1 on any recorded failure / return 0 otherwise — no suite silently changes its exit contract.
• No stray references: grepped for leftover $PASS/$FAIL usage across all migrated files — none found, so no suite is left referencing removed counters.
• Harness self-test (scripts/lib/test-harness.test.sh): asserts the sourced-only guard, the fail+report non-zero exit, the all-pass zero exit, and — via last_code_line/sources_harness — that every current and future sourcer ends with test_harness::report as its literal last statement. That check is self-consistent (it also correctly classifies itself).
• CI wiring: the new Run shared test-harness tests step in .github/workflows/ci.yml is placed consistently alongside the sibling scripts/lib/*.test.sh steps, gated the same way (if: needs.scope.outputs.run_full == 'true').
• Docs: docs/conventions/shell-test-helpers/README.md update accurately reflects the new shared library and doesn't leave stale "bespoke PASS/FAIL by design" language behind (confirmed via grep — no such stale comments remain in any migrated file).
• Spot-checked several suites with non-trivial control flow (check-cross-plugin-source-drift.test.sh's run_check/run_discover subshell helpers) to confirm ok/fail calls all happen at top level, not inside a subshell where the counter increment would be lost.

Not independently verified

The PR description's bash scripts/lib/test-harness.test.sh, shellcheck/shfmt, and bash -n runs are author-claimed; my sandbox in this pass didn't have permission to execute scripts, so I did not independently re-run them — this review is based on static reading of the diff and full file contents only.

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.