fix(tests): make every negated bats assertion able to fail, and lint for new ones (#2263)

Merged
DO
10d ago
+785-117

Summary

Makes every negated bats assertion able to fail, and adds a lint so new inert ones cannot land.

  • scripts/check-bats-negation.sh fails on a ! cmd in a .bats file that is not the last command of its test and has no || false. It skips heredoc bodies, and treats an unclosed heredoc or an empty scan set as an error, not a clean pass.
  • lint.yml runs it as a new bats-negation job and runs its tests in the bats job.
  • 113 inert negations in 41 test files get || false appended. No assertion is removed, skipped or exempted.
  • tests/dev-lead/unit/test_engine_mcp.bats: two "no MCP knob → no MCP flags" tests failed as soon as their negation was real. The repo ships .github/review-mcp.json, the conventional default path, so "no knob" means MCP on. The tests now point the default path away from it, as their sibling tests already do.
  • scripts/dev-lead-fix-reviews.sh: one product fix the conversion exposed. finalize_review_application decided whether a commit was substantive with git diff -w --name-only, but git lists a file under --name-only even when -w leaves nothing to show. The #1567 guard therefore never fired, and a whitespace-only commit was posted as status=applied. It now uses --numstat; a diff that cannot be computed still counts as substantive.
  • AGENTS.md states the rule and documents the repo-specific lint job.

Problem

Closes #2263. Bash exempts a !-negated command from errexit, so mid-test it runs, its result is discarded, and the test passes either way. These lines mostly guard the "must not happen" half of a fix (no comment posted, no flag passed, no retraction), so they looked like coverage and were not. dev-lead added one on PR #2205 as its fix for a bot finding.

Risk

Low to medium. Tests, a lint script, the Lint workflow and docs change, plus one behaviour change in the dev-lead harness: a review-changes pass whose commit differs from the PR head only in whitespace is now reported not-applied instead of applied, which is what #1567 specified. Like any harness change it is inert until the dev-lead channel tag moves onto it.

Test plan

  • shellcheck --severity=warning -x scripts/check-bats-negation.sh: clean.
  • bats tests/test_check_bats_negation.bats: 17/17, under both gawk and the stock macOS awk.
  • bash scripts/check-bats-negation.sh: 113 findings on main before the conversion, 0 after (240 files scanned).
  • bats tests/dev-lead/unit/test_engine_mcp.bats: 34/34 after the setup fix (2 failing before it, at the converted lines).
  • Seven converted suites run in full locally (193 tests) with no failures.
  • All converted files were also run locally in a scratch checkout: 3 tests failed at a converted line. Two are the MCP tests fixed here. The third is test_fix_reviews.bats "review-changes evidence: whitespace/cosmetic-only change → not applied (#1567)", which fails on main's harness and passes with the --numstat fix (all 5 review-changes evidence tests pass; git diff -w --name-only vs --numstat checked directly on a whitespace-only commit with git 2.50.1).
  • The other local failures were at unrelated lines and match this Mac's known tooling gaps; CI is the authority for those.
  • shellcheck --severity=warning -x scripts/dev-lead-fix-reviews.sh: clean.
  • actionlint .github/workflows/lint.yml and markdownlint-cli2 AGENTS.md: clean.

Review follow-up (1006b6f6, 5216f09b)

  • A failed find exits 2 (CodeAnt). The listing ran inside a process substitution, where its status is ignored; it is now captured and checked.
  • Commands sharing a line (CodeAnt, QA Lead). A line is split at each ;: foo; ! bar is judged like a negation on its own line, and ! foo; bar is flagged. Quoted text and a trailing comment are blanked first, so a || false inside either no longer exempts a negation. tests/ held no instance of any of these, so nothing more was converted for them.
  • persist-credentials: false on the bats-negation checkout (CodeRabbit), as the later read-only jobs in lint.yml have.
  • Two more tests for the whitespace-only fix (QA Lead). One pins the old git diff -w --name-only answer with a git stub, so it fails on the old check whatever git the runner has. The other covers the fail-open branch when the diff errors. The version dependence was reproduced only on git 2.50.1; git 2.55 was not available to check.
  • Three more conversions. main gained three inert negations after this branch's CI last ran (two from #2231, one from #2241), which the new job would have failed on. main is merged in and they are converted; all three assertions hold. Total is now 116.
  • Not done: tests pinning how a commit that only adds a blank line, or only changes a file mode, is classified. Under -w --numstat both count as substantive today. Which outcome #1567 intends is a product decision, so nothing pins it here.
  • bats tests/test_check_bats_negation.bats: 26/26. Of the 9 new tests, 5 fail on the previous script; the other 4 guard against over-flagging. bash scripts/check-bats-negation.sh: 241 files, clean. The 7 review-changes evidence tests pass; the 2 new ones fail with the source change reverted.

Rollback

Revert this PR. No non-revertible side effects.

Monitoring

The bats-negation job in the Lint workflow, and the existing bats and dev-lead unit-test jobs. For the harness fix: a review-changes terminal comment reading "no substantive change (whitespace/cosmetic only)" once the change reaches the dev-lead ring a repo pins (it rides the next autocut to dev-lead/v139-next, then ring promotion).

Interaction contract

  • N/A: no agentic role added or changed.

🤖 Generated with Claude Code