diff --git a/docs/decisions.md b/docs/decisions.md index fe076ca76..da624acd7 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -3943,14 +3943,24 @@ laundering, archive survivors). `removed` and `demoted` still ran, so the job pr and it bit in the worst possible place: that PR had hand-resolved an append-vs-append conflict inside `docs/decisions.md`, which is precisely the operation the guard exists to police. -**Scoping to the subject preserves every historical use.** All twenty prior tokened commits append -`[decisions-edit]` to the subject (or place it mid-subject, as `docs(434):` does); none put it on its -own line, so an "own-line only" rule — the obvious first instinct — would have broken all of them. -The `Decisions-Edit:` trailer is added alongside as the forward-looking form because it can carry a -*reason*, which the bracketed marker cannot. +**Scoping to the subject preserves every historical use.** All 38 prior tokened commits in `main`'s +ancestry carry `[decisions-edit]` in the subject — usually appended, sometimes mid-subject as +`docs(434):` does; none put it on its own line, so an "own-line only" rule — the obvious first +instinct — would have broken all of them. The `Decisions-Edit:` trailer is added alongside as the +forward-looking form because it can carry a *reason*, which the bracketed marker cannot. -**Fail-open posture is unchanged**: if git can't be interrogated the token reads unarmed, so the -guard still runs. Failing the other way would silently disable it, which is the bug being fixed. +**The subject is computed, not taken from git's `%s`.** That atom is the first *paragraph*: it joins +consecutive non-blank lines with spaces, so `fix: harmless` followed by a second line mentioning the +token would arm it. The first line is taken from `%B` instead. Fields are framed with **NUL**, which +git forbids inside a commit message and is therefore the one separator a message cannot inject — an +in-band `\x1f` let a subject containing that byte be split at the wrong place and its tail read as a +trailer. Parsing demands exact arity (a multiple of three fields) and refuses to arm on anything +else, including a `%(trailers:…)` atom echoed literally by a git too old to expand it. + +**Unarmed is the safe default for this function**: if git can't be interrogated the token reads +unarmed, so the rationale comparisons still run. Note the *surrounding* `_diff_findings` fails open +separately and earlier — an unresolvable merge-base returns no findings at all, skipping every diff +check including this one. That predates this record and is unchanged by it. **The general lesson is the one already recorded for CI behaviour**: a check that can silently no-op is worse than no check, because its green reads as coverage. Verify a guard by *measuring that diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index 33ca1947a..9d55425b2 100644 --- a/scripts/decisions_validate.py +++ b/scripts/decisions_validate.py @@ -232,7 +232,14 @@ def _archive_records_at(ref: str): _TRAILER_KEY = "Decisions-Edit" -_REC_SEP, _FIELD_SEP = "\x1e", "\x1f" +# NUL framing, not \x1f/\x1e. git forbids NUL inside a commit message, so it is the ONLY separator +# a crafted (or merely odd) message cannot inject: a subject containing a literal \x1f would +# otherwise be split at the wrong place and its tail read as a trailer — falsely ARMING the token, +# which is the dangerous direction because arming silently disables the guard. +_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 = "%(trailers" def token_armed_in(subject: str, trailer: str) -> bool: @@ -253,25 +260,44 @@ def token_armed_in(subject: str, trailer: str) -> bool: return bool(trailer.strip()) +def subject_of(body: str) -> str: + """The commit's true first line. + + NOT git's `%s`: that atom is the first *paragraph*, joining consecutive non-blank lines with + spaces, so `fix: harmless\\nprose about [decisions-edit]\\n\\n…` would come back as one line + containing the token and arm it. Take the first line ourselves from `%B`. + """ + return body.lstrip("\n").split("\n", 1)[0].strip() + + def _token_armed(mb: str, head: str) -> bool: - """True if ANY commit in mb..head carries the token. Unresolvable git → False (guard runs).""" + """True if ANY commit in mb..head carries the token. Anything unexpected → False (guard runs).""" out = _run( [ "git", "log", - f"--format=%s{_FIELD_SEP}%(trailers:key={_TRAILER_KEY},valueonly){_REC_SEP}", + f"--format=%H{_NUL_FMT}%B{_NUL_FMT}%(trailers:key={_TRAILER_KEY},valueonly){_NUL_FMT}", f"{mb}..{head}", ] ) if out is None: - # Same posture as the previous implementation: on git trouble the token is NOT armed, so - # the guard still runs. Failing the other way would silently disable it. + # 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 - for entry in out.split(_REC_SEP): - if not entry.strip(): + parts = out.split(_NUL) + if parts and not parts[-1].strip(): + parts.pop() # trailing inter-record newline after the final NUL + if len(parts) % _FIELDS_PER_COMMIT != 0: + # Exact arity, not best-effort. NUL cannot occur in a commit message, so a bad count means + # the output was truncated or the format atom wasn't understood — refuse to arm. + return False + for i in range(0, len(parts), _FIELDS_PER_COMMIT): + body, trailer = parts[i + 1], parts[i + 2] + if trailer.strip().startswith(_UNEXPANDED_ATOM): + # git too old to expand %(trailers:...): it echoes the atom literally with exit 0, which + # would read as a non-empty trailer and falsely arm. Treat as unarmed. continue - subject, _, trailer = entry.partition(_FIELD_SEP) - if token_armed_in(subject.strip(), trailer): + if token_armed_in(subject_of(body), trailer): return True return False diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index 911847c55..89f3aad19 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -510,3 +510,98 @@ def test_token_NOT_armed_by_body_prose(): def test_token_not_armed_by_empty_or_whitespace_trailer(): assert not dv.token_armed_in("docs: something", "") assert not dv.token_armed_in("docs: something", " \n ") + + +# ---- _token_armed INTEGRATION against real git (ersatztv#609 review round 2) ---- +# +# The unit tests above only exercise `token_armed_in`. Both false-arm defects found in review +# (git's `%s` folding the first PARAGRAPH, and an injectable in-band separator) lived in the git +# plumbing and passed those tests untouched. These drive the real command. + + +def _repo(tmp_path, messages: list[str]) -> tuple[str, str]: + """Build a throwaway repo; return (base_sha, head_sha).""" + import subprocess + + 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") + (tmp_path / "f").write_text("base\n") + g("add", "-A") + g("commit", "-qm", "base") + base = g("rev-parse", "HEAD").stdout.strip() + for i, msg in enumerate(messages): + (tmp_path / "f").write_text(f"{i}\n") + g("add", "-A") + g("commit", "-q", "-F", "-", input=msg) + return base, g("rev-parse", "HEAD").stdout.strip() + + +def _armed(tmp_path, message: str) -> bool: + import os + + base, head = _repo(tmp_path, [message]) + cwd = os.getcwd() + os.chdir(tmp_path) + try: + return dv._token_armed(base, head) + finally: + os.chdir(cwd) + + +def test_integration_subject_token_arms(tmp_path): + assert _armed(tmp_path, "fix(460): write null LastScan [decisions-edit]\n\nbody\n") + + +def test_integration_trailer_arms(tmp_path): + assert _armed(tmp_path, "docs: correct a figure\n\nwhy\n\nDecisions-Edit: the load number was wrong\n") + + +def test_integration_body_prose_does_not_arm(tmp_path): + assert not _armed(tmp_path, "docs: explain it\n\nThe check matches [decisions-edit] as a substring.\n") + + +def test_integration_second_line_of_first_paragraph_does_not_arm(tmp_path): + """git's %s is the first PARAGRAPH, not the first line — it folds line 2 in with a space. + + Using %s directly, this message arms the token. It must not: line 2 is body prose. + """ + assert not _armed(tmp_path, "fix: harmless subject\nThis explains [decisions-edit] on line two.\n\nbody\n") + + +def test_integration_separator_in_subject_cannot_inject(tmp_path): + """An in-band \\x1f separator was injectable; NUL cannot appear in a commit message.""" + assert not _armed(tmp_path, "docs: harmless\x1fsuffix\n\nbody\n") + assert not _armed(tmp_path, "docs: harmless\x1e\x1fsuffix\n\nbody\n") + + +def test_integration_empty_trailer_does_not_arm(tmp_path): + assert not _armed(tmp_path, "docs: something\n\nbody\n\nDecisions-Edit:\n") + + +def test_integration_only_a_later_commit_carries_it(tmp_path): + import os + + base, head = _repo(tmp_path, ["chore: unrelated\n", "fix: real correction [decisions-edit]\n"]) + cwd = os.getcwd() + os.chdir(tmp_path) + try: + assert dv._token_armed(base, head) + finally: + os.chdir(cwd) + + +def test_integration_unresolvable_refs_do_not_arm(tmp_path): + """Guard must still RUN when git can't answer; arming on failure would disable it.""" + assert not _armed(tmp_path, "docs: x\n") or True # build a repo first + import os + + cwd = os.getcwd() + os.chdir(tmp_path) + try: + assert dv._token_armed("nope1", "nope2") is False + finally: + os.chdir(cwd)