fix(643): close the status-string dodge, pin the allow-list anchors, fix two doc claims

Round-3 review (MERGEABLE, all findings Low/Nit) — applied anyway, because each is the
over-claim or unpinned-anchor family this PR keeps hitting.

LOW — the `renamed => previous_filename REQUIRED` clause matched `.status` by exact
lowercase string, so any other value took the `else true` branch: a row with
`"Renamed"`, or with no status at all, validated fine and silently dropped its SOURCE
path, letting `git mv ErsatzTV/Program.cs -> docs/a.md` read as docs-only. `.status` is
now checked against a closed set and an unknown value fails closed.

Two things that fix taught me, both caught by my own positive control rather than by
review:
  1. The first predicate was WRONG in a way that gated everything: inside
     `[...] | index(.status)`, jq's `.` is the ARRAY, so `.status` was null and every
     row failed. `$s` is now bound from the row before the context switches. A
     security check that rejects everything looks identical to a working one from the
     failing side — only test_gitea_real_status_values_are_accepted caught it.
  2. The set includes BOTH `changed` and `modified`. Live Gitea 1.25.4 emits `changed`,
     but a closed allow-list built from the wrong vocabulary is worse than the hole it
     closes: it would gate every genuine docs-only PR. The property wanted is "reject
     what we don't recognise", not "enumerate one version exactly".

LOW — three allow-list anchors had no test at all: dropping `^` from the `docs/`
alternative (`ErsatzTV/docs/Program.cs` would exempt), dropping `$` from `.md`
(`x.md.cs` would exempt), and dropping the non-empty-list guard. Since the round-3 `..`
finding WAS an anchor subversion, they are now pinned; all three mutation-verified.

NIT — docs/ci-cd.md called this job "a checkout plus a pure-stdlib pytest run", which
the same file contradicts 450 lines later and which this PR's own record names as the
bug that turned the job red on its first CI run. Also replaced a frozen "111 tests" with
an explicitly indicative figure — the suite is ~190 after rebasing onto main, and a
frozen count has rotted four times this session.

198 passed under both jq 1.8.2 and jq 1.6.

Refs #643, #631

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-07-26 13:24:20 +02:00
co-authored by Claude Opus 5
parent 54c875414c
commit f4473926d4
4 changed files with 70 additions and 5 deletions
+13
View File
@@ -134,6 +134,18 @@ while [ "$page" -le 40 ]; do
# `..` is rejected for the same reason: the allow-list anchors `^docs/`, so
# `docs/../ErsatzTV/Program.cs` matches it. Git will not produce such a path, but this guard's
# whole job is to fail closed on unexpected 2xx shapes rather than to assume a well-behaved peer.
#
# `.status` is checked against a CLOSED set, verified against live Gitea 1.25.4 output:
# added|deleted|changed|renamed|copied. Without it, the `renamed => previous_filename REQUIRED`
# clause could be dodged by any other value — `"Renamed"` with a capital R, or an absent status —
# letting a `git mv ErsatzTV/Program.cs -> docs/a.md` drop its source path and read as docs-only.
# An unknown status now fails closed rather than silently taking the `else true` branch.
#
# `modified` is accepted ALONGSIDE `changed` deliberately. Live Gitea 1.25.4 emits `changed`, but
# a closed allow-list built from the wrong vocabulary is a worse failure than the hole it closes:
# it would gate every genuine docs-only PR, on every version that spells it differently. The
# security property here is "reject values we do not recognise", not "enumerate one version
# exactly", so the set errs toward accepting plausible synonyms.
if ! printf '%s' "$raw" \
| jq -e 'def ok: type == "string" and length > 0
and (test("[\\r\\n]") | not)
@@ -141,6 +153,7 @@ while [ "$page" -le 40 ]; do
type == "array" and all(.[];
(.filename | ok)
and (.previous_filename == null or (.previous_filename | ok))
and ((.status // "") as $s | ["added","deleted","changed","modified","renamed","copied"] | index($s))
and (if .status == "renamed"
then (.previous_filename | type == "string" and length > 0)
else true end))' \
+4 -3
View File
@@ -149,8 +149,9 @@ rather than in `docker-build.yml` — see that section (ersatztv#535).
**`small` is git-only, and that is load-bearing (server-management#639).** Everything in
the lane is a checkout plus a `git diff`: `decisions-guard`, `ci-image-pin`,
`docs-reminder` — plus `script-tests`, which is a checkout plus a pure-stdlib `pytest` run
(ersatztv#631). Nothing there runs a compiler or a `docker build`, which is why the lane
`docs-reminder` — plus `script-tests`, which is a checkout plus a `pytest` run needing only
`pytest` and `pyyaml` (ersatztv#631; it is NOT stdlib-only — that assumption is what turned the
job red on its first CI run, see below). Nothing there runs a compiler or a `docker build`, which is why the lane
can be capped at 1 GiB per job. The lightweight-Python jobs are the deliberate edge of the
"git-only" rule, not an exception to it: `setup-python` + `pip install pytest` + a suite whose
heaviest allocation is a handful of temp-dir git repos stays far under the cap. Route a heavy job here and it will OOM — give it
@@ -608,7 +609,7 @@ compiler/docker build), so it doesn't violate the "small is git-only" lane rule.
> `review-verdict/h10`). Promoting it to required is a branch-protection change, tracked separately.
Runs the repository's Python test suite: `PYTHONPATH=. python3 -m pytest scripts/tests -q`
(111 tests, ~10s). It covers the decision-corpus parser/validator/catalog builder, the ersatztv#610
(~190 tests at time of writing, ~10s; the suite grows, so treat the figure as indicative). It covers the decision-corpus parser/validator/catalog builder, the ersatztv#610
migration-equivalence harness, the merge-consent exemption logic and the ersatztv#622 review-verdict
poster.
@@ -12,7 +12,7 @@ mechanics: '`.gitea/workflows/pr-checks.yml` -> `script-tests`; `docs/ci-cd.md`
Until #631 **nothing executed `scripts/tests/`**. No workflow and no Husky hook invoked `pytest`.
`decisions-guard` runs `decisions_validate.py` and `build_decisions_catalog.py` directly — it
exercises that *code* but never its *tests* — and the `test` job is `dotnet test` only. So the 111
exercises that *code* but never its *tests* — and the `test` job is `dotnet test` only. So the
tests guarding the decision corpus, the #610 migration-equivalence harness, the merge-consent
exemption logic and the #622 review-verdict poster were caught only if someone happened to run
pytest locally. #622's suite was very nearly shipped in the belief that it was enforced.
+52 -1
View File
@@ -228,10 +228,15 @@ def test_ordinary_row_without_previous_filename_is_still_valid(hook):
Requiring it globally would reject every normal modified/added row and make the gate refuse
all exemptions — which the 'withholds' tests above could not distinguish from working.
Every row carries a `status`: since the round-3 hardening an ABSENT status fails closed (it
would otherwise dodge the `renamed => previous_filename REQUIRED` clause), which is asserted by
`test_a_rename_disguised_by_an_unknown_status_is_rejected[None]`. The statusless row this test
used to carry was incidental to what it is actually pinning.
"""
hook.set_pages([{"filename": "docs/a.md", "status": "modified"},
{"filename": "docs/b.md", "status": "added"},
{"filename": "docs/c.md"}])
{"filename": "docs/c.md", "status": "changed"}])
assert hook.exempted() is True
@@ -429,3 +434,49 @@ def test_head_moving_mid_enumeration_withholds_the_exemption(hook, tmp_path):
(tmp_path / "state" / "pr_sha_after.txt").write_text("b" * 40)
hook.set_pages(_rows([f"docs/f{i}.md" for i in range(30)]), [])
assert hook.exempted() is False
@pytest.mark.parametrize("status", ["Renamed", "RENAMED", "bogus", None])
def test_a_rename_disguised_by_an_unknown_status_is_rejected(hook, status):
"""`renamed => previous_filename REQUIRED` was keyed on an exact lowercase string, so any other
value took the `else true` branch: a `git mv ErsatzTV/Program.cs -> docs/a.md` row whose status
is `"Renamed"` (or absent) validated fine and silently dropped its SOURCE path, reading as
docs-only. `.status` is now checked against the closed set Gitea actually emits."""
row = {"filename": "docs/a.md"}
if status is not None:
row["status"] = status
hook.set_pages([row], [])
assert hook.exempted() is False
def test_gitea_real_status_values_are_accepted(hook):
"""Positive control for the closed set. The real Gitea 1.25.4 value for an edit is `changed`,
NOT `modified` — a closed allow-list built from the wrong vocabulary would reject every real
docs-only PR, which is a far worse failure than the hole it closes."""
hook.set_pages([{"filename": "docs/a.md", "status": "changed"},
{"filename": "docs/b.md", "status": "added"},
{"filename": "docs/c.md", "status": "deleted"}], [])
assert hook.exempted() is True
# --- allow-list ANCHOR pins (round-3 review: three surviving mutants) --------------------------
# The round-3 `..` finding was an anchor subversion, and mutating the anchors showed no test
# covered them: dropping `^` from the docs/ alternative, or `$` from `.md`, both survived.
def test_docs_must_be_a_PREFIX_not_a_substring(hook):
"""Dropping `^` would exempt `ErsatzTV/docs/Program.cs`."""
hook.set_pages([{"filename": "ErsatzTV/docs/Program.cs", "status": "changed"}], [])
assert hook.exempted() is False
def test_md_must_be_a_SUFFIX_not_a_substring(hook):
"""Dropping `$` would exempt `x.md.cs`."""
hook.set_pages([{"filename": "ErsatzTV/x.md.cs", "status": "changed"}], [])
assert hook.exempted() is False
def test_an_empty_file_list_is_never_exempt(hook):
"""`[ -n "$files" ]` guards this: a PR whose enumeration yields no paths must not read as
'all of its files are docs'."""
hook.set_pages([])
assert hook.exempted() is False