diff --git a/scripts/pr-state b/scripts/pr-state index 426811d36d..f04d0951ad 100755 --- a/scripts/pr-state +++ b/scripts/pr-state @@ -393,7 +393,7 @@ review_evidence_json() { markdown_prose_lines | any( . - | gsub("^[[:space:]]*(?:(?:[-*+]\\s+)|(?:[0-9]+\\.\\s+))?(?:\\*\\*|__|_|\\*)?"; "") + | sub("^[[:space:]]*(?:(?:[-*+]\\s+)|(?:[0-9]+\\.\\s+))?(?:\\*\\*|__|_|\\*)?"; "") | test("^(?:blocked|changes[ _-]?requested|action required|must fix|required fix|please fix)\\b"; "i") ); def review_eligibility: @@ -752,7 +752,7 @@ fetch_review_threads_nodes_json() { } } }' \ - -f owner="$owner" -f repo="$repo" -F pr="$pr_number" "${cursor_args[@]}")"; then + -f owner="$owner" -f repo="$repo" -F pr="$pr_number" ${cursor_args[@]+"${cursor_args[@]}"})"; then rm -f "$nodes_file" return 1 fi diff --git a/scripts/pr-state.test.sh b/scripts/pr-state.test.sh index 163a29aa5d..15a1ca8728 100644 --- a/scripts/pr-state.test.sh +++ b/scripts/pr-state.test.sh @@ -1484,6 +1484,85 @@ test_jq_file_args_avoid_process_substitution() { } test_jq_file_args_avoid_process_substitution +# Both of these guard against host-toolchain failures that CI cannot reproduce: +# the runners ship bash 5 and jq 1.7, while macOS ships bash 3.2 as /bin/bash and +# jq 1.6 is still widely installed. Each bug made pr-state report a dirty PR as +# clean, so the invariant is asserted structurally rather than left untested. +test_array_expansions_are_safe_under_set_u() { + local offenders + # The guarded form contains "${arr[@]}" inside it, so lines carrying the + # [@]+ guard are not offenders. + offenders="$(grep -n '"\${[A-Za-z_][A-Za-z0-9_]*\[@\]}"' "$SCRIPT" | grep -v '\[@\]+' || true)" + if [ -n "$offenders" ]; then + printf '%s\n' "$offenders" >&2 + fail "array expansions use \${arr[@]+\"\${arr[@]}\"} (bash 3.2 errors on an empty array under set -u)" + fi + + # And the guarded form itself must be safe both empty and populated. + if ! bash -c 'set -u; a=(); : ${a[@]+"${a[@]}"}' 2>/dev/null; then + fail "the guarded expansion is safe for an empty array" + fi + if [ "$(bash -c 'set -u; a=(one two); printf "%s," ${a[@]+"${a[@]}"}')" != "one,two," ]; then + fail "the guarded expansion preserves array elements" + fi + + pass "array expansions survive bash 3.2 under set -u" +} + +test_anchored_prefix_strip_cannot_loop() { + # A gsub whose pattern is entirely optional matches the empty string at every + # position, which never terminates on jq 1.6. The strips here are ^-anchored, + # so sub() is both the terminating and the semantically correct operator. + # The hazard is a pattern that can match the empty string. A trailing optional + # group is the realistic shape of that: gsub("...(?:x)?"; ...) matches nothing + # at every position. This source guard is conservative. It flags any gsub + # pattern that contains `?`, including safe patterns that contain required atoms. + local offenders + offenders="$(grep -n 'gsub("[^"]*?";' "$SCRIPT" || true)" + if [ -n "$offenders" ]; then + printf '%s\n' "$offenders" >&2 + fail "a gsub pattern ending in an optional group can match empty and loops on jq 1.6; use sub()" + fi + + # The filter must terminate quickly whatever jq is installed. Use a background + # watchdog because macOS does not provide the GNU `timeout` command by default. + local start elapsed tmp timeout_file jq_pid watchdog_pid jq_status + make_tmp_dir tmp + timeout_file="$tmp/jq-timeout" + start=$(date +%s) + jq -n '"- **blocked** on review" | sub("^[[:space:]]*(?:(?:[-*+]\\s+)|(?:[0-9]+\\.\\s+))?(?:\\*\\*|__|_|\\*)?"; "")' >/dev/null 2>&1 & + jq_pid=$! + ( + sleep 5 + if kill -0 "$jq_pid" 2>/dev/null; then + : >"$timeout_file" + kill "$jq_pid" 2>/dev/null || true + fi + ) & + watchdog_pid=$! + + if wait "$jq_pid"; then + jq_status=0 + else + jq_status=$? + fi + kill "$watchdog_pid" 2>/dev/null || true + wait "$watchdog_pid" 2>/dev/null || true + + if [ -e "$timeout_file" ]; then + fail "the prefix strip terminates promptly (timed out after 5s)" + fi + if [ "$jq_status" -ne 0 ]; then + fail "the prefix strip evaluates successfully" + fi + elapsed=$(( $(date +%s) - start )) + if [ "$elapsed" -gt 5 ]; then + fail "the prefix strip terminates promptly (took ${elapsed}s)" + fi + + pass "anchored prefix strips terminate on jq 1.6" +} + test_snapshot_happy_path test_old_head_review_does_not_qualify test_exact_head_selected_review_qualifies @@ -1524,3 +1603,5 @@ test_job_log_mode_unpacks_zip_responses test_comment_mode_returns_full_review_comment test_comment_mode_reports_fetch_failure test_comment_mode_rejects_incompatible_flags +test_array_expansions_are_safe_under_set_u +test_anchored_prefix_strip_cannot_loop