From 9bfba5bef296417ce7d2e6a67901ad7cf308966e Mon Sep 17 00:00:00 2001
From: David Bednarczyk <davidbzyk@gmail.com>
Date: Sun, 11 Oct 2026 12:31:01 -0400
Subject: [PATCH 1/2] CI review: a git that cannot read the skip marks fails
 the run instead of skipping

Before 2.40, git has no check-attr --source; the error was swallowed, every changed file read as
marked, and the run reported nothing to review and passed. check-attr now writes to a file
first and a failure exits 2. Replicated with a git shim in the new test.
---
 .code-review/run-review.sh        | 12 +++++++++---
 action/run-review.sh              | 12 +++++++++---
 tests/launcher/test_install_ci.py | 32 +++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/.code-review/run-review.sh b/.code-review/run-review.sh
index 47c45de..5fb4e61 100755
--- a/.code-review/run-review.sh
+++ b/.code-review/run-review.sh
@@ -27,14 +27,20 @@ done < <(git -C "$repo" diff --no-ext-diff -z --name-only --diff-filter=ACMRD "$
 # Files .gitattributes marks linguist-generated or linguist-vendored (snapshots, generated output)
 # are skipped, as GitHub collapses them. The marks are read from the base, so a change cannot
 # mark itself unreviewed. check-attr answers two attributes per path, in order.
+# A git that cannot read them (check-attr --source needs 2.40) fails the run: skipping on an
+# error would pass a change nobody reviewed.
 paths=()
 if [ ${#changed[@]} -gt 0 ]; then
+  marks="$work/marks"
+  if ! printf '%s\0' "${changed[@]}" | git -C "$repo" check-attr -z --stdin --source "$base" \
+       linguist-generated linguist-vendored > "$marks"; then
+    echo "code-review: git check-attr could not read the skip marks (git 2.40 or newer is needed)" >&2
+    exit 2
+  fi
   while IFS= read -r -d '' path && IFS= read -r -d '' _ && IFS= read -r -d '' generated \
         && IFS= read -r -d '' _ && IFS= read -r -d '' _ && IFS= read -r -d '' vendored; do
     case "$generated $vendored" in set\ *|true\ *|*\ set|*\ true) ;; *) paths+=("--path=$path") ;; esac
-  done < <(printf '%s\0' "${changed[@]}" \
-           | git -C "$repo" check-attr -z --stdin --source "$base" linguist-generated \
-             linguist-vendored)
+  done < "$marks"
 fi
 if [ ${#paths[@]} -eq 0 ]; then
   echo "code-review: no changed files outside .code-review/ and the generated or vendored ones, nothing to review"
diff --git a/action/run-review.sh b/action/run-review.sh
index 47c45de..5fb4e61 100755
--- a/action/run-review.sh
+++ b/action/run-review.sh
@@ -27,14 +27,20 @@ done < <(git -C "$repo" diff --no-ext-diff -z --name-only --diff-filter=ACMRD "$
 # Files .gitattributes marks linguist-generated or linguist-vendored (snapshots, generated output)
 # are skipped, as GitHub collapses them. The marks are read from the base, so a change cannot
 # mark itself unreviewed. check-attr answers two attributes per path, in order.
+# A git that cannot read them (check-attr --source needs 2.40) fails the run: skipping on an
+# error would pass a change nobody reviewed.
 paths=()
 if [ ${#changed[@]} -gt 0 ]; then
+  marks="$work/marks"
+  if ! printf '%s\0' "${changed[@]}" | git -C "$repo" check-attr -z --stdin --source "$base" \
+       linguist-generated linguist-vendored > "$marks"; then
+    echo "code-review: git check-attr could not read the skip marks (git 2.40 or newer is needed)" >&2
+    exit 2
+  fi
   while IFS= read -r -d '' path && IFS= read -r -d '' _ && IFS= read -r -d '' generated \
         && IFS= read -r -d '' _ && IFS= read -r -d '' _ && IFS= read -r -d '' vendored; do
     case "$generated $vendored" in set\ *|true\ *|*\ set|*\ true) ;; *) paths+=("--path=$path") ;; esac
-  done < <(printf '%s\0' "${changed[@]}" \
-           | git -C "$repo" check-attr -z --stdin --source "$base" linguist-generated \
-             linguist-vendored)
+  done < "$marks"
 fi
 if [ ${#paths[@]} -eq 0 ]; then
   echo "code-review: no changed files outside .code-review/ and the generated or vendored ones, nothing to review"
diff --git a/tests/launcher/test_install_ci.py b/tests/launcher/test_install_ci.py
index 9ded4c2..c07e9da 100644
--- a/tests/launcher/test_install_ci.py
+++ b/tests/launcher/test_install_ci.py
@@ -375,3 +375,35 @@ def test_the_install_carries_every_module_the_wrapper_runs(workdir):
 
     for module in sorted(set(re.findall(r"-m (review_core[\w.]+)", script))):
         assert (dest / "src" / (module.replace(".", "/") + ".py")).is_file(), module
+
+
+def test_a_git_that_cannot_read_the_marks_fails_the_run_instead_of_skipping(workdir, tmp_path):
+    """Git before 2.40 has no `check-attr --source`: the run must fail, not report nothing to
+    review and pass (the change would go unreviewed)."""
+    bundle.install(workdir)
+    git = ["git", "-C", str(workdir), "-c", "user.name=t", "-c", "user.email=t@t"]
+    subprocess.run([*git, "add", "-A"], check=True, capture_output=True)
+    subprocess.run([*git, "commit", "-q", "-m", "install"], check=True, capture_output=True)
+    subprocess.run([*git, "tag", "installed"], check=True, capture_output=True)
+    (workdir / "src.py").write_text("y = 2\n")
+    subprocess.run([*git, "add", "-A"], check=True, capture_output=True)
+    subprocess.run([*git, "commit", "-q", "-m", "code"], check=True, capture_output=True)
+    shim = tmp_path / "bin"
+    shim.mkdir()
+    real = subprocess.run(["which", "git"], capture_output=True, text=True).stdout.strip()
+    (shim / "git").write_text(
+        "#!/bin/bash\n"
+        'for a in "$@"; do\n'
+        '  [ "$a" = check-attr ] && { echo "unknown option: --source" >&2; exit 129; }\n'
+        "done\n"
+        f'exec {real} "$@"\n'
+    )
+    (shim / "git").chmod(0o755)
+    env = {"PATH": f"{shim}:{os.environ['PATH']}", "HOME": str(tmp_path),
+           "RUNNER_TEMP": str(tmp_path / "rt"), "GITHUB_WORKSPACE": str(workdir),
+           "CR_BASE": "installed", "CR_HEAD": "HEAD"}  # fmt: skip
+    done = subprocess.run(["bash", str(workdir / ".code-review" / "run-review.sh")], env=env,
+                          capture_output=True, text=True)  # fmt: skip
+    assert "nothing to review" not in done.stdout
+    assert done.returncode != 0
+    assert "check-attr" in done.stderr

From 967e2c96047427f43f18380b9cba05c0f0665e78 Mon Sep 17 00:00:00 2001
From: David Bednarczyk <davidbzyk@gmail.com>
Date: Sun, 11 Oct 2026 12:34:08 -0400
Subject: [PATCH 2/2] Test: assert the exit status the run promises (2)

---
 tests/launcher/test_install_ci.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/launcher/test_install_ci.py b/tests/launcher/test_install_ci.py
index c07e9da..9c2fa74 100644
--- a/tests/launcher/test_install_ci.py
+++ b/tests/launcher/test_install_ci.py
@@ -405,5 +405,5 @@ def test_a_git_that_cannot_read_the_marks_fails_the_run_instead_of_skipping(work
     done = subprocess.run(["bash", str(workdir / ".code-review" / "run-review.sh")], env=env,
                           capture_output=True, text=True)  # fmt: skip
     assert "nothing to review" not in done.stdout
-    assert done.returncode != 0
+    assert done.returncode == 2
     assert "check-attr" in done.stderr