fix(609): close round-3 review findings

All four LOW; no HIGH remained. The subject_of fix from round 2 was confirmed
correct across every message shape and all 38 historical commits.

1. The old-git compat check was a VALUE sentinel: it blanked any trailer whose
   value happened to equal the atom string, so a legitimate
   `Decisions-Edit: %(trailers:key=Decisions-Edit,valueonly)` was silently
   discarded. Replaced with a capability probe on `git --version` (>= 2.22).
   Detecting by version instead of by sniffing output removes the collision
   class entirely rather than narrowing it. Unknown/unparseable version resolves
   to False -- trailers ignored, subject-only matching -- which is the safe
   direction: a trailer-only token not arming is an annoyance, whereas reading an
   unexpanded atom as a value would falsely arm and disable the guard.

2. The compat test never called `_token_armed`, so it pinned nothing -- deleting
   the guard would have left it green. Replaced with three tests that drive the
   real function through a stubbed `_run`, covering old git (trailers ignored),
   modern git (trailer arms), a tokened subject surviving an unusable trailer,
   and version-string parsing incl. unparseable input. Proven non-vacuous:
   forcing the probe True makes the old-git test fail.

3. `_repo()` still ignored return codes from init/config/base-commit and never
   checked that the base sha resolved, so a rejected base could leave it
   returning ("", <root sha>) and negative range tests would pass vacuously. All
   commands are now checked and the base sha is asserted to be a full 40 chars.

4. docs/decisions.md line 65 still said "append it, as every prior use does".
   37 of 38 append; docs(434) is mid-subject.

docs/decisions.md

@@ -62,8 +62,9 @@ updating metadata fields, regenerating the catalog) are token-free; the validato
 legitimate structurally instead of gating on the token.
 
 **Where the token counts (ersatztv#609).** It is recognized in a commit's **subject line** (the
-established form — append it, as every prior use does) or as a **`Decisions-Edit: <reason>`** git
-trailer. It is *not* recognized anywhere else in the message body, so you can safely write *about*
+established form — all 38 prior uses carry it there, 37 appended) or as a
+**`Decisions-Edit: <reason>`** git trailer. It is *not* recognized anywhere else in the message
+body, so you can safely write *about*
 the mechanism in a commit body without arming it. That scoping is the fix for a real failure: the
 check used to be a bare substring over the whole message, so a commit merely describing the token
 armed it and silently suppressed every rationale-rewrite comparison — the job still printed

scripts/decisions_validate.py

@@ -239,7 +239,12 @@ _TRAILER_KEY = "Decisions-Edit"
 _NUL = "\x00"  # what we SPLIT the output on
 _NUL_FMT = "%x00"  # what goes in the --format string; a literal NUL in argv raises ValueError
 _FIELDS_PER_COMMIT = 3
-_UNEXPANDED_ATOM = f"%(trailers:key={_TRAILER_KEY},valueonly)"  # exact, not a prefix
+# git < 2.22 cannot expand %(trailers:key=…,valueonly) and echoes the atom verbatim with exit 0.
+# Detect that by VERSION, not by sniffing the output for the atom text: a value-based sentinel can
+# collide with a legitimate trailer whose value happens to be that exact string, silently discarding
+# a real token (#609 round 3).
+_MIN_TRAILER_GIT = (2, 22)
+_GIT_VERSION_RE = re.compile(r"(\d+)\.(\d+)")
 
 
 def token_armed_in(subject: str, trailer: str) -> bool:
@@ -274,6 +279,20 @@ def subject_of(body: str) -> str:
     return body.split("\n", 1)[0].strip()
 
 
+def _git_supports_trailer_atom() -> bool:
+    """Whether this git expands `%(trailers:key=…,valueonly)` (2.22+). Unknown → False.
+
+    Unknown resolving to False means trailers are ignored and matching falls back to the subject,
+    which is the safe direction: a genuine trailer-only token silently not arming is an annoyance,
+    whereas treating an unexpanded atom as a value would falsely arm and disable the guard.
+    """
+    out = _run(["git", "--version"])
+    if not out:
+        return False
+    m = _GIT_VERSION_RE.search(out)
+    return bool(m) and (int(m.group(1)), int(m.group(2))) >= _MIN_TRAILER_GIT
+
+
 def _token_armed(mb: str, head: str) -> bool:
     """True if ANY commit in mb..head carries the token. Anything unexpected → False (guard runs)."""
     out = _run(
@@ -288,6 +307,7 @@ def _token_armed(mb: str, head: str) -> bool:
         # On git trouble the token is NOT armed, so the guard still runs. Failing the other way
         # would silently disable it — the bug this whole function exists to fix.
         return False
+    trailers_usable = _git_supports_trailer_atom()
     parts = out.split(_NUL)
     if parts and not parts[-1].strip():
         parts.pop()  # trailing inter-record newline after the final NUL
@@ -297,13 +317,9 @@ def _token_armed(mb: str, head: str) -> bool:
         return False
     for i in range(0, len(parts), _FIELDS_PER_COMMIT):
         body, trailer = parts[i + 1], parts[i + 2]
-        # A git older than 2.22 can't expand %(trailers:...) and echoes the atom verbatim with
-        # exit 0, which would read as a non-empty trailer and falsely arm. Neutralise just the
-        # trailer on an EXACT match — a prefix match would swallow a legitimate value, and
-        # `continue` would additionally discard a perfectly good tokened SUBJECT on that commit.
-        if trailer.strip() == _UNEXPANDED_ATOM:
-            trailer = ""
-        if token_armed_in(subject_of(body), trailer):
+        # On a git too old to expand the atom, ignore the trailer field entirely and fall back to
+        # subject-only matching. The SUBJECT is always honoured either way.
+        if token_armed_in(subject_of(body), trailer if trailers_usable else ""):
             return True
     return False
 

scripts/tests/test_decisions_validate.py

@@ -526,13 +526,13 @@ def _repo(tmp_path, messages: list[str]) -> tuple[str, str]:
     def g(*a, **kw):
         return subprocess.run(["git", *a], cwd=tmp_path, capture_output=True, text=True, **kw)
 
-    g("init", "-q", ".")
-    g("config", "user.email", "t@e")
-    g("config", "user.name", "t")
+    for cmd in (("init", "-q", "."), ("config", "user.email", "t@e"), ("config", "user.name", "t")):
+        assert g(*cmd).returncode == 0, f"setup failed: {cmd}"
     (tmp_path / "f").write_text("base\n")
-    g("add", "-A")
-    g("commit", "-qm", "base")
+    assert g("add", "-A").returncode == 0
+    assert g("commit", "-qm", "base").returncode == 0, "base commit rejected"
     base = g("rev-parse", "HEAD").stdout.strip()
+    assert len(base) == 40, f"base sha not resolved ({base!r}) — range tests would be vacuous"
     for i, msg in enumerate(messages):
         (tmp_path / "f").write_text(f"{i}\n")
         assert g("add", "-A").returncode == 0
@@ -645,17 +645,49 @@ def test_integration_leading_blank_line_does_not_promote_body_to_subject(tmp_pat
         os.chdir(cwd)
 
 
-def test_unexpanded_trailer_atom_is_matched_exactly_not_by_prefix():
-    """The old-git compat guard must be an EXACT match.
+def test_old_git_falls_back_to_subject_only(monkeypatch):
+    """Exercises the real `_token_armed` branch, not just the predicate.
 
-    A prefix match would swallow a legitimate trailer value that happens to start with the same
-    text. Neutralisation lives in `_token_armed` (it blanks the trailer field); `token_armed_in`
-    stays a pure predicate where any non-empty trailer arms.
+    Round-3 review caught that the previous version of this test never called `_token_armed`, so
+    deleting the guard entirely would have left it green.
     """
-    atom = dv._UNEXPANDED_ATOM
-    assert atom == "%(trailers:key=Decisions-Edit,valueonly)", "guard must name the exact atom"
-    # pure predicate: any non-empty trailer arms, including one starting with the atom text
-    assert dv.token_armed_in("docs: plain subject", atom + " and more")
-    assert not dv.token_armed_in("docs: plain subject", "")
-    # and the guard blanks ONLY an exact match, so a tokened subject on that same commit survives
-    assert dv.token_armed_in("fix: real [decisions-edit]", "")
+    calls = {}
+
+    def fake_run(args):
+        if args[:2] == ["git", "--version"]:
+            return calls["version"]
+        # one commit: sha, body, trailer — the trailer field is the UNEXPANDED atom, as an old
+        # git would emit it verbatim.
+        return "sha\x00docs: plain subject\n\x00%(trailers:key=Decisions-Edit,valueonly)\x00"
+
+    monkeypatch.setattr(dv, "_run", fake_run)
+
+    calls["version"] = "git version 2.20.1\n"  # too old: trailers ignored, subject-only
+    assert not dv._token_armed("a", "b"), "unexpanded atom must not be read as a trailer value"
+
+    calls["version"] = "git version 2.39.5\n"  # new enough: the field is a real value
+    assert dv._token_armed("a", "b"), "a modern git's trailer value must still arm"
+
+
+def test_old_git_still_honours_a_tokened_subject(monkeypatch):
+    def fake_run(args):
+        if args[:2] == ["git", "--version"]:
+            return "git version 2.20.1\n"
+        return "sha\x00fix: real [decisions-edit]\n\x00%(trailers:key=Decisions-Edit,valueonly)\x00"
+
+    monkeypatch.setattr(dv, "_run", fake_run)
+    assert dv._token_armed("a", "b"), "subject must arm even when trailers are unusable"
+
+
+def test_git_version_probe_parses_and_fails_safe(monkeypatch):
+    for out, want in [
+        ("git version 2.39.5\n", True),
+        ("git version 2.22.0\n", True),
+        ("git version 2.21.9\n", False),
+        ("git version 3.0.0\n", True),
+        ("", False),
+        (None, False),
+        ("not a version string", False),
+    ]:
+        monkeypatch.setattr(dv, "_run", lambda _a, _o=out: _o)
+        assert dv._git_supports_trailer_atom() is want, out