From e6e47fb03935ea356475374001b6b0f288460c4a Mon Sep 17 00:00:00 2001 From: znerol2 <6800371+znerol2@users.noreply.github.com> Date: Mon, 26 Oct 2026 07:25:26 +0100 Subject: [PATCH] fix(comments): require a complete status record The scanner records each command's exit status in a file and reads it back. A write that stopped after the first digit left a bare number, which read as a valid status and could pass the check unchecked. Write the status as " done" with a newline, and accept only that exact form. The reader keeps the final newline by appending a sentinel before command substitution strips trailing newlines. Anything else fails with the existing "could not record the exit status" error. The self-test plants partial records and expects each one to fail. Refs dmfdeploy/dmfdeploy#768 Signed-off-by: znerol2 <6800371+znerol2@users.noreply.github.com> --- bin/check-review-comments.sh | 52 ++++++++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/bin/check-review-comments.sh b/bin/check-review-comments.sh index 2038fe6..eced0c5 100755 --- a/bin/check-review-comments.sh +++ b/bin/check-review-comments.sh @@ -390,6 +390,39 @@ self_test() { done rm -rf "$shim" + # A status write that stops part way leaves a record without its end marker. + # The git shim plants the partial record in a read-only file just as the + # targeted git grep starts, so the script's own write fails and the partial + # record stays. The planted file is clean, so a check that accepted the + # partial record would pass. + shim=$(mktemp -d) + # shellcheck disable=SC2016 + printf '%s\n' '#!/bin/bash' \ + 'd=$("$REAL_MKTEMP" "$@") || exit' \ + 'printf "%s\n" "$d" > "$WORK_NOTE"' \ + 'printf "%s\n" "$d"' > "$shim/mktemp" + # shellcheck disable=SC2016 + printf '%s\n' '#!/bin/bash' \ + 'if [ "$1" = grep ]; then for a in "$@"; do' \ + ' if [ "$a" = -nIiE ]; then f="$(cat "$WORK_NOTE")/grep.rc"; printf "%s" "$PARTIAL" > "$f"; chmod 444 "$f"; fi' \ + 'done; fi' \ + "exec \"$real_git\" \"\$@\"" > "$shim/git" + chmod +x "$shim/mktemp" "$shim/git" + local partial + for partial in '1' '1 do' '1 done'; do + git -C "$tmp" rm -q -r -f --ignore-unmatch -- . >/dev/null + printf '// a plain note\n' > "$tmp/a.ts" + git -C "$tmp" add a.ts + rc=0 + out=$(cd "$tmp" && REAL_MKTEMP=$(command -v mktemp) WORK_NOTE="$shim/note" PARTIAL=$partial PATH="$shim:$PATH" bash "$self" 2>&1) || rc=$? + if [ "$rc" -ne 1 ] || ! printf '%s\n' "$out" | grep -q 'could not record the exit status of'; then + echo "FAIL partial status record '$partial': expected exit 1 with an error, got exit $rc: $out"; failed=$((failed + 1)) + else + echo "ok partial status record '$partial' fails the check" + fi + done + rm -rf "$shim" + if [ "$failed" -gt 0 ]; then echo "self-test: $failed case(s) failed." >&2 return 1 @@ -954,19 +987,28 @@ fresh "$work/scan" # redirected block to a separate file. A missing or malformed status file means # the redirection or the status write failed, never that the command ran. Each # status write ends in "|| :" so that a failed write reaches read_status. +# The record is " done" plus a newline. A write that stops part way +# leaves a bare number, which would otherwise read as a real exit status, so +# the end marker is what proves the write finished. # read_status