#417 - hooks/tests: release-gate.test.sh still scores a crashed gate as "allow" - and the audit scope is an open question - sixfold-space/madtea

hexajon commented

2026-08-17 15:49:19 +00:00

Owner

Follow-up to #366. The harness hardening there covered the eight suites that invoke the guard as bash "$HOOK". hooks/tests/release-gate.test.sh has the identical defect and was missed, because it names its subject $GATE rather than $HOOK and so fell outside the grep that produced the file list.

hooks/tests/release-gate.test.sh:112-118:

run_gate() {
  local repo="$1" stdin_json="$2"
  ( cd "$repo" && CLAUDE_PROJECT_DIR="$repo" RELEASE_GATE_SCORE=10 \
    bash "$GATE" <<<"$stdin_json" 2>/dev/null )
}

Same three problems #366 describes: stderr discarded, exit status never inspected, and scoring that treats absent output as a permissive result. Demonstrated against a gate stub that exits 9: eight expect-ALLOW cases still report PASS, because a gate that never ran looks exactly like a gate that allowed.

This one guards the release gate specifically, so the blind spot sits on the path that decides whether a release may proceed.

Open question - how wide should the sweep be

Not settled, and worth deciding before the work starts rather than after:

Fix release-gate.test.sh alone, mirroring the lib-hook-harness.sh treatment.
Audit every file under hooks/tests/ for the general shape: any subprocess-under-test invoked with 2>/dev/null and no exit-status check, scored by absence. This does not assume $HOOK and $GATE are the only two spellings. It is the option that answers "what else did the grep miss", which is the actual lesson of this follow-up: the file list came from a pattern that matched a naming convention, not from the defect.

My recommendation is (2), because the miss here was caused by scoping on a name rather than on the behaviour, and repeating that method would likely repeat the miss. The cost is a wider diff across suites that are currently green.

Acceptance criteria

A gate or hook that exits non-zero, or is never reached, is reported as an error naming the cause, never as allow or block.
Captured stderr is surfaced on a failing case.
Expect-allow assertions require positive evidence the subject actually ran.
Whatever scope is chosen, the resulting file list is derived from the defect shape and stated explicitly, so the next reader can tell what was audited and what was not.
Suites keep their existing pass counts.

Follow-up to #366. The harness hardening there covered the eight suites that invoke the guard as `bash "$HOOK"`. `hooks/tests/release-gate.test.sh` has the identical defect and was missed, because it names its subject `$GATE` rather than `$HOOK` and so fell outside the grep that produced the file list. `hooks/tests/release-gate.test.sh:112-118`: ```bash run_gate() { local repo="$1" stdin_json="$2" ( cd "$repo" && CLAUDE_PROJECT_DIR="$repo" RELEASE_GATE_SCORE=10 \ bash "$GATE" <<<"$stdin_json" 2>/dev/null ) } ``` Same three problems #366 describes: stderr discarded, exit status never inspected, and scoring that treats absent output as a permissive result. Demonstrated against a gate stub that exits 9: eight expect-ALLOW cases still report PASS, because a gate that never ran looks exactly like a gate that allowed. This one guards the release gate specifically, so the blind spot sits on the path that decides whether a release may proceed. ## Open question - how wide should the sweep be Not settled, and worth deciding before the work starts rather than after: 1. Fix `release-gate.test.sh` alone, mirroring the `lib-hook-harness.sh` treatment. 2. Audit every file under `hooks/tests/` for the general shape: any subprocess-under-test invoked with `2>/dev/null` and no exit-status check, scored by absence. This does not assume `$HOOK` and `$GATE` are the only two spellings. It is the option that answers "what else did the grep miss", which is the actual lesson of this follow-up: the file list came from a pattern that matched a naming convention, not from the defect. My recommendation is (2), because the miss here was caused by scoping on a name rather than on the behaviour, and repeating that method would likely repeat the miss. The cost is a wider diff across suites that are currently green. ## Acceptance criteria - A gate or hook that exits non-zero, or is never reached, is reported as an error naming the cause, never as `allow` or `block`. - Captured stderr is surfaced on a failing case. - Expect-allow assertions require positive evidence the subject actually ran. - Whatever scope is chosen, the resulting file list is derived from the defect shape and stated explicitly, so the next reader can tell what was audited and what was not. - Suites keep their existing pass counts.

hexajon added the

bug

testing

status/needs-decision

labels

2026-08-17 15:49:19 +00:00

hexajon commented

2026-08-17 15:49:25 +00:00

Author

Owner

Sticking with the body's lean: option (2), the behavioral audit across hooks/tests/. Any subprocess-under-test run with 2>/dev/null and no exit-status check, scored by absence, is in scope, not just the two known spellings ($HOOK and $GATE). The miss here came from scoping on a name, so repeating that method would repeat the miss. The cost is a wider diff, but it preserves the green suites.

Open question before the work starts: whether the wider diff is worth it now, or whether release-gate.test.sh gets the point fix first and the audit follows as its own change. My lean is the full audit in one pass, because splitting it leaves the same "what else did the grep miss" gap open in between.

Sticking with the body's lean: option (2), the behavioral audit across `hooks/tests/`. Any subprocess-under-test run with `2>/dev/null` and no exit-status check, scored by absence, is in scope, not just the two known spellings (`$HOOK` and `$GATE`). The miss here came from scoping on a name, so repeating that method would repeat the miss. The cost is a wider diff, but it preserves the green suites. Open question before the work starts: whether the wider diff is worth it now, or whether `release-gate.test.sh` gets the point fix first and the audit follows as its own change. My lean is the full audit in one pass, because splitting it leaves the same "what else did the grep miss" gap open in between.

hexajon self-assigned this

2026-08-17 17:45:48 +00:00

hexajon commented

2026-08-27 17:18:59 +00:00

Author

Owner

Real-world reproductions of this defect, in CI.

Two occurrences this session: the cold pull_request gate runs for the #429 merge (680d199) and the #456 merge (f09a263d) both failed on over-threshold-stale with expected deny (score 16 >= 10, no grace), got allow. Both passed the gate on their push runs of the identical SHA, and both pass the full gate locally. Neither commit touches scripts/release-gate.sh. So the gate did not wrongly allow - it produced no "deny" output on those cold runs (a transient failure under CI load, the same load class as #410), and the test scored that absence as allow. The run history shows the same gate-job failure recurring across many older SHAs too.

This is exactly the defect: run_gate runs bash "$GATE" ... 2>/dev/null with stderr discarded and the exit status never inspected, and is_deny treats the absence of a "deny" string as a pass. A gate that crashed or produced nothing is indistinguishable from a gate that allowed, so a transient gate failure surfaces as a false over-threshold-stale / prs-merge-deny failure and red-lights an unrelated PR.

The point fix is ready in shape (capture the gate exit status and stderr; report a non-zero exit or empty output as an error naming the cause; require positive evidence the gate ran for the expect-allow cases - the lib-hook-harness.sh treatment from #366). I am still holding on the open question above - point-fix this file, or the behavioral audit across all of hooks/tests/ in one pass. My lean stays the full audit, but these occurrences argue for landing at least the point fix soon, because it is now causing red CI on unrelated merges.

Real-world reproductions of this defect, in CI. Two occurrences this session: the cold `pull_request` `gate` runs for the #429 merge (680d199) and the #456 merge (f09a263d) both failed on `over-threshold-stale` with `expected deny (score 16 >= 10, no grace), got allow`. Both passed the gate on their `push` runs of the identical SHA, and both pass the full gate locally. Neither commit touches scripts/release-gate.sh. So the gate did not wrongly allow - it produced no `"deny"` output on those cold runs (a transient failure under CI load, the same load class as #410), and the test scored that absence as allow. The run history shows the same `gate`-job failure recurring across many older SHAs too. This is exactly the defect: `run_gate` runs `bash "$GATE" ... 2>/dev/null` with stderr discarded and the exit status never inspected, and `is_deny` treats the absence of a `"deny"` string as a pass. A gate that crashed or produced nothing is indistinguishable from a gate that allowed, so a transient gate failure surfaces as a false `over-threshold-stale` / `prs-merge-deny` failure and red-lights an unrelated PR. The point fix is ready in shape (capture the gate exit status and stderr; report a non-zero exit or empty output as an error naming the cause; require positive evidence the gate ran for the expect-allow cases - the lib-hook-harness.sh treatment from #366). I am still holding on the open question above - point-fix this file, or the behavioral audit across all of hooks/tests/ in one pass. My lean stays the full audit, but these occurrences argue for landing at least the point fix soon, because it is now causing red CI on unrelated merges.

hexajon removed the

status/needs-decision

label

2026-08-27 20:10:18 +00:00

hexajon commented

2026-08-27 20:10:18 +00:00

Author

Owner

Decision: the full behavioral audit across all of hooks/tests/ (option 2), not the point fix alone. Building it now.

hexajon referenced this issue from a pull request that will close it,

2026-08-27 21:28:31 +00:00

test(hooks): audit hooks/tests for absence-scored crashed subjects, harden the affected suites #465