From e4a8b6ffb4bd702e7cbbdb472a7665bf9c71b225 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 30 Aug 2026 12:37:52 +0200 Subject: [PATCH] =?UTF-8?q?fix(858,859):=20review=20round=204=20=E2=80=94?= =?UTF-8?q?=20the=20corrected=20sentence=20had=20a=20twin,=20and=20its=20t?= =?UTF-8?q?win=20had=20two=20more?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 returned NOT-MERGEABLE with Blocker and High both EMPTY: one Medium and two Lows, all prose, no behavioural defect found across four attack directions. MEDIUM — round 3 corrected "deleting it left all 57 tests green" in the hook and left the IDENTICAL sentence, with the identical wrong number, in the test file — a line this branch had added. Measured: 64, not 57. A comment fix applied to one of two sites, inside the PR whose subject is arms that state what did not happen. Fix-one-path-then-check-its-twin, one level up from the code. Sweeping the CLASS rather than the instance then found two more, both pre-existing and both made MORE wrong by this branch's new tests ("all 71 tests", "all 29 tests" — now 111 and 64). Fixed here because this PR falsified them; all three now state the invariant ("the WHOLE suite") with a parenthetical saying why the count was dropped. A fourth lives in test_ci_dropped_step_guard.py, which this PR neither touches nor falsifies — left alone and filed as ersatztv#895 with a candidate detector, since four instances of a mechanically-detectable class is the criterion for a check rather than another sweep. LOW — "THE PAYLOAD MUST BE AN ARRAY before the classifier sees it" is false on jq 1.6, the version scripts/jq-preflight.sh pins for script-tests: `jq -e` over EMPTY input exits 0 there, so an empty 200 body is not diverted and does reach the classifier. Measured on 1.6 and 1.8.2 — the class word is `unreadable` either way, so the OUTCOME is version-independent and the comment now states the outcome rather than the mechanism (ci.jq-version-contract, ersatztv#643). LOW — scripts/hook-fire-log.sh's own USAGE recipe still showed the ${CLAUDE_PROJECT_DIR:-...} form that process.hook-resolves-inputs-from-repo-root bans. That header is what a new hook copies, so leaving it would keep minting the defect after #891 sweeps the existing twelve. Corrected, with the reason and the measurement beside it. Suites: 193 passed, 2 skipped. --- scripts/check-required-contexts.sh | 16 ++++++++++++---- scripts/hook-fire-log.sh | 10 +++++++++- scripts/tests/test_check_required_contexts.py | 6 ++++-- .../tests/test_merge_consent_required_check.py | 8 +++++--- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/scripts/check-required-contexts.sh b/scripts/check-required-contexts.sh index db93c3b28..e29b9fdea 100755 --- a/scripts/check-required-contexts.sh +++ b/scripts/check-required-contexts.sh @@ -90,13 +90,21 @@ fi classifier="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/lib/branch-rule-classifier.jq" [ -r "$classifier" ] || { echo "classifier not readable: $classifier" >&2; exit 2; } -# THE PAYLOAD MUST BE AN ARRAY before the classifier sees it, the same gate the merge hook applies +# THE PAYLOAD MUST BE AN ARRAY to produce a finding, the same gate the merge hook applies # before its own call. Without it a JSON OBJECT reached the classifier, matched no rule, and this # script printed `nomatch` — "no rule governs the branch at all (protection removed)", a positive # claim about the server's configuration derived from a payload that is not the shape it must be. -# `{}` produced exactly that, measured. Pre-existing rather than introduced by ersatztv#859, and -# fixed here because that issue is precisely about arms asserting findings they did not establish; -# `unreadable` already means "no comparison was made", which is the truth for a non-array body. +# `{}` produced exactly that, measured; removing the gate also turns `{"a":{...}}` into a confident +# `drift`. Pre-existing rather than introduced by ersatztv#859, and fixed here because that issue is +# precisely about arms asserting findings they did not establish; `unreadable` already means "no +# comparison was made", which is the truth for a non-array body. +# +# "To produce a finding" rather than "before the classifier sees it", and the distinction is the +# `ci.jq-version-contract` gap (ersatztv#643): on jq 1.6 — the version `scripts/jq-preflight.sh` +# pins for `script-tests` — `jq -e` over EMPTY input exits 0, so an empty 200 body is not diverted +# here and does reach the classifier. Measured on both 1.6 and 1.8.2: the class word is `unreadable` +# either way, because the classifier then produces nothing and the catch-all answers. The outcome is +# version-independent; only the mechanism differs, so the comment states the outcome. if ! printf '%s' "$payload" | jq -e 'type == "array"' >/dev/null 2>&1; then printf 'unreadable\n'; exit 0 fi diff --git a/scripts/hook-fire-log.sh b/scripts/hook-fire-log.sh index bf637d792..2be896c5d 100755 --- a/scripts/hook-fire-log.sh +++ b/scripts/hook-fire-log.sh @@ -33,9 +33,17 @@ # replay is the first act of the exit path, and the original exit code is re-raised explicitly. # # USAGE (write side) — the first two lines of every hook, before anything reads stdin: -# . "${CLAUDE_PROJECT_DIR:-$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../.." && pwd)}/scripts/hook-fire-log.sh" +# . "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../.." && pwd)/scripts/hook-fire-log.sh" # etv_hook_fire_begin [label] # +# SELF-LOCATED, and the `${CLAUDE_PROJECT_DIR:-...}` form this recipe used to show is BANNED — +# `process.hook-resolves-inputs-from-repo-root` (ersatztv#858). This line is SOURCED, so whatever it +# names runs as code inside the hook before the hook decides anything; a file of this name in an +# env-var-designated tree can print a decision and exit 0. Measured on the merge-consent hook: it +# granted the merge outright. The recipe is corrected here because the header is what a new hook +# copies, so leaving it would keep minting the defect after ersatztv#891 sweeps the existing hooks. +# Twelve hooks still carry the old form; that sweep is #891, not this line. +# # USAGE (read side): # scripts/hook-fire-log.sh report # this session (or every session, see below) # scripts/hook-fire-log.sh report --all # every session in the log dir diff --git a/scripts/tests/test_check_required_contexts.py b/scripts/tests/test_check_required_contexts.py index 86ceff14b..e7a6c0c84 100644 --- a/scripts/tests/test_check_required_contexts.py +++ b/scripts/tests/test_check_required_contexts.py @@ -96,8 +96,10 @@ def test_a_rule_with_status_checks_DISABLED_is_drift_against_a_nonempty_snapshot `enable_status_check: false` means Gitea requires nothing on the branch whatever `status_check_contexts` still holds. Type-checking the flag and ignoring its VALUE certifies disabled protection as a current mirror — measured: with that clause removed the same payload - returns `match`, and all 71 tests across this file and the merge-consent file stayed green, - because `rule()`'s `enabled=` parameter had no call site that ever passed False. + returns `match`, and the ENTIRE suite across this file and the merge-consent file stayed green, + because `rule()`'s `enabled=` parameter had no call site that ever passed False. (Stated as an + invariant: this sentence carried a test COUNT until ersatztv#859, and a count is falsified by + the next person to add a test — which is exactly what that PR did to it.) """ assert run(json.dumps([rule(list(SNAP_CONTEXTS), enabled=False)]))[1] == "drift" diff --git a/scripts/tests/test_merge_consent_required_check.py b/scripts/tests/test_merge_consent_required_check.py index 50018d34e..edd994901 100644 --- a/scripts/tests/test_merge_consent_required_check.py +++ b/scripts/tests/test_merge_consent_required_check.py @@ -818,7 +818,8 @@ def test_the_precedence_check_runs_even_when_an_exactly_named_rule_EXISTS(hook): def test_a_GLOB_rule_that_could_outrank_an_exact_one_wins_and_ASKS(hook): """The arm ORDER, pinned. Without this the reorder is invisible to the suite — swapping the arms - back left all 29 tests green, which is how an unproven change ships. + back left the WHOLE suite green, which is how an unproven change ships. (An invariant, not a + count: this sentence carried one until ersatztv#859, whose own new tests falsified it.) Gitea picks the governing rule with `GetFirstMatched` over a list sorted by Priority and THEN by plain-name-ness, so a glob rule can outrank an exactly-named one. Here `main` requires @@ -1203,8 +1204,9 @@ def test_a_rule_with_an_UNUSABLE_NAME_asks_and_names_that_cause(hook, shape): "the hook still claims the full rule list was read and none matched, about a rule it could not read at all" ) # And it must NOT be collapsed back into the generic parse failure. That sentinel was measured - # to be a no-op when both classes shared it: deleting the arm left all 57 tests green, because - # nothing downstream could tell them apart. + # to be a no-op when both classes shared it: deleting the arm left the WHOLE suite green, + # because nothing downstream could tell them apart. (An invariant, not a count — the number this + # sentence first carried was already wrong by seven when a later review re-ran it.) assert "could not parse" not in out["permissionDecisionReason"], ( "the declared `unreadable` class was folded back into the generic parse-failure sentinel, " "which no observation can distinguish from the catch-all"