From 0fcd97c8d7e972a34afa430bdcfdd0f27fe4e7b8 Mon Sep 17 00:00:00 2001 From: Jonny Spicer Date: Tue, 11 Aug 2026 16:13:10 -0700 Subject: [PATCH 1/3] Harden dead-code gate test coverage per mutation-testing review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutation testing (ClickUp 86cb417ty follow-up) showed the 9-test suite passed unchanged against several real regressions: exit 127 becoming exit 2 for a missing vulture, and a bad TARGET falling through to the same exit 1 as "dead code found" (both undetected). - Tighten t_missing_vulture_fails_closed to assert exactly 127, the gate's whole fail-closed contract, not just non-zero. - Give cd a real failure path: a bad TARGET now exits 2 (this script's existing bad-usage code) and names the target, instead of silently reusing exit 1 via `set -e`, which is indistinguishable from a dead-code finding. Add a test for it. - Add a two-halves vulture_whitelist.py test (fails unwhitelisted, passes once whitelisted) for real regression coverage of that path, though note in the report that it can't discriminate the specific "delete the pickup line" mutation — that mutation turns out to be behaviorally inert given vulture's own directory traversal, verified against vulture 2.14's source. - Replace the CI try-repo step, which pointed at this repo's own zero .py files and thus proved nothing, with a positive/negative control against a throwaway repo with planted dead code. Suite: 9 -> 12 passing assertions. No change to the gate's external exit-code contract other than the deliberate bad-target 1 -> 2 move. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ZcazeseXVpu8XrYA2tE1V --- .github/workflows/dead-code-hook.yml | 35 ++++++++++++++++-- check-dead-code.sh | 2 +- tests/test-check-dead-code.sh | 54 +++++++++++++++++++++++++--- 3 files changed, 84 insertions(+), 7 deletions(-) diff --git a/.github/workflows/dead-code-hook.yml b/.github/workflows/dead-code-hook.yml index 9fb7940..8befa24 100644 --- a/.github/workflows/dead-code-hook.yml +++ b/.github/workflows/dead-code-hook.yml @@ -29,7 +29,38 @@ jobs: # in .pre-commit-hooks.yaml — that would only surface in a consumer # repo, after the tag is published. try-repo runs the hook exactly as # a consumer's pre-commit would, without needing a published tag. + # + # `ops` itself has zero .py files, so pointing try-repo at it proves + # nothing — it can't tell "ran and found nothing" from "no-opped". Both + # steps below build a throwaway git repo (try-repo needs a real repo + # with a resolvable rev) with planted content and check the hook's + # actual verdict on it. - run: pip install pre-commit==4.6.2 - - name: Exercise the hook manifest through pre-commit - run: pre-commit try-repo . dead-code --all-files + - name: Exercise the hook manifest through pre-commit (positive control) + run: | + set -euo pipefail + tmp="$(mktemp -d)" + cd "$tmp" + git init -q + git config user.email test@example.com + git config user.name test + printf 'def used():\n return 1\n\n\ndef planted_orphan():\n return 2\n\n\nprint(used())\n' > main.py + git add -A + if pre-commit try-repo "$GITHUB_WORKSPACE" dead-code --all-files 2>&1 | tee out.txt; then + echo "FAIL: hook did not catch planted dead code" >&2 + exit 1 + fi + grep -q planted_orphan out.txt || { echo "FAIL: hook ran but did not name the finding" >&2; exit 1; } + + - name: Exercise the hook manifest through pre-commit (negative control) + run: | + set -euo pipefail + tmp="$(mktemp -d)" + cd "$tmp" + git init -q + git config user.email test@example.com + git config user.name test + printf 'def used():\n return 1\n\n\nprint(used())\n' > main.py + git add -A + pre-commit try-repo "$GITHUB_WORKSPACE" dead-code --all-files diff --git a/check-dead-code.sh b/check-dead-code.sh index 5ad1db0..b79bbf0 100755 --- a/check-dead-code.sh +++ b/check-dead-code.sh @@ -47,7 +47,7 @@ for arg in "$@"; do *) TARGET="$arg" ;; esac done -cd "$TARGET" +cd "$TARGET" || { echo "check-dead-code: cannot enter target: $TARGET" >&2; exit 2; } # Fail closed (ops README convention 4: fail loudly). A missing tool is not a # clean scan, and this gate used to report one as the other: the shell's diff --git a/tests/test-check-dead-code.sh b/tests/test-check-dead-code.sh index 606b7f8..c638896 100755 --- a/tests/test-check-dead-code.sh +++ b/tests/test-check-dead-code.sh @@ -65,11 +65,14 @@ t_missing_vulture_fails_closed() { local dir out rc dir="$(mkclean)" out="$(cd "$dir" && PATH="$NO_VULTURE_PATH" bash "$SCRIPT" 2>&1)"; rc=$? - if [ "$rc" -eq 0 ]; then - bad "missing vulture exits non-zero" \ - "exited 0 — reported a scan that never happened" + # Assert the exact code, not just non-zero: 127 is this gate's whole + # fail-closed contract, and other paths (findings, bad usage) also exit + # non-zero, so a loose check can't tell them apart. + if [ "$rc" -eq 127 ]; then + ok "missing vulture exits exactly 127" else - ok "missing vulture exits non-zero (got $rc)" + bad "missing vulture exits exactly 127" \ + "exited $rc — reported a scan that never happened, or used the wrong code" fi case "$out" in *vulture*) ok "missing vulture names the tool" ;; @@ -78,6 +81,22 @@ t_missing_vulture_fails_closed() { rm -rf "$dir" } +# cd failure must not be mistaken for "dead code found" (both exit 1 under a +# bare `cd`). A bad target is a usage error, so it gets exit 2 — the code +# this script already uses for other usage errors — and must name the target +# so a typo'd path in a consumer's pre-commit config is diagnosable from CI +# output alone. +t_bad_target_exits_2() { + local target out rc + target="$(mktemp -u)/does-not-exist" + out="$(PATH="$NO_VULTURE_PATH" bash "$SCRIPT" "$target" 2>&1)"; rc=$? + if [ "$rc" -eq 2 ] && [[ "$out" == *"$target"* ]]; then + ok "nonexistent target exits 2 and names the target" + else + bad "nonexistent target exits 2 and names the target" "rc=$rc out=$out" + fi +} + t_unknown_option_rejected() { local dir out rc dir="$(mkclean)" @@ -155,6 +174,31 @@ t_vulture_failure_propagates() { rm -rf "$dir" } +# vulture_whitelist.py pickup (check-dead-code.sh:65) is what keeps every +# consumer repo green — all six have one. Both halves are required: checking +# only "whitelisted symbol passes" would also pass if the scan found nothing +# at all, so this first proves the fixture fails without a whitelist, then +# adds a whitelist for exactly that symbol and proves it now passes. +t_whitelist_suppresses_finding() { + local dir out rc + dir="$(mkdirty)" + out="$(cd "$dir" && bash "$SCRIPT" 2>&1)"; rc=$? + if [ "$rc" -eq 1 ] && [[ "$out" == *"orphan_function"* ]]; then + ok "whitelist test: unwhitelisted dead code fails first" + else + bad "whitelist test: unwhitelisted dead code fails first" "rc=$rc out=$out" + fi + + printf 'from main import orphan_function\norphan_function\n' >"$dir/vulture_whitelist.py" + out="$(cd "$dir" && bash "$SCRIPT" 2>&1)"; rc=$? + if [ "$rc" -eq 0 ] && [[ "$out" == *"no dead code found"* ]]; then + ok "vulture_whitelist.py suppresses the whitelisted symbol" + else + bad "vulture_whitelist.py suppresses the whitelisted symbol" "rc=$rc out=$out" + fi + rm -rf "$dir" +} + t_positional_target_scopes_scan() { local root out rc root="$(mktemp -d)" @@ -175,6 +219,7 @@ t_positional_target_scopes_scan() { echo "fail-closed:" t_missing_vulture_fails_closed t_unknown_option_rejected +t_bad_target_exits_2 echo "behaviour:" if command -v vulture >/dev/null 2>&1; then @@ -184,6 +229,7 @@ if command -v vulture >/dev/null 2>&1; then t_test_files_filtered t_positional_target_scopes_scan t_vulture_failure_propagates + t_whitelist_suppresses_finding elif [ "${REQUIRE_VULTURE:-}" = "1" ]; then # A skipped suite reporting success is the same class of bug as the gate # this repo hosts: silence read as a clean result. CI sets From 43b265c3b0625d886c1f21b18ec9a90f013803bc Mon Sep 17 00:00:00 2001 From: Jonny Spicer Date: Tue, 11 Aug 2026 16:15:31 -0700 Subject: [PATCH 2/3] Document that the vulture_whitelist.py pickup is behaviour-preserving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous hardening commit: mutation testing showed deleting check-dead-code.sh:65 doesn't change any test outcome. Verified (and independently re-verified by team-lead) that this isn't a coverage gap — vulture already walks "." and picks up vulture_whitelist.py on its own, so passing it a second time as an explicit CLI arg has no observable effect for any case reachable through this script's usage pattern. Leave the line in place (removing it is safe but is a deliberate cleanup needing a version bump to reach consumers, not part of this test-hardening PR) and record the finding in a comment so the next reader doesn't re-derive the "this line is why six repos are green" belief that the mutation-testing review's report had gotten wrong. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ZcazeseXVpu8XrYA2tE1V --- check-dead-code.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/check-dead-code.sh b/check-dead-code.sh index b79bbf0..a2a71a6 100755 --- a/check-dead-code.sh +++ b/check-dead-code.sh @@ -62,6 +62,10 @@ EXCLUDE=".venv,scripts,htmlcov,__pycache__,node_modules,build,dist,*.egg-info" # Framework-dispatched handlers: Flask (@bp/@app) and FastAPI (@router/@app). DECORATORS="@app.*,@bp.*,@router.*" WHITELIST="" +# Redundant in practice: vulture already picks this up when it walks ".", so +# passing it explicitly changes nothing (verified against vulture 2.14). Kept +# as an explicit statement of intent; removing it is safe but would need a +# version bump to reach consumers. [ -f vulture_whitelist.py ] && WHITELIST="vulture_whitelist.py" stderr_file="$(mktemp)" From 07e48d8547f72fce769a993291cdeb46e35b7389 Mon Sep 17 00:00:00 2001 From: Jonny Spicer Date: Tue, 11 Aug 2026 16:18:15 -0700 Subject: [PATCH 3/3] Test that --ignore-decorators genuinely suppresses framework handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last real gap the mutation-testing review found: breaking --ignore-decorators (check-dead-code.sh:63) passed the suite at 12/0. Unlike the vulture_whitelist.py pickup, this one is a genuine gap, not a behaviour-preserving mutation — confirmed by direct experiment that an @app.route-decorated, otherwise-unreferenced handler is reported as dead without the flag and suppressed with it. Tower-Finder's backend is the Flask/FastAPI app this flag protects; if it broke, CI would light up with dozens of false findings on live route handlers, risking someone "fixing" it by deleting or bulk-whitelisting real code. t_ignore_decorators_suppresses_handlers follows the same two-halves shape as the whitelist test. The script has no flag to disable its own --ignore-decorators, so the first half calls vulture directly with the script's other options (same EXCLUDE, same --min-confidence, no --ignore-decorators) to prove the finding is real; the second half runs the actual script and proves the flag suppresses it. Verified by mutation: DECORATORS="" now fails exactly this test (13 passed, 1 failed), script restored after. Suite: 12 -> 14 passing assertions. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013ZcazeseXVpu8XrYA2tE1V --- tests/test-check-dead-code.sh | 40 +++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/test-check-dead-code.sh b/tests/test-check-dead-code.sh index c638896..b16d5e9 100755 --- a/tests/test-check-dead-code.sh +++ b/tests/test-check-dead-code.sh @@ -59,6 +59,16 @@ mkbroken() { printf '%s' "$dir" } +# A decorated, otherwise-unreferenced handler — the Flask/FastAPI dispatch +# pattern --ignore-decorators exists for (Tower-Finder's backend is exactly +# this shape). vulture can't see the framework's dispatch wiring, so without +# the flag this reads as dead. +mkdirty_decorated() { + local dir; dir="$(mktemp -d)" + printf 'def used():\n return 1\n\n\n@app.route("/foo")\ndef handler_endpoint():\n return "hi"\n\n\nprint(used())\n' >"$dir/main.py" + printf '%s' "$dir" +} + # --- tests that run whether or not vulture is installed --------------------- t_missing_vulture_fails_closed() { @@ -199,6 +209,35 @@ t_whitelist_suppresses_finding() { rm -rf "$dir" } +# --ignore-decorators (check-dead-code.sh:63) is what stops every Flask/ +# FastAPI route handler reading as dead — Tower-Finder's backend is exactly +# this shape and depends on it directly. The script has no flag to disable +# its own --ignore-decorators, so the first half calls vulture directly +# with the script's other options (same EXCLUDE, same --min-confidence, no +# --ignore-decorators) to establish the finding is real; the second half +# runs the actual script and proves the flag suppresses it. As with the +# whitelist test, both halves are required — the second half alone would +# also pass if the scan found nothing at all. +t_ignore_decorators_suppresses_handlers() { + local dir out rc + dir="$(mkdirty_decorated)" + out="$(cd "$dir" && vulture . --min-confidence 60 \ + --exclude ".venv,scripts,htmlcov,__pycache__,node_modules,build,dist,*.egg-info" 2>&1)"; rc=$? + if [ "$rc" -eq 3 ] && [[ "$out" == *"handler_endpoint"* ]]; then + ok "decorators test: scan without --ignore-decorators finds the handler dead" + else + bad "decorators test: scan without --ignore-decorators finds the handler dead" "rc=$rc out=$out" + fi + + out="$(cd "$dir" && bash "$SCRIPT" 2>&1)"; rc=$? + if [ "$rc" -eq 0 ] && [[ "$out" == *"no dead code found"* ]]; then + ok "--ignore-decorators suppresses the decorated handler" + else + bad "--ignore-decorators suppresses the decorated handler" "rc=$rc out=$out" + fi + rm -rf "$dir" +} + t_positional_target_scopes_scan() { local root out rc root="$(mktemp -d)" @@ -230,6 +269,7 @@ if command -v vulture >/dev/null 2>&1; then t_positional_target_scopes_scan t_vulture_failure_propagates t_whitelist_suppresses_finding + t_ignore_decorators_suppresses_handlers elif [ "${REQUIRE_VULTURE:-}" = "1" ]; then # A skipped suite reporting success is the same class of bug as the gate # this repo hosts: silence read as a clean result. CI sets