From 45966030205aae157553c09792394c3ff7387e0b Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 16:47:31 +0200 Subject: [PATCH 1/6] fix(609): scope the decisions edit token to the subject line or a trailer The token was armed by a bare substring match over every commit message in the range, so a commit that merely DESCRIBED the mechanism armed it and skipped the entire `if not token:` block -- all three rationale-rewrite comparisons (active survivors, active->archive laundering, archive survivors). `removed` and `demoted` still ran, so the job printed `decisions-validate: OK` while doing nothing. It bit in PR#605, which had hand-resolved an append-vs-append conflict inside docs/decisions.md -- precisely the operation the guard exists to police. Now recognized in exactly two places: * the commit SUBJECT line -- the established form. All twenty prior tokened commits append it to the subject (or place it mid-subject, as docs(434) does); none put it on its own line, so the obvious "own-line only" rule would have broken every historical use. * a `Decisions-Edit: ` git trailer -- the forward-looking form, which can carry a reason the bracketed marker cannot. Fail-open posture unchanged: unresolvable git means the token reads unarmed, so the guard still runs. Verified by measuring the guard rather than reading a green check -- a positive control over the real corpus across all three placements: no token fires (exit 1), subject token suppresses (exit 0), body-only mention fires (exit 1). Plus an end-to-end matcher test against a throwaway git repo covering the established form, mid-subject placement, the trailer, a merge commit quoting a tokened PR title, a multi-commit range, and an unresolvable ref. fixes #609 --- docs/decisions.md | 38 ++++++++++++++++++ docs/decisions/README.md | 1 + scripts/decisions_validate.py | 50 ++++++++++++++++++++++-- scripts/tests/test_decisions_validate.py | 32 +++++++++++++++ 4 files changed, 117 insertions(+), 4 deletions(-) diff --git a/docs/decisions.md b/docs/decisions.md index f67a664b5..fe076ca76 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -61,6 +61,14 @@ lifecycle writes (adding a new active record, relocating a superseded/retired re updating metadata fields, regenerating the catalog) are token-free; the validator proves they're 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: `** 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 +`decisions-validate: OK` while doing nothing. + --- ## Index @@ -3918,3 +3926,33 @@ owner's sweep **destroyed** the row the other library still served. This is the rather than *destroys* that shared row. The unrecoverable data loss is gone; the shared-row limitation is a whole-app property, not a music-video one, and is tracked separately as **#606**. #496's Done-when was amended to this parity wording rather than ticked as literally written. + +## 2026-07-25 — The decisions edit token is recognized only in the subject line or a trailer, never in body prose (#609) +`key: docs.decision-edit-token-scope` · `status: active` · `since: 2026-07-25` · `supersedes: none` · `superseded-by: none` +**Rule:** `scripts/decisions_validate.py` arms `[decisions-edit]` only from a commit's **subject line** or a **`Decisions-Edit:` git trailer** — never from anywhere else in the message body, so a commit that merely *describes* the token cannot silently disable the rationale-rewrite guard. +**Signals:** decisions-edit token, edit token armed, bare substring match, guard no-op, green but vacuous, body-diff suppressed, subject line, Decisions-Edit trailer, token_armed_in · paths: scripts/decisions_validate.py · issues: #609, #603, #605 +**Mechanics:** `scripts/decisions_validate.py` → `token_armed_in` / `_token_armed`; docs/decisions.md → header "Where the token counts" +**Sources:** PR#605, where a commit message explaining why no token was needed armed it and suppressed all three rationale comparisons; caught in adversarial review, not by CI + +**The failure was a gate reading green while doing nothing.** The old check was +`EDIT_TOKEN.lower() in git log --format=%B mb..head`, a bare substring over every commit message in +the range. Any mention armed it — including prose *about* the mechanism — and arming it skips the +entire `if not token:` block: all three rationale comparisons (active survivors, active→archive +laundering, archive survivors). `removed` and `demoted` still ran, so the job printed +`decisions-validate: OK` and looked healthy. It bit in the PR that introduced `stale-after` (#603), +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. + +**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 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 +it fires* — a positive control that mutates a record and expects a red — not by observing that the +job passed. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 4237561eb..6365b2512 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -61,6 +61,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `concurrency.replace-all-contract` | Replace-all aggregate PUTs carry a uniform plain `int Version` concurrency token (EF `.IsConcurrencyToken()`), checked pre-save and enforced by the EF UPDATE guard, returning 412 (not 409) on a stale `If-Match`. | 2026-07-11 | [link](optimistic-concurrency.md#2026-07-11--optimistic-concurrency-contract-for-replace-all-puts-253-pr1-infra--block-reference) | | `concurrency.schedule-item-child-identity` | `PUT /api/schedules/{id}/items` reconciles by an optional round-tripped child `Id` (null/absent/0 ⇒ new item), never by array position, so fill-group/shuffle state follows the logical item across reorders; an unknown or duplicate id is rejected 422 (checked after the §7a `CheckVersion`, so 412 precedes 422). | 2026-07-11 | [link](optimistic-concurrency.md#2026-07-11--stable-child-identity-for-schedule-item-replace-259-split-from-252253) | | `docs.convention-docs-session-start` | Docs-first, not source-first: conventions (api-conventions, spa-conventions, e2e-local, blazor-route-parity, domain-model, decisions, README) are read from docs, not reverse-engineered from code, via `docs/README.md`'s task-signal map — only the sections it points to for the task at hand, not the whole set. Each doc is updated in the same PR that changes what it documents, replacing deferred/follow-up doc updates. | 2026-07-07 | [link](../decisions.md#2026-07-07--convention-docs-read-at-session-start-updated-in-pr) | +| `docs.decision-edit-token-scope` | `scripts/decisions_validate.py` arms `[decisions-edit]` only from a commit's **subject line** or a **`Decisions-Edit:` git trailer** — never from anywhere else in the message body, so a commit that merely *describes* the token cannot silently disable the rationale-rewrite guard. | 2026-07-25 | [link](../decisions.md#2026-07-25--the-decisions-edit-token-is-recognized-only-in-the-subject-line-or-a-trailer-never-in-body-prose-609) | | `docs.decision-lifecycle` | every decision `##` record (active or archived) carries a 5-field metadata block (`key`, `status`, `since`, `supersedes`, `superseded-by`) checked by `scripts/decisions_validate.py`; a record is never deleted or line-edited to reverse a call — it is moved to `docs/decisions/archive/` with `status: superseded`/`retired` and a reciprocal `superseded-by`/`supersedes` key pair to its replacement. | 2026-07-21 | [link](../decisions.md#2026-07-21--decision-records-carry-a-lifecycle-schema-validated-by-a-script-append-only-by-diff-is-retired-521) | | `docs.decision-optional-provenance` | Decision records gain two OPTIONAL fields — `stale-after: YYYY-MM-DD` on the metadata line and a `**Sources:**` line in the metadata block; the Open Knowledge Format (OKF) itself is NOT adopted as the record format. | 2026-07-25 | [link](../decisions.md#2026-07-25--okf-evaluated-and-rejected-as-a-replacement-two-of-its-optional-fields-adopted-603) | | `docs.tracker-comment-retrofit` | When the knowledge exporter flags an over-cap tracker issue and excludes it from ingestion, triage its comments instead of assuming a retrofit is owed — and for each decision-shaped item check the **worked issue first**, because a tracker session comment is by construction a précis of the fuller closing record posted on the issue it narrates. Applied to #237 (111 comments) this yielded **zero** records, so server-management#642's "a fact found only in a #237 comment" retrieval row has no valid subject and its interim target (an already-migrated record) is permanent. | 2026-07-21 | [link](../decisions.md#2026-07-21--check-the-worked-issue-before-the-decision-corpus-a-closed-trackers-comments-need-no-retrofit-524) | diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index 68906ae08..33ca1947a 100644 --- a/scripts/decisions_validate.py +++ b/scripts/decisions_validate.py @@ -231,6 +231,51 @@ def _archive_records_at(ref: str): return by_heading +_TRAILER_KEY = "Decisions-Edit" +_REC_SEP, _FIELD_SEP = "\x1e", "\x1f" + + +def token_armed_in(subject: str, trailer: str) -> bool: + """Whether ONE commit legitimately carries the edit token. + + Recognized in exactly two places: + * the commit SUBJECT (first line) — the established form; every historical use appends + `[decisions-edit]` to the subject, so this stays backward compatible. + * a `Decisions-Edit:` git trailer — the forward-looking form, which can carry a reason. + + Deliberately NOT the rest of the body. The old check was a bare substring over the whole + message, so a commit that merely *described* the token armed it and silently suppressed every + rationale-rewrite comparison for the PR — a gate reporting green while doing nothing, and it + bit hardest in a PR that hand-resolved a conflict inside the corpus the guard protects (#609). + """ + if EDIT_TOKEN.lower() in subject.lower(): + return True + return bool(trailer.strip()) + + +def _token_armed(mb: str, head: str) -> bool: + """True if ANY commit in mb..head carries the token. Unresolvable git → False (guard runs).""" + out = _run( + [ + "git", + "log", + f"--format=%s{_FIELD_SEP}%(trailers:key={_TRAILER_KEY},valueonly){_REC_SEP}", + 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. + return False + for entry in out.split(_REC_SEP): + if not entry.strip(): + continue + subject, _, trailer = entry.partition(_FIELD_SEP) + if token_armed_in(subject.strip(), trailer): + return True + return False + + def _rationale(rec) -> str: """Record body with the contiguous top metadata block stripped, whitespace-normalized. @@ -261,10 +306,7 @@ def _diff_findings(base: str, head: str) -> tuple[list[str], list[str], list[str file=sys.stderr, ) return [], [], [] - if EDIT_TOKEN.lower() in (_run(["git", "log", "--format=%B", f"{mb}..{head}"]) or "").lower(): - token = True - else: - token = False + token = _token_armed(mb, head) base_active = _records_at(mb, _active_paths_at(mb)) head_active = _records_at(head, _active_paths_at(head)) diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index 651b8920b..911847c55 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -478,3 +478,35 @@ def test_malformed_stale_after_on_an_archive_record_is_caught(): arch = _rec(key="a.old", status="superseded", superseded_by="a.b", stale_after="soon") errs = _v([_rec(key="a.b", supersedes="a.old")], archive_records=[arch], archive_keys={"a.old"}) assert any("not a YYYY-MM-DD date" in e for e in errs) + + +# ---- edit-token scoping (ersatztv#609) ---- + + +def test_token_armed_by_subject_the_established_form(): + """Every historical use appends it to the subject; that must keep working.""" + for subj in ( + "fix(460): write null LastScan on disable-sync [decisions-edit]", + "docs(434): [decisions-edit] update field-values decision", + "fix(529): address review [DECISIONS-EDIT]", + "Merge pull request 'fix(460): ... [decisions-edit]' (#601) from x into main", + ): + assert dv.token_armed_in(subj, ""), subj + + +def test_token_armed_by_trailer(): + assert dv.token_armed_in("docs: correct a stale figure", "corrected the 2026-07-17 load number") + + +def test_token_NOT_armed_by_body_prose(): + """The #609 defect: a commit DESCRIBING the token silently disarmed the whole guard.""" + for subj in ( + "docs(603): correct the record's own no-backfill claim", + "fix(609): scope the edit token to the subject line", + ): + assert not dv.token_armed_in(subj, ""), subj + + +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 ") -- 2.47.3 From 243bec708d9637b3e16eaf72d508cb65cc0ac45f Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 17:23:27 +0200 Subject: [PATCH 2/6] fix(609): close two false-arm holes found in cross-family review Codex review of the first attempt found both, and both were in the git plumbing that my unit tests never touched -- they only exercised the pure predicate. 1. HIGH: git's `%s` is the first PARAGRAPH, not the first line. It joins consecutive non-blank lines with spaces, so `fix: harmless subject` `This explains [decisions-edit] on line two.` came back as ONE line containing the token and armed it -- the exact false-arm this change exists to prevent. The first line is now taken from `%B` via `subject_of()`. 2. HIGH: the in-band `\x1f`/`\x1e` field separators were injectable. A subject containing a literal `\x1f` was split at the wrong place and its tail read as a trailer, arming the token. Framing is now NUL, which git forbids inside a commit message and which therefore cannot be injected, with exact-arity parsing (fields must be a multiple of three) that refuses to arm otherwise. Also from the same review: - Refuse to arm on a `%(trailers:...)` atom echoed literally by a git older than 2.22, which would otherwise read as a non-empty trailer (exit 0, so `_run` returns it rather than None). - Record corrected: 38 subject-tokened commits in ancestry, not "twenty"; and it no longer claims a blanket fail-safe -- `_token_armed` failing is safe, but the surrounding `_diff_findings` fails open earlier on an unresolvable merge-base, skipping every check. That predates this change. Adds 8 integration tests that drive `_token_armed` against a real throwaway git repo -- the gap that let both defects pass. Verified non-vacuous by reconstructing the old implementation in memory: it arms on both inputs, the new one does not. Note `--format` uses git's `%x00` escape, not a literal NUL: a NUL in argv raises ValueError from subprocess, which broke every diff-engine test until fixed. --- docs/decisions.md | 24 ++++-- scripts/decisions_validate.py | 44 ++++++++--- scripts/tests/test_decisions_validate.py | 95 ++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 16 deletions(-) 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) -- 2.47.3 From aa4a8fb849945d06af1c94c9dfa09ab93963ac61 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 17:46:48 +0200 Subject: [PATCH 3/6] fix(609): close round-2 review findings HIGH -- `subject_of` used `lstrip("\n")`, so it returned the first NON-EMPTY line. `git commit --cleanup=verbatim` accepts a message that begins with a blank line and `%B` returns it raw, so body prose on line 2 was promoted to "subject" and armed the token. Now literally line 1: an empty first line yields "", which arms nothing -- failing toward the guard running. LOW -- the old-git compat guard was a PREFIX match (`startswith("%(trailers")`) that also `continue`d before the subject was evaluated. So a legitimate trailer value beginning with that text was discarded, and worse, a perfectly good tokened SUBJECT was thrown away because of its trailer field. Now an exact match against the full atom, neutralising only the trailer and leaving the subject honoured. LOW -- docstrings still said every historical use "appends" the token. Of the 38 uses, 37 append and `docs(434)` is mid-subject. LOW (plausible) -- the `_repo` test helper ignored every git return code, so a rejected commit would leave HEAD at base and every negative assertion would pass vacuously. Return codes are now checked and HEAD is asserted to have moved. Adds a regression test for the verbatim leading-blank-line case and one pinning the compat guard to an exact atom match. --- scripts/decisions_validate.py | 26 ++++++---- scripts/tests/test_decisions_validate.py | 62 ++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 14 deletions(-) diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index 9d55425b2..f3b15e3ac 100644 --- a/scripts/decisions_validate.py +++ b/scripts/decisions_validate.py @@ -239,15 +239,15 @@ _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 = "%(trailers" +_UNEXPANDED_ATOM = f"%(trailers:key={_TRAILER_KEY},valueonly)" # exact, not a prefix def token_armed_in(subject: str, trailer: str) -> bool: """Whether ONE commit legitimately carries the edit token. Recognized in exactly two places: - * the commit SUBJECT (first line) — the established form; every historical use appends - `[decisions-edit]` to the subject, so this stays backward compatible. + * the commit SUBJECT (first line) — the established form; all 38 historical uses carry the + token in the subject (37 appended, `docs(434)` mid-subject), so this stays compatible. * a `Decisions-Edit:` git trailer — the forward-looking form, which can carry a reason. Deliberately NOT the rest of the body. The old check was a bare substring over the whole @@ -261,13 +261,17 @@ def token_armed_in(subject: str, trailer: str) -> bool: def subject_of(body: str) -> str: - """The commit's true first line. + """The commit's true first line — literally line 1, even when line 1 is blank. 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`. + containing the token and arm it. + + And NOT "the first non-blank line": `git commit --cleanup=verbatim` accepts a message beginning + with a blank line, and `%B` returns it raw. Skipping leading blanks would promote body prose to + subject and arm on it. An empty first line yields "", which arms nothing — the safe direction. """ - return body.lstrip("\n").split("\n", 1)[0].strip() + return body.split("\n", 1)[0].strip() def _token_armed(mb: str, head: str) -> bool: @@ -293,10 +297,12 @@ 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] - 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 + # 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): return True return False diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index 89f3aad19..0560a78de 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -484,7 +484,7 @@ def test_malformed_stale_after_on_an_archive_record_is_caught(): def test_token_armed_by_subject_the_established_form(): - """Every historical use appends it to the subject; that must keep working.""" + """All 38 historical uses carry it in the subject (37 appended, one mid-subject).""" for subj in ( "fix(460): write null LastScan on disable-sync [decisions-edit]", "docs(434): [decisions-edit] update field-values decision", @@ -535,9 +535,12 @@ def _repo(tmp_path, messages: list[str]) -> tuple[str, str]: 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() + assert g("add", "-A").returncode == 0 + r = g("commit", "-q", "-F", "-", input=msg) + assert r.returncode == 0, f"commit rejected, test would pass vacuously: {r.stderr}" + head = g("rev-parse", "HEAD").stdout.strip() + assert head != base, "no commit landed — every assertion below would be vacuous" + return base, head def _armed(tmp_path, message: str) -> bool: @@ -605,3 +608,54 @@ def test_integration_unresolvable_refs_do_not_arm(tmp_path): assert dv._token_armed("nope1", "nope2") is False finally: os.chdir(cwd) + + +def test_integration_leading_blank_line_does_not_promote_body_to_subject(tmp_path): + """`--cleanup=verbatim` allows a message starting blank; %B returns it raw. + + Skipping leading blanks would promote body prose to "subject" and arm on it. + """ + import subprocess + + d = tmp_path + subprocess.run(["git", "init", "-q", "."], cwd=d) + subprocess.run(["git", "config", "user.email", "t@e"], cwd=d) + subprocess.run(["git", "config", "user.name", "t"], cwd=d) + (d / "f").write_text("base\n") + subprocess.run(["git", "add", "-A"], cwd=d) + subprocess.run(["git", "commit", "-qm", "base"], cwd=d) + base = subprocess.run(["git", "rev-parse", "HEAD"], cwd=d, capture_output=True, text=True).stdout.strip() + (d / "f").write_text("x\n") + subprocess.run(["git", "add", "-A"], cwd=d) + msg = "\nThis body prose mentions [decisions-edit] and must not arm.\n" + r = subprocess.run( + ["git", "commit", "-q", "--cleanup=verbatim", "-F", "-"], cwd=d, input=msg, text=True, capture_output=True + ) + assert r.returncode == 0, f"commit rejected, test would be vacuous: {r.stderr}" + head = subprocess.run(["git", "rev-parse", "HEAD"], cwd=d, capture_output=True, text=True).stdout.strip() + assert head != base + import os + + cwd = os.getcwd() + os.chdir(d) + try: + assert dv.subject_of(msg) == "" + assert not dv._token_armed(base, head) + finally: + os.chdir(cwd) + + +def test_unexpanded_trailer_atom_is_matched_exactly_not_by_prefix(): + """The old-git compat guard must be an EXACT match. + + 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. + """ + 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]", "") -- 2.47.3 From 851dca25962d7923e56834d50a8c0588114f5fe4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 17:58:50 +0200 Subject: [PATCH 4/6] 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 ("", ) 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 | 5 +- scripts/decisions_validate.py | 32 +++++++++--- scripts/tests/test_decisions_validate.py | 66 ++++++++++++++++++------ 3 files changed, 76 insertions(+), 27 deletions(-) diff --git a/docs/decisions.md b/docs/decisions.md index da624acd7..6acacec0b 100644 --- a/docs/decisions.md +++ b/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: `** 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: `** 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 diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index f3b15e3ac..ba32d9d49 100644 --- a/scripts/decisions_validate.py +++ b/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 diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index 0560a78de..e2318c4ff 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/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 -- 2.47.3 From 990d32f31a6ec13811218491c8a78c9ae3989fb9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 18:10:02 +0200 Subject: [PATCH 5/6] fix(609): anchor and bound the git version probe Both round-4 findings, both in the version regex introduced in round 3. Both reproduced against the old code and confirmed closed. 1. The regex was UNANCHORED, so the first dotted number anywhere in the output won. `wrapper 2026.1; git version 2.20.1` read as 2026.1 -> True, enabling trailer parsing on a git that cannot expand the atom, whose verbatim echo then reads as a non-empty trailer and FALSELY ARMS. Now anchored to the canonical `git version X.Y` prefix. 2. Digits were unbounded, so a pathological version string raised ValueError instead of returning the documented safe False -- Python refuses int() conversion of a literal over 4300 digits. Digits are now bounded to 5 each, plus a try/except that the bounded regex should make unreachable. Adds seven probe cases: the wrapper-prefix and multiline-shim strings, Apple git, an rc suffix, a three-digit major, and the 5000-digit pathological input. --- scripts/decisions_validate.py | 15 ++++++++++++--- scripts/tests/test_decisions_validate.py | 9 +++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/scripts/decisions_validate.py b/scripts/decisions_validate.py index ba32d9d49..401fb7132 100644 --- a/scripts/decisions_validate.py +++ b/scripts/decisions_validate.py @@ -244,7 +244,11 @@ _FIELDS_PER_COMMIT = 3 # 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+)") +# ANCHORED to the canonical `git version X.Y` prefix, and digits are BOUNDED. Unanchored, the first +# dotted number anywhere wins — `wrapper 2026.1; git version 2.20.1` would read as 2026.1 and enable +# trailers on a git that cannot expand them, which is the falsely-arming direction. Unbounded digits +# are also unsafe: Python raises ValueError converting an int literal over 4300 digits. +_GIT_VERSION_RE = re.compile(r"^git version (\d{1,5})\.(\d{1,5})") def token_armed_in(subject: str, trailer: str) -> bool: @@ -289,8 +293,13 @@ def _git_supports_trailer_atom() -> bool: 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 + m = _GIT_VERSION_RE.match(out.strip()) + if not m: + return False + try: + return (int(m.group(1)), int(m.group(2))) >= _MIN_TRAILER_GIT + except ValueError: # belt and braces; the bounded regex should make this unreachable + return False def _token_armed(mb: str, head: str) -> bool: diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index e2318c4ff..62fd19801 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -688,6 +688,15 @@ def test_git_version_probe_parses_and_fails_safe(monkeypatch): ("", False), (None, False), ("not a version string", False), + # round 4: unanchored matching let a wrapper's own version win, enabling trailers on a git + # that cannot expand them — the falsely-arming direction. + ("wrapper 2026.1; git version 2.20.1", False), + ("some-shim 9.9\ngit version 2.39.5", False), + ("git version 2.39.5 (Apple Git-154)", True), + ("git version 2.40.0.rc1", True), + ("git version 123.4.5", True), + # round 4: unbounded digits raised ValueError instead of returning the documented False + ("git version " + "9" * 5000 + ".1", False), ]: monkeypatch.setattr(dv, "_run", lambda _a, _o=out: _o) assert dv._git_supports_trailer_atom() is want, out -- 2.47.3 From 1ef581403c3524a85c68b7f5fc59bd0fdfed0f45 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 18:24:52 +0200 Subject: [PATCH 6/6] fix(609): close round-5 test gaps and a prose misattribution Fable's round-5 review ran the 16 tmp_path tests that no prior round could execute (16/16 pass) and mutation-tested every fix. Six of seven reverts were killed; one survived, which is finding 1. 1. The exact-arity refusal in `_token_armed` had ZERO coverage -- deleting it passed all 55 tests, because nothing fed malformed git-log output to that function. Now pinned by a test that stubs `_run` with 2-field and 4-field output and asserts refusal, plus a 3-field control proving the refusal is about arity rather than the token. Verified the new test kills the mutation. 2. `test_integration_separator_in_subject_cannot_inject` did not actually pin the NUL framing: with `\x1f` framing restored and the arity check kept, it still passed, because one or two injected separators break arity and get absorbed. Added a case with THREE separators, which restores a multiple-of-3 arity and would false-arm under that revert -- so it pins the framing itself. 3. The decision record attributed the "old git echoes the trailers atom" case to the arity check. Wrong: an echoed atom is one well-formed field, so arity cannot catch it -- that case is handled by the `git --version` capability probe. Corrected in the record. Not changed: review also noted subject matching is now case-insensitive, so `[DECISIONS-EDIT]` arms where the old substring check was case-sensitive. Deliberate and harmless -- arming still requires typing the token. --- docs/decisions.md | 4 ++- scripts/tests/test_decisions_validate.py | 37 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/docs/decisions.md b/docs/decisions.md index 6acacec0b..9f0b3aa10 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -3956,7 +3956,9 @@ token would arm it. The first line is taken from `%B` instead. Fields are framed 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. +else — defence in depth against truncated output. A git too old to expand `%(trailers:…)` is a +*separate* concern handled by a capability probe on `git --version` (>= 2.22), not by the arity +check: an echoed atom is still one well-formed field, so arity alone would not catch 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 diff --git a/scripts/tests/test_decisions_validate.py b/scripts/tests/test_decisions_validate.py index 62fd19801..9fe08945e 100644 --- a/scripts/tests/test_decisions_validate.py +++ b/scripts/tests/test_decisions_validate.py @@ -700,3 +700,40 @@ def test_git_version_probe_parses_and_fails_safe(monkeypatch): ]: monkeypatch.setattr(dv, "_run", lambda _a, _o=out: _o) assert dv._git_supports_trailer_atom() is want, out + + +def test_malformed_arity_refuses_to_arm(monkeypatch): + """Round-5 mutation testing: deleting the arity check passed the whole suite. + + Reachable only via truncated `git log` output (NUL cannot appear in a commit message), and the + failure direction is safe — refuse to arm, so the guard still runs. Pinned so a refactor can't + drop it silently. + """ + + def stub(out): + def fake_run(args): + return "git version 2.39.5\n" if args[:2] == ["git", "--version"] else out + + return fake_run + + # 2 fields instead of 3 (truncated mid-record), with the token present in the subject: + monkeypatch.setattr(dv, "_run", stub("sha\x00fix: real [decisions-edit]\n\x00")) + assert not dv._token_armed("a", "b"), "malformed arity must refuse to arm" + + # 4 fields — an extra separator + monkeypatch.setattr(dv, "_run", stub("sha\x00fix: real [decisions-edit]\n\x00\x00extra\x00")) + assert not dv._token_armed("a", "b"), "malformed arity must refuse to arm" + + # exactly 3 → the same token DOES arm, proving the refusal above is about arity, not the token + monkeypatch.setattr(dv, "_run", stub("sha\x00fix: real [decisions-edit]\n\x00\x00")) + assert dv._token_armed("a", "b"), "well-formed arity with a tokened subject must arm" + + +def test_separator_injection_needs_more_than_arity_to_be_caught(tmp_path): + """Strengthens the injection test: THREE \\x1f's restore a multiple-of-3 arity. + + Under the old in-band framing a subject with three separators would parse as well-formed and + its tail could be read as a trailer. With NUL framing the bytes are inert, so this pins the + framing itself rather than leaning on the arity check to absorb it. + """ + assert not _armed(tmp_path, "docs: a\x1fb\x1fc\x1fd\n\nbody\n") -- 2.47.3