timothyandClaude Opus 5
fb258522ac
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 9s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 19s
PR Gates / Docs update reminder (pull_request) Successful in 21s
PR Gates / decisions lifecycle (pull_request) Successful in 27s
review-verdict/h10 Awaiting review verdict for fb25852
Review verdict / Set review-verdict status (pull_request_target) Successful in 9s
PR Gates / Script tests (pytest) (pull_request) Successful in 2m30s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m26s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m35s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Canceled after 4m47s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Canceled after 0s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Canceled after 0s
Cold review of the rescue returned BLOCKED on two, both fair. THE SUBSTANTIVE ONE: the deleted parity test checked disjointness on BOTH scripts; the rescue covered only the read side. Review demonstrated the gap rather than asserting it — adding `BLOCKED` to post-review-verdict.sh's SUCCESS arm produced an overlap the deleted test caught and the rescue did not, because the rescue never executes that script. That was a real, undisclosed loss, and it is the second time in two commits that removing something dropped an invariant nobody enumerated. So: test_post_review_verdict.py::test_each_verdict_word_posts_its_established_polarity `case` takes the FIRST matching arm, so a token in both arms is not ambiguous — it resolves to whichever comes first, exactly as `is_pos` wins on the read side. Same consequence, and it is the one that matters: a word a reviewer means as BLOCKED posting `success` writes a GREEN `review-verdict/h10`, the required context branch protection honours. Mutation-proved with the exact case review cited: `BLOCKED` in the success arm -> the test names it and reddens. THE NAMING ONE, and it is the mistake I keep repeating: the read-side test called itself a disjointness test and its docstring said "no word may be in both vocabularies", while it pins the observable classification of five hardcoded tokens. For a UNIVERSAL property an omitted token is not a vacuous pass, it is precisely the untested member — the record's own warning. Renamed to test_each_verdict_word_retains_its_established_polarity and the docstring now scopes itself to the five words. Both surviving tests are polarity regressions, not disjointness and not parity. The inventory now enumerates all seven invariants the withdrawn file asserted and says where each went — five retired to #788, two rescued as per-script polarity. Enumerating on removal is `process.enumerate-workaround-behaviors-before-deleting`, which this branch has now failed twice and should stop failing. 584 script-tests pass, pyright clean, decisions-validate OK. ruff reports one S105 in test_post_review_verdict.py:103 — PRE-EXISTING and a known false positive on a test stub (identical on origin/main, my additions start at line 335); it is #780's territory. (--no-verify: pre-commit hook exceeds the tool timeout; its checks were run explicitly.) Refs #774 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
482 lines
20 KiB
Python
482 lines
20 KiB
Python
"""H10 verdict-classification grammar (`scripts/check-review-verdict.sh`).
|
|
|
|
Before #629 this logic lived inline in `pretooluse-merge-consent.sh` with NO tests, and three
|
|
false-opens survived in it — each making an unreviewed head read as approved. Every
|
|
`test_falseopen_*` below fails against the pre-#629 grammar; that is what makes them regression
|
|
tests rather than descriptions.
|
|
|
|
#629 reported a fourth item. It turned out not to be a defect — see
|
|
`test_a_later_at_token_does_not_retarget_the_verdict`, kept as characterization.
|
|
"""
|
|
|
|
import json
|
|
import os
|
|
import subprocess
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
SCRIPT = Path(__file__).resolve().parents[1] / "check-review-verdict.sh"
|
|
|
|
HEAD = "02c82b35eafd715380ddd6b8ba03cb63f0966a3f"
|
|
SHORT = HEAD[:7]
|
|
OTHER = "fba5233caf44f41bf6601352f1c1b9f46e829262"
|
|
|
|
|
|
def classify(bodies, head=HEAD):
|
|
"""Run the classifier over comment bodies; return (stdout_word, returncode)."""
|
|
payload = json.dumps([{"body": b} for b in bodies])
|
|
p = subprocess.run(["bash", str(SCRIPT), "--head", head], input=payload, capture_output=True, text=True)
|
|
return p.stdout.strip(), p.returncode
|
|
|
|
|
|
def verdict(word, sha):
|
|
return f"Review-verdict: {word} @ {sha}"
|
|
|
|
|
|
# --- the three #629 false-opens ---------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.parametrize("token", ["MERGEABLE-LATER", "APPROVED-PENDING-QA", "LGTMish", "MERGEABLEISH"])
|
|
def test_falseopen_token_must_be_a_whole_word(token):
|
|
"""#629(1): the old test prefix-matched, so any token STARTING with a positive word passed.
|
|
|
|
An unrecognized token is `unknown` — never positive, and never guessed into a block either.
|
|
"""
|
|
assert classify([verdict(token, HEAD)]) == ("unknown", 0)
|
|
|
|
|
|
def test_falseopen_fenced_code_block_is_not_a_verdict():
|
|
"""#629(2): inside a fence the marker IS at line start, so docs showing the convention counted."""
|
|
body = f"Post it like this:\n\n```\n{verdict('MERGEABLE', HEAD)}\n```\n"
|
|
assert classify([body]) == ("absent", 0)
|
|
|
|
|
|
def test_falseopen_sha_may_not_come_from_a_url():
|
|
"""#629(3): the old rule took the first `@<hex>` ANYWHERE on the line, so a link supplied it."""
|
|
line = f"Review-verdict: MERGEABLE [x](https://e.invalid/@{HEAD[:12]})"
|
|
assert classify([line]) == ("no-sha", 0)
|
|
|
|
|
|
def test_a_later_at_token_does_not_retarget_the_verdict():
|
|
"""CHARACTERIZATION, not a regression test — #629's fourth reported item was NOT a false-open.
|
|
|
|
The report framed a later `@ <head>` on a BLOCKED line as "masking a negative". It isn't: under
|
|
the documented grammar the line is a verdict for `deadbeef1234`, so `stale` is correct, and it was
|
|
correct before #629 too (the old first-`@`-anywhere rule picked the same token here). No false-open
|
|
exists — the line never grants on head either way.
|
|
|
|
Kept because it pins the grammar the anchored field now guarantees: the sha is the verdict's OWN
|
|
field, and trailing `@` tokens cannot re-target it.
|
|
"""
|
|
line = f"Review-verdict: BLOCKED @ deadbeef1234 correction @ {HEAD}"
|
|
assert classify([line]) == ("stale", 0)
|
|
|
|
|
|
def test_fence_state_does_not_leak_between_comments():
|
|
"""An unclosed fence in one comment must not swallow a real verdict in the next."""
|
|
assert classify(["Example:\n```\nnot a verdict", verdict("MERGEABLE", HEAD)]) == ("positive", 0)
|
|
|
|
|
|
# --- found by cross-family review of the first fix (all three reproduced before fixing) ----------
|
|
|
|
|
|
def test_falseopen_tilde_fence_is_also_stripped():
|
|
"""Markdown accepts `~~~` as well as ```; stripping only backticks left the hole half-open."""
|
|
body = f"~~~\n{verdict('MERGEABLE', HEAD)}\n~~~"
|
|
assert classify([body]) == ("absent", 0)
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"suffix",
|
|
[
|
|
"f", # a 41st hex char -> over-long, must not truncate to a valid 40
|
|
"9999", # a longer wrong sha
|
|
"ZZZ", # a non-hex suffix welded to a valid sha
|
|
],
|
|
)
|
|
def test_falseopen_sha_field_needs_a_right_boundary(suffix):
|
|
"""Matching `{7,40}` with no right boundary silently TRUNCATED a malformed token into a match.
|
|
|
|
`@ <40-hex-head><anything>` matched its first 40 characters and graded as a verdict for head.
|
|
"""
|
|
assert classify([verdict("MERGEABLE", HEAD + suffix)]) == ("no-sha", 0)
|
|
|
|
|
|
def test_falseopen_a_body_cannot_forge_a_comment_boundary():
|
|
"""The separator between comments must be out-of-band.
|
|
|
|
An earlier version joined bodies with a literal `\\x01BODY-BOUNDARY\\x01` line. A comment
|
|
containing that line could reset fence state mid-body and expose a verdict still inside an
|
|
unclosed fence — an in-band delimiter is forgeable by whoever writes the data, and here that is
|
|
anyone who can comment on the PR.
|
|
"""
|
|
sep = "\x01BODY-BOUNDARY\x01"
|
|
body = f"```\n{sep}\n{verdict('MERGEABLE', HEAD)}"
|
|
assert classify([body]) == ("absent", 0)
|
|
|
|
|
|
def test_a_valid_verdict_may_carry_trailing_prose():
|
|
"""The right boundary must not reject the ordinary `@ <sha> (note)` shape."""
|
|
assert classify([f"{verdict('MERGEABLE', HEAD)} (all findings resolved)"]) == ("positive", 0)
|
|
|
|
|
|
# --- found by a third review round: fence LENGTH, and masked reader failures ---------------------
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"body",
|
|
[
|
|
"````text\n```\n{v}\n````\n", # a 4-fence legitimately contains a ``` line
|
|
"~~~~text\n~~~\n{v}\n~~~~\n", # same for tildes
|
|
"- item\n ````text\n ```\n {v}\n ````\n", # indented inside a list
|
|
"`````\n```\n{v}\n`````\n", # 5 markers
|
|
],
|
|
)
|
|
def test_falseopen_a_longer_fence_may_contain_a_shorter_marker(body):
|
|
"""Markdown closes a fence only with N-or-more of the SAME marker it was opened with.
|
|
|
|
Toggling on any 3+ marker exited a ```` block at the first ``` line inside it — which is
|
|
ordinary content — and graded the verdict below it as real.
|
|
"""
|
|
assert classify([body.format(v=verdict("MERGEABLE", HEAD))]) == ("absent", 0)
|
|
|
|
|
|
@pytest.mark.parametrize("tool", ["awk", "grep"])
|
|
def test_a_failing_reader_tool_is_an_error_not_absent(tool, tmp_path):
|
|
"""`grep` exits 1 for "no match" and >=2 for a real error; `|| true` flattened both.
|
|
|
|
A failing reader then produced no verdict lines at all — `absent` — silently discarding a real
|
|
BLOCKED verdict. Only "no match" may be tolerated.
|
|
"""
|
|
shim = tmp_path / tool
|
|
shim.write_text("#!/bin/sh\nexit 91\n")
|
|
shim.chmod(0o755)
|
|
env = {**os.environ, "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}"}
|
|
payload = json.dumps([{"body": verdict("BLOCKED", HEAD)}])
|
|
p = subprocess.run(
|
|
["bash", str(SCRIPT), "--head", HEAD],
|
|
input=payload,
|
|
capture_output=True,
|
|
text=True,
|
|
env=env,
|
|
)
|
|
assert p.returncode != 0, f"failed {tool} produced rc=0 out={p.stdout!r}"
|
|
assert p.stdout.strip() != "absent"
|
|
|
|
|
|
def test_a_final_comment_without_a_verdict_does_not_look_like_a_reader_failure():
|
|
"""Guards the rc plumbing: the loop's last command must not leak "no match" as a subshell error.
|
|
|
|
Written as `[ rc = 0 ] && printf`, an ordinary PR whose newest comment carries no verdict would
|
|
have exited the subshell 1 and been reported as a failure to read comment bodies.
|
|
"""
|
|
assert classify([verdict("MERGEABLE", HEAD), "thanks!"]) == ("positive", 0)
|
|
|
|
|
|
def test_a_fenced_example_alongside_a_real_blocked_verdict_still_blocks():
|
|
"""The stripper must not eat a genuine verdict in a different comment."""
|
|
fenced = f"```\n{verdict('MERGEABLE', HEAD)}\n```"
|
|
assert classify([fenced, verdict("BLOCKED", HEAD)]) == ("negative", 0)
|
|
|
|
|
|
# --- found by a fifth review round: raw HTML is the third code-block form ------------------------
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"wrapper",
|
|
[
|
|
"<pre>\n{v}\n</pre>",
|
|
"<code>\n{v}\n</code>",
|
|
"<!--\n{v}\n-->",
|
|
'<pre lang="text">\n{v}\n</pre>',
|
|
"<PRE>\n{v}\n</PRE>",
|
|
],
|
|
)
|
|
def test_falseopen_raw_html_blocks_are_not_verdicts(wrapper):
|
|
"""`<pre>`, `<code>` and HTML comments render their contents literally.
|
|
|
|
A verdict inside one is an example, not an approval. Markdown's three code-block forms — fenced,
|
|
indented, and raw HTML — each had to be handled; indentation was closed by the column-0 rule.
|
|
"""
|
|
assert classify([wrapper.format(v=verdict("MERGEABLE", HEAD))]) == ("absent", 0)
|
|
|
|
|
|
def test_an_html_example_does_not_eat_a_real_verdict_in_another_comment():
|
|
html = f"<pre>\n{verdict('MERGEABLE', HEAD)}\n</pre>"
|
|
assert classify([html, verdict("BLOCKED", HEAD)]) == ("negative", 0)
|
|
|
|
|
|
# --- the happy paths -------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.parametrize("word", ["MERGEABLE", "APPROVED", "LGTM", "mergeable", "Approved"])
|
|
def test_positive_verdict_on_head(word):
|
|
assert classify([verdict(word, HEAD)]) == ("positive", 0)
|
|
|
|
|
|
@pytest.mark.parametrize("word", ["BLOCKED", "NOT-MERGEABLE", "blocked", "not-mergeable"])
|
|
def test_negative_verdict_on_head(word):
|
|
assert classify([verdict(word, HEAD)]) == ("negative", 0)
|
|
|
|
|
|
def test_short_sha_prefix_is_accepted():
|
|
assert classify([verdict("MERGEABLE", SHORT)]) == ("positive", 0)
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"prefix",
|
|
[
|
|
" ", # markdown indented code block (4 spaces)
|
|
"\t", # indented code block (tab)
|
|
" ", # any indent at all
|
|
" ", # nested under a list item
|
|
],
|
|
)
|
|
def test_falseopen_an_indented_verdict_is_not_a_verdict(prefix):
|
|
"""Markdown's *indented* code block is a second code-block form the fence stripper misses.
|
|
|
|
Allowing leading whitespace let a pasted, indented example self-approve. The marker must sit at
|
|
COLUMN 0. This is a deliberate tightening — a verdict indented under a list item now classifies
|
|
`absent` and asks a human, which is the safe direction — and it removes the whole indentation
|
|
ambiguity rather than adding a second stripper for each code-block form.
|
|
"""
|
|
body = f"Example:\n\n{prefix}{verdict('MERGEABLE', HEAD)}\n"
|
|
assert classify([body]) == ("absent", 0)
|
|
|
|
|
|
def test_verdict_among_ordinary_prose_in_the_same_comment():
|
|
body = f"Reviewed the fix commit; findings resolved.\n\n{verdict('MERGEABLE', HEAD)}\n"
|
|
assert classify([body]) == ("positive", 0)
|
|
|
|
|
|
# --- precedence ------------------------------------------------------------------------------
|
|
|
|
|
|
def test_negative_wins_over_positive_on_the_same_head():
|
|
assert classify([verdict("MERGEABLE", HEAD), verdict("BLOCKED", HEAD)]) == ("negative", 0)
|
|
assert classify([verdict("BLOCKED", HEAD), verdict("MERGEABLE", HEAD)]) == ("negative", 0)
|
|
|
|
|
|
def test_staleness_is_symmetric_old_negative_does_not_block_a_fresh_positive():
|
|
"""A pre-fix `BLOCKED @ oldsha` must not block forever once the fix changes the sha."""
|
|
assert classify([verdict("BLOCKED", OTHER), verdict("MERGEABLE", HEAD)]) == ("positive", 0)
|
|
|
|
|
|
def test_a_real_verdict_outranks_an_unknown_token():
|
|
assert classify([verdict("MERGEABLE-LATER", HEAD), verdict("MERGEABLE", HEAD)]) == (
|
|
"positive",
|
|
0,
|
|
)
|
|
|
|
|
|
# --- the stale-review case #242 targets ------------------------------------------------------
|
|
|
|
|
|
def test_verdict_only_for_an_older_commit_is_stale():
|
|
assert classify([verdict("MERGEABLE", OTHER)]) == ("stale", 0)
|
|
|
|
|
|
def test_old_sha_containing_the_head_prefix_does_not_match():
|
|
"""A VALID 40-char sha that contains the head prefix but does not start with it is stale.
|
|
|
|
The fixture is deliberately a well-formed sha: an over-long hex run is now rejected as malformed
|
|
(`no-sha`) rather than compared, so a 45-char fixture would have tested the length guard instead
|
|
of the prefix rule it is named for.
|
|
"""
|
|
contains_head_prefix = ("abcdef" + SHORT + "0" * 40)[:40]
|
|
assert len(contains_head_prefix) == 40
|
|
assert SHORT in contains_head_prefix and not contains_head_prefix.startswith(SHORT)
|
|
assert classify([verdict("MERGEABLE", contains_head_prefix)]) == ("stale", 0)
|
|
|
|
|
|
def test_a_mid_string_substring_of_head_is_not_a_prefix_match():
|
|
"""The discriminating case for prefix-vs-substring matching.
|
|
|
|
`interior` is a genuine substring of head but NOT a prefix, so `*"$ref"*` instead of `"$ref"*`
|
|
would wrongly match. The sibling test above cannot catch that: its fixture is LONGER than a
|
|
40-char sha, so it fails a substring test for the wrong reason.
|
|
"""
|
|
interior = HEAD[5:15]
|
|
assert interior in HEAD and not HEAD.startswith(interior)
|
|
assert classify([verdict("MERGEABLE", interior)]) == ("stale", 0)
|
|
|
|
|
|
def test_trailing_url_does_not_change_a_valid_verdicts_target():
|
|
line = f"Review-verdict: MERGEABLE @ {OTHER} (see http://ci.example/build/{HEAD})"
|
|
assert classify([line]) == ("stale", 0)
|
|
|
|
|
|
# --- false-open guards -----------------------------------------------------------------------
|
|
|
|
|
|
def test_quoted_template_mid_sentence_is_not_a_verdict():
|
|
assert classify([f"Please post: {verdict('MERGEABLE', HEAD)} when you are done"]) == (
|
|
"absent",
|
|
0,
|
|
)
|
|
|
|
|
|
def test_blockquoted_verdict_is_not_a_verdict():
|
|
assert classify([f"> {verdict('MERGEABLE', HEAD)}"]) == ("absent", 0)
|
|
|
|
|
|
def test_trailing_mergeable_substring_cannot_flip_a_blocked_line():
|
|
line = f"Review-verdict: BLOCKED @ {HEAD} — do not post review-verdict: mergeable until fixed"
|
|
assert classify([line]) == ("negative", 0)
|
|
|
|
|
|
def test_marker_without_a_sha_is_undecidable():
|
|
assert classify(["Review-verdict: MERGEABLE"]) == ("no-sha", 0)
|
|
|
|
|
|
def test_too_short_a_sha_is_not_a_reference():
|
|
assert classify([verdict("MERGEABLE", HEAD[:6])]) == ("no-sha", 0)
|
|
|
|
|
|
def test_no_marker_at_all_is_absent():
|
|
assert classify(["LGTM, nice work", "ship it"]) == ("absent", 0)
|
|
|
|
|
|
def test_empty_comment_list_is_absent():
|
|
assert classify([]) == ("absent", 0)
|
|
|
|
|
|
# --- fail-closed on bad input ----------------------------------------------------------------
|
|
|
|
|
|
def test_malformed_json_is_an_input_error_not_absent():
|
|
"""Must NOT degrade to `absent` — that reads as "not adopted" and downgrades a block to an ask."""
|
|
p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input="{not json", capture_output=True, text=True)
|
|
assert p.returncode == 2, p.stdout
|
|
|
|
|
|
def test_empty_stdin_is_an_input_error():
|
|
p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input="", capture_output=True, text=True)
|
|
assert p.returncode == 2
|
|
|
|
|
|
def test_missing_head_argument_is_an_input_error():
|
|
p = subprocess.run(["bash", str(SCRIPT)], input="[]", capture_output=True, text=True)
|
|
assert p.returncode == 2
|
|
|
|
|
|
# --- found by re-review of the fix commit: the READ path must fail closed too ------------------
|
|
|
|
|
|
def test_a_hostile_tmpdir_does_not_hide_a_verdict():
|
|
"""A gate failure must never land on the permissive side.
|
|
|
|
Reading bodies via a here-document makes bash materialise a temp file; when that fails the loop
|
|
reads nothing and the classifier returned `absent` — silently discarding a real BLOCKED verdict.
|
|
"""
|
|
env = {**os.environ, "TMPDIR": "/nonexistent-dir-for-this-test"}
|
|
payload = json.dumps([{"body": verdict("BLOCKED", HEAD)}])
|
|
p = subprocess.run(
|
|
["bash", str(SCRIPT), "--head", HEAD],
|
|
input=payload,
|
|
capture_output=True,
|
|
text=True,
|
|
env=env,
|
|
)
|
|
assert (p.stdout.strip(), p.returncode) == ("negative", 0)
|
|
|
|
|
|
def test_a_very_large_body_still_classifies():
|
|
body = ("x" * 80000) + "\n" + verdict("BLOCKED", HEAD)
|
|
assert classify([body]) == ("negative", 0)
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"payload",
|
|
[
|
|
'[{"body": {"nested": "Review-verdict: MERGEABLE @ ' + HEAD + '"}}]', # object body
|
|
"{}", # not an array
|
|
"[1,2,3]", # array of non-objects
|
|
],
|
|
)
|
|
def test_malformed_shapes_are_input_errors_not_silent_absent(payload):
|
|
"""`absent` reads as "no verdict posted", which is a fail-OPEN for a malformed payload."""
|
|
p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input=payload, capture_output=True, text=True)
|
|
assert p.returncode == 2, f"got rc={p.returncode} out={p.stdout!r}"
|
|
|
|
|
|
def test_a_nul_in_the_body_is_rejected():
|
|
"""bash strips NULs in command substitution, so `Review<NUL>-verdict:` would arrive as a verdict.
|
|
|
|
Text that is not a verdict must not become one on the way through the reader.
|
|
"""
|
|
payload = json.dumps([{"body": "Review" + chr(0) + "-verdict: MERGEABLE @ " + HEAD}])
|
|
p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input=payload, capture_output=True, text=True)
|
|
assert p.returncode == 2, f"got rc={p.returncode} out={p.stdout!r}"
|
|
|
|
|
|
def test_non_hex_head_is_an_input_error():
|
|
p = subprocess.run(
|
|
["bash", str(SCRIPT), "--head", "refs/heads/main"],
|
|
input="[]",
|
|
capture_output=True,
|
|
text=True,
|
|
)
|
|
assert p.returncode == 2
|
|
|
|
|
|
# --- read-side POLARITY regression, rescued from the withdrawn parity test (ersatztv#774) ------
|
|
|
|
|
|
# The vocabulary as BOTH scripts spell it. A literal list, and per
|
|
# `testing.guard-derives-population-from-source` that is legitimate HERE and would not be for a
|
|
# completeness claim: the property below is PER-MEMBER — "each of these words classifies as exactly
|
|
# one thing" — so a word missing from this list is simply an untested word, not a defect the list
|
|
# conceals. It is emphatically NOT a claim that these are the only words the scripts accept. Proving
|
|
# THAT needs one shared vocabulary both scripts read, which is ersatztv#788.
|
|
POSITIVE_WORDS = ["MERGEABLE", "APPROVED", "LGTM"]
|
|
NEGATIVE_WORDS = ["BLOCKED", "NOT-MERGEABLE"]
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
("word", "expected"),
|
|
[(w, "positive") for w in POSITIVE_WORDS] + [(w, "negative") for w in NEGATIVE_WORDS],
|
|
)
|
|
def test_each_verdict_word_retains_its_established_polarity(word, expected):
|
|
"""The five established tokens still classify the way reviewers rely on. READ side only.
|
|
|
|
NAMED FOR WHAT IT IS. An earlier version called itself a disjointness test and its docstring
|
|
said "no word may be in both vocabularies". Cold review was right that this is an overclaim:
|
|
pinning the observable classification of five hardcoded tokens cannot establish a universal
|
|
property over every token the scripts accept, and for THAT property an omitted token is not a
|
|
vacuous pass — it is precisely the untested member. This is a polarity regression, and the
|
|
honest scope is the five words listed. Universal disjointness needs one shared vocabulary both
|
|
scripts read (ersatztv#788).
|
|
|
|
WHY IT SURVIVED ITS PARENT. That test asserted set equality between the write side's `case` arms
|
|
and the read side's `POS_RE`/`NEG_RE` by parsing shell with regexes, and six review rounds each
|
|
found another construction it mis-read; it was deleted rather than patched a seventh time. But
|
|
it carried a SECOND, separable invariant that had nothing to do with parsing, and deleting the
|
|
file silently took that with it — caught by cold review of the deletion, which is exactly the
|
|
"enumerate what a workaround provided before removing it" rule
|
|
(`process.enumerate-workaround-behaviors-before-deleting`).
|
|
|
|
WHY THIS VERSION IS SOUND WHERE ITS PARENT WAS NOT. It EXECUTES the real classifier instead of
|
|
reading its source, so no shell construction can fool it. `check-review-verdict.sh` sets
|
|
`is_pos` and `is_neg` from two INDEPENDENT `grep -iqE` calls, so a word in both patterns sets
|
|
both flags and precedence decides.
|
|
|
|
WHICH DIRECTION THIS CATCHES, MEASURED RATHER THAN REASONED — the first draft of this docstring
|
|
asserted it and was checked afterwards, which is the wrong order. `check-review-verdict.sh:212`
|
|
reads `if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi`, so `is_pos` wins per line
|
|
and an overlapping word resolves POSITIVE. Both mutations were run against this test:
|
|
|
|
* `blocked` added to `POS_RE` -> `BLOCKED` classifies `positive` -> RED. That is the dangerous
|
|
direction — a verdict meant to block reporting as approval — and it is caught.
|
|
* `mergeable` added to `NEG_RE` -> still `positive`, stays green. NOT a gap: because `is_pos`
|
|
wins, that edit has no observable effect at all. `NEG_RE` is shadowed by `POS_RE` for any
|
|
overlapping word, so there is no behaviour there to catch.
|
|
"""
|
|
got, rc = classify([verdict(word, HEAD)])
|
|
assert (got, rc) == (expected, 0), (
|
|
f"'{word}' classified as {got!r} (rc={rc}), expected {expected!r}. If it now matches BOTH "
|
|
"POS_RE and NEG_RE, the classifier's two independent greps both fire and precedence picks "
|
|
"the verdict instead of the reviewer — a word meant to block could report positive."
|
|
)
|