fix(609): scope the decisions edit token to the subject line or a trailer #614

Closed

timothy wants to merge 6 commits from fix/609-decisions-edit-token-scope into main

pull from: fix/609-decisions-edit-token-scope

merge into: :main

:main

:renovate/meziantou.analyzer-3.x

:release/v26.15.0-notes

:fix/830-add-items-error-surface

:renovate/lucene.net

:renovate/cliwrap-3.x

:issue-806-guard-populations

:renovate/dotnet-monorepo

:scratch/767b-poisoned

:scratch/767b-control

:release/v26.14.0-notes

:release/v26.14.0

:renovate/sqlitepclraw.bundle_e_sqlite3-3.x

:docs/510-skill-logo-bug-policy

:fix/510-watermark-resolution-policy

:fix/629-verdict-classifier-falseopens

:fix/609-decisions-edit-token-scope

:issue-135-clear-to-none

:release/v26.12.0-notes

:fix/409b-lastscan-api-parity

:fix/401-updatechannel-mirror-422

:fix/327-playlist-rename-validation

:fix/410-scancancel-log-level

:fix/409-447-librariesscreen-neverscanned

:fix/338-zap-exit-code

:fix/367-plex-budget-message

:fix/310-debom-legacy-cs

:ci/604-lane-rebalance

:feat/388-design-mirror

:feat/247-test-ownership

:feat/247-primary-action

:feat/357-player-owned-playback

:feat/357-jellyfin-plugin-poc

:fix/289-mcp-hardening

:issue58-mcp

:feat/244-channels-extract

:ci/auto-bump-prod-compose

:feat/multi-rerun-collections-api

:feat/collections-api

:feat/quick-wins

:feat/185-docs-part2

:feat/140-collections-screen

:feat/146-channel-edit

:feat/147-classic-ui-link

:issue22-renovate-dashboard

:feat/91-cutover

:feat/63-composite-create

:feat/65-library-browse

:feat/85-epg

:feat/86-schedule-editor

:feat/109-dashboard-data

:feat/99-session-tracking

:fix/dockerfile-node-tag

:feat/59-spa-foundation

:docs/59-ui-redesign-brief

:feat/102-json-guide

:feat/111-schedule-durations

:feat/104-artwork-upload

:feat/103-media-sources-api

:feat/playouts-read-api

:feat/108-health-api

:feat/105-picker-list-endpoints

:issue-97-channel-state-api

:issue42-jellyfin-musicvideos

:issue46-rest-api-error-contract

:dependabot/nuget/ErsatzTV.FFmpeg.Tests/multi-d307a2e06f

:qsv-improvements

:hdr-vulkan-cuda-test

Conversation 3 Commits 6 Files Changed 4

+414 -4

timothy commented 2026-07-25 16:47:48 +02:00

Owner

Copy Link

Copy Source

fixes #609

The [decisions-edit] 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. Caught in adversarial review, not by CI.

The fix

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 — worth checking before picking a form.
• A Decisions-Edit: <reason> git trailer — the forward-looking form, which can carry a reason the bracketed marker cannot.

Body prose no longer arms it, so you can write about the mechanism safely.

Fail-open posture unchanged: unresolvable git means the token reads unarmed, so the guard still runs. Failing the other way would silently disable it, which is the bug.

Verification — by measuring the guard, not by reading a green check

Positive control over the real corpus, mutating ci.runner-placement's rationale and re-running across all three placements:

commit message	guard fires	exit
no token	yes	1
[decisions-edit] in subject	no (correctly suppressed)	0
mentioned only in body	yes	1

That last row is the defect, now closed.

End-to-end matcher test against a throwaway git repo — established subject form, mid-subject placement, the trailer, a merge commit quoting a tokened PR title, a multi-commit range where only a later commit carries it, and an unresolvable ref → all pass.

Regression check against real historical commits: 890745d1, 9cb51f6a, 71706f38 all still arm; a non-tokened commit does not.

55 tests (4 new); ruff, ruff format, pyright clean on touched files; decisions-validate: OK; catalog --check OK.

Docs

• docs/decisions.md — new header section "Where the token counts", plus a new active record docs.decision-edit-token-scope.
• docs/decisions/README.md — regenerated.

Note this PR needs no token itself: the new record is new at HEAD (the body-diff guard doesn't apply to it) and the header text isn't a record.

🤖 Generated with Claude Code

fixes #609 The `[decisions-edit]` 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. Caught in adversarial review, not by CI. ## The fix 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 — worth checking before picking a form. - **A `Decisions-Edit: <reason>` git trailer** — the forward-looking form, which can carry a reason the bracketed marker cannot. Body prose no longer arms it, so you can write *about* the mechanism safely. Fail-open posture unchanged: unresolvable git means the token reads unarmed, so the guard still runs. Failing the other way would silently disable it, which is the bug. ## Verification — by measuring the guard, not by reading a green check Positive control over the **real corpus**, mutating `ci.runner-placement`'s rationale and re-running across all three placements: | commit message | guard fires | exit | |---|---|---| | no token | yes | 1 | | `[decisions-edit]` in subject | no (correctly suppressed) | 0 | | mentioned only in body | **yes** | **1** | That last row is the defect, now closed. End-to-end matcher test against a throwaway git repo — established subject form, mid-subject placement, the trailer, a merge commit quoting a tokened PR title, a multi-commit range where only a later commit carries it, and an unresolvable ref → all pass. Regression check against real historical commits: `890745d1`, `9cb51f6a`, `71706f38` all still arm; a non-tokened commit does not. 55 tests (4 new); ruff, ruff format, pyright clean on touched files; `decisions-validate: OK`; catalog `--check` OK. ## Docs - `docs/decisions.md` — new header section "Where the token counts", plus a new active record `docs.decision-edit-token-scope`. - `docs/decisions/README.md` — regenerated. Note this PR needs no token itself: the new record is new at HEAD (the body-diff guard doesn't apply to it) and the header text isn't a record. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 1 commit 2026-07-25 16:47:48 +02:00

fix(609): scope the decisions edit token to the subject line or a trailer ... 55e218de33

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: <reason>` 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

timothy force-pushed fix/609-decisions-edit-token-scope from 55e218de33 to 09df6de534 2026-07-25 16:49:21 +02:00 Compare

timothy added 1 commit 2026-07-25 17:23:30 +02:00

fix(609): close two false-arm holes found in cross-family review ... 4306b57119

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.

timothy commented 2026-07-25 17:24:56 +02:00

Author

Owner

Copy Link

Copy Source

Cross-family review round 1 — BLOCKED, two HIGH false-arm holes

Codex CLI (codex-cli 0.144.6, run directly — the subagent wrapper couldn't retrieve its own background result, so its earlier "BLOCKED" was a tooling failure, not a finding). Both HIGH defects were in the git plumbing my unit tests never touched — they only exercised the pure predicate, so both passed clean.

1. %s is the first paragraph, not the first line. git joins consecutive non-blank lines with spaces, so:

fix: harmless subject
This explains [decisions-edit] on line two.

body

came back as one line containing the token and armed it — the exact false-arm this PR exists to prevent, reintroduced by the fix. Reproduced directly against git. Now subject_of() takes the first line from %B.

2. The in-band \x1f/\x1e 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. Reframed on NUL — which git forbids inside a commit message, so it's the one separator a message cannot inject — plus exact-arity parsing (fields must be a multiple of three) that refuses to arm on anything else.

Both reproduced against the old implementation and confirmed fixed in the new one:

case	old	new
token on line 2 of first paragraph	armed	not armed
\x1f injected in subject	armed	not armed

Also fixed:

• Refuse to arm on a %(trailers:…) atom echoed literally by a git older than 2.22 (exit 0, so _run returns it rather than None).
• Record overclaimed the fail-safe. _token_armed failing is safe, but _diff_findings fails open earlier on an unresolvable merge-base and skips every check. Prose corrected; that behaviour predates this PR.
• "All twenty prior tokened commits" was wrong — there are 38 in main's ancestry. Corrected, along with "appended" → "usually appended, sometimes mid-subject".

Added 8 integration tests driving _token_armed against a real throwaway git repo — the gap that let both defects through. Verified non-vacuous by reconstructing the old implementation in memory and confirming it arms on both inputs while the new one doesn't.

One self-inflicted note: the first NUL attempt put a literal NUL in argv, which raises ValueError from subprocess and broke every diff-engine test until I switched to git's %x00 escape. Caught by running the suite, not by reading.

63 tests pass; ruff/format/pyright clean; validator and catalog --check OK — all verified in a disposable worktree at 4306b571 so unrelated in-flight work couldn't contaminate the result.

Round-2 cross-family review of the fixes is running.

## Cross-family review round 1 — BLOCKED, two HIGH false-arm holes Codex CLI (`codex-cli 0.144.6`, run directly — the subagent wrapper couldn't retrieve its own background result, so its earlier "BLOCKED" was a tooling failure, not a finding). Both HIGH defects were in the **git plumbing my unit tests never touched** — they only exercised the pure predicate, so both passed clean. **1. `%s` is the first *paragraph*, not the first line.** git joins consecutive non-blank lines with spaces, so: ``` fix: harmless subject This explains [decisions-edit] on line two. body ``` came back as one line containing the token and **armed** it — the exact false-arm this PR exists to prevent, reintroduced by the fix. Reproduced directly against git. Now `subject_of()` takes the first line from `%B`. **2. The in-band `\x1f`/`\x1e` 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. Reframed on **NUL** — which git forbids inside a commit message, so it's the one separator a message cannot inject — plus exact-arity parsing (fields must be a multiple of three) that refuses to arm on anything else. Both reproduced against the *old* implementation and confirmed fixed in the new one: | case | old | new | |---|---|---| | token on line 2 of first paragraph | armed | not armed | | `\x1f` injected in subject | armed | not armed | **Also fixed:** - Refuse to arm on a `%(trailers:…)` atom echoed literally by a git older than 2.22 (exit 0, so `_run` returns it rather than `None`). - **Record overclaimed the fail-safe.** `_token_armed` failing is safe, but `_diff_findings` fails open *earlier* on an unresolvable merge-base and skips every check. Prose corrected; that behaviour predates this PR. - **"All twenty prior tokened commits" was wrong** — there are **38** in `main`'s ancestry. Corrected, along with "appended" → "usually appended, sometimes mid-subject". **Added 8 integration tests** driving `_token_armed` against a real throwaway git repo — the gap that let both defects through. Verified non-vacuous by reconstructing the old implementation in memory and confirming it arms on both inputs while the new one doesn't. One self-inflicted note: the first NUL attempt put a literal NUL in argv, which raises `ValueError` from `subprocess` and broke every diff-engine test until I switched to git's `%x00` escape. Caught by running the suite, not by reading. 63 tests pass; ruff/format/pyright clean; validator and catalog `--check` OK — all verified in a disposable worktree at `4306b571` so unrelated in-flight work couldn't contaminate the result. Round-2 cross-family review of the fixes is running.

timothy added 1 commit 2026-07-25 17:47:01 +02:00

fix(609): close round-2 review findings ... be129cad91

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.

timothy added 1 commit 2026-07-25 17:58:54 +02:00

fix(609): close round-3 review findings ... a642f9e451

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.

timothy added 1 commit 2026-07-25 18:10:07 +02:00

fix(609): anchor and bound the git version probe ... f4d005815b

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.

timothy force-pushed fix/609-decisions-edit-token-scope from f4d005815b to 1ef581403c 2026-07-25 18:26:09 +02:00 Compare

timothy commented 2026-07-25 18:51:45 +02:00

Author

Owner

Copy Link

Copy Source

Review rounds 2–5 — MERGEABLE

Five adversarial rounds: four cross-family (Codex CLI 0.144.6, driven directly) plus a final Claude round with filesystem access. Every round found something real, and rounds 3–5 each found a defect introduced by the previous round's fix — which is precisely what a single review pass misses.

round	findings
1	2 HIGH — %s is the first paragraph not line; \x1f separator injectable
2	1 HIGH — lstrip("\n") promoted body prose to subject on a verbatim message; + 3 LOW
3	0 HIGH, 4 LOW — value sentinel collided with a real trailer; a test that pinned nothing; helper ignored git return codes
4	0 HIGH, 2 LOW — version regex unanchored (a wrapper's version could win → false-arm) and unbounded (5000 digits raised ValueError)
5	0 HIGH, 2 LOW test-gaps + 1 prose error

Round 5 closed the coverage gap that made rounds 1–4 partly blind

Codex's sandbox has no writable temp directory, so 16 tmp_path tests never executed in any prior round — including every integration test covering both original HIGH defects. Round 5 ran them: 16/16 executed and passed, 0 skipped.

It also mutation-tested every fix. Six of seven reverts were killed; one survived — the exact-arity refusal had zero coverage, so deleting it passed all 55 tests. Now pinned, and the new test verifiably kills that mutation.

It also caught that test_integration_separator_in_subject_cannot_inject didn't pin what its name claims: one or two injected separators break arity and get absorbed, so the test passed even with \x1f framing restored. It takes three separators to restore a multiple-of-3 arity and expose the framing — added.

Its independent false-arm hunt: 21 adversarial messages, zero false arms (CRLF verbatim, 5000-char subjects, a subject that is a trailer line, Turkish-İ casing, invalid UTF-8, merge commits, literal %x00/%H/%B text). It independently found all 38 historical tokened commits and confirmed every one still arms.

CI

The earlier decisions lifecycle red was the known flake, ruled out concretely rather than assumed: a heading diff against the merge-base showed one added, none removed or renamed (the failure mode that impersonates the flake), and both job steps passed locally at that exact head. It is green on the rebased head.

Rebased onto current main — the append-vs-append conflict was resolved keeping all three records and verified numerically: main has 162 active keys, this branch 163 = main's 162 + one new, zero lost.

8/9 checks green; the only outstanding job is the advisory Build & push image, which is not a required context.

Review-verdict: MERGEABLE @ 1ef581403c

## Review rounds 2–5 — MERGEABLE Five adversarial rounds: four cross-family (Codex CLI 0.144.6, driven directly) plus a final Claude round with filesystem access. Every round found something real, and rounds 3–5 each found a defect **introduced by the previous round's fix** — which is precisely what a single review pass misses. | round | findings | |---|---| | 1 | 2 HIGH — `%s` is the first *paragraph* not line; `\x1f` separator injectable | | 2 | 1 HIGH — `lstrip("\n")` promoted body prose to subject on a verbatim message; + 3 LOW | | 3 | 0 HIGH, 4 LOW — value sentinel collided with a real trailer; a test that pinned nothing; helper ignored git return codes | | 4 | 0 HIGH, 2 LOW — version regex unanchored (a wrapper's version could win → **false-arm**) and unbounded (5000 digits raised `ValueError`) | | 5 | 0 HIGH, 2 LOW test-gaps + 1 prose error | ### Round 5 closed the coverage gap that made rounds 1–4 partly blind Codex's sandbox has no writable temp directory, so **16 `tmp_path` tests never executed in any prior round** — including every integration test covering both original HIGH defects. Round 5 ran them: **16/16 executed and passed**, 0 skipped. It also mutation-tested every fix. Six of seven reverts were killed; **one survived** — the exact-arity refusal had *zero* coverage, so deleting it passed all 55 tests. Now pinned, and the new test verifiably kills that mutation. It also caught that `test_integration_separator_in_subject_cannot_inject` didn't pin what its name claims: one or two injected separators break arity and get absorbed, so the test passed even with `\x1f` framing restored. It takes **three** separators to restore a multiple-of-3 arity and expose the framing — added. Its independent false-arm hunt: **21 adversarial messages, zero false arms** (CRLF verbatim, 5000-char subjects, a subject that *is* a trailer line, Turkish-İ casing, invalid UTF-8, merge commits, literal `%x00`/`%H`/`%B` text). It independently found all **38** historical tokened commits and confirmed every one still arms. ### CI The earlier `decisions lifecycle` red was the known flake, ruled out concretely rather than assumed: a heading diff against the merge-base showed **one added, none removed or renamed** (the failure mode that impersonates the flake), and both job steps passed locally at that exact head. It is green on the rebased head. Rebased onto current `main` — the append-vs-append conflict was resolved keeping all three records and verified numerically: `main` has 162 active keys, this branch 163 = main's 162 + one new, **zero lost**. 8/9 checks green; the only outstanding job is the advisory `Build & push image`, which is not a required context. Review-verdict: MERGEABLE @ 1ef581403c3524a85c68b7f5fc59bd0fdfed0f45

timothy commented 2026-07-25 18:54:02 +02:00

Author

Owner

Copy Link

Copy Source

Closing in favour of #618 — duplicate work, and theirs is the better design

Two sessions worked #609 concurrently. This PR opened at 16:47; #618's session claimed the issue at 18:15 and opened at 18:42. I was first by ~90 minutes but never claimed the issue, which is the process failure that caused this — not theirs.

Their design is structurally better and I'm deferring to it.

Every HIGH defect five review rounds found in this PR — %s being the first paragraph rather than the first line, the \x1f separator being injectable, lstrip("\n") promoting body prose to subject on a verbatim message — existed only because this PR parses commit subjects by hand. #618 delegates to git's own trailer machinery (--format with unfold, --no-merges), so that entire bug class cannot arise.

They also identified a weakness here that I argued past: this repo's docs commits routinely quote example commit messages, so a subject like docs: explain [decisions-edit] arms this matcher. My "38 historical uses" backward-compatibility argument was weaker than I presented it — those commits are already merged and never re-evaluated. What matters is what contributors type going forward, and retiring the ambiguous bracket marker with a ::warning:: nudge (as #618 does) is the better end state.

Worth carrying over

One finding from round 5 that #618 should confirm it covers: a defensive branch can survive mutation with zero coverage. Here, deleting the exact-arity refusal passed all 55 tests, because nothing fed malformed output to the parser. #618's body claims mutation-verified negative controls, which is the right practice — worth confirming it extends to every defensive branch, not only the happy-path matcher.

Also worth preserving from this thread: five rounds each found a defect introduced by the previous round's fix, which a single review pass would have missed.

Disposition

• Branch fix/609-decisions-edit-token-scope is left pushed, not deleted, so nothing is lost if #618 stalls.
• feat/610 is being rebased off these commits onto #618's branch; #610's work is independent of which token design lands.

No hard feelings toward the diff — it was well-reviewed, just solving a problem that shouldn't have existed.

## Closing in favour of #618 — duplicate work, and theirs is the better design Two sessions worked #609 concurrently. This PR opened at 16:47; #618's session claimed the issue at 18:15 and opened at 18:42. I was first by ~90 minutes but **never claimed the issue**, which is the process failure that caused this — not theirs. **Their design is structurally better and I'm deferring to it.** Every HIGH defect five review rounds found in this PR — `%s` being the first *paragraph* rather than the first line, the `\x1f` separator being injectable, `lstrip("\n")` promoting body prose to subject on a verbatim message — existed **only because this PR parses commit subjects by hand**. #618 delegates to git's own trailer machinery (`--format` with `unfold`, `--no-merges`), so that entire bug class cannot arise. They also identified a weakness here that I argued past: this repo's docs commits routinely quote example commit messages, so a subject like `docs: explain [decisions-edit]` **arms this matcher**. My "38 historical uses" backward-compatibility argument was weaker than I presented it — those commits are already merged and never re-evaluated. What matters is what contributors type going forward, and retiring the ambiguous bracket marker with a `::warning::` nudge (as #618 does) is the better end state. ### Worth carrying over One finding from round 5 that #618 should confirm it covers: **a defensive branch can survive mutation with zero coverage.** Here, deleting the exact-arity refusal passed all 55 tests, because nothing fed malformed output to the parser. #618's body claims mutation-verified negative controls, which is the right practice — worth confirming it extends to every defensive branch, not only the happy-path matcher. Also worth preserving from this thread: five rounds each found a defect **introduced by the previous round's fix**, which a single review pass would have missed. ### Disposition - Branch `fix/609-decisions-edit-token-scope` is left pushed, not deleted, so nothing is lost if #618 stalls. - `feat/610` is being rebased off these commits onto #618's branch; #610's work is independent of which token design lands. No hard feelings toward the diff — it was well-reviewed, just solving a problem that shouldn't have existed.

timothy closed this pull request 2026-07-25 18:54:05 +02:00

timothy referenced this issue from a commit 2026-07-25 19:15:01 +02:00

feat(610): split the decision corpus into one YAML-frontmatter file per record

timothy referenced this issue from a commit 2026-07-25 19:16:22 +02:00

feat(610): split the decision corpus into one YAML-frontmatter file per record

timothy referenced this issue from a commit 2026-07-25 19:45:14 +02:00

feat(610): split the decision corpus into one YAML-frontmatter file per record

timothy referenced this pull request 2026-07-25 20:37:02 +02:00

decisions: split the monolithic corpus into one YAML-frontmatter file per record #610

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#614