diff --git a/scripts/cast-vote.py b/scripts/cast-vote.py index 0f3c8b17..5558a477 100644 --- a/scripts/cast-vote.py +++ b/scripts/cast-vote.py @@ -413,6 +413,40 @@ def _merge_run( #: ambiguity. Uppercase is accepted because a SHA has no case. _HEX_TOKEN = re.compile(r"[0-9a-fA-F]{7,40}") +#: What a token gets when this clone cannot resolve it at all. Not a git object kind, +#: which is exactly why it is spelled apart from the words `cat-file -t` answers. +MISSING = "missing" + + +def token_kinds(body: str) -> list[tuple[str, str]]: + """Every hex token the body names, paired with what this clone says it is. + + ``kind`` is `git cat-file -t`'s own answer — ``tree``, ``commit``, ``blob``, + ``tag`` — or :data:`MISSING` when the clone cannot resolve the token at all. + Tokens keep the abbreviation they were written with and appear once, in body + order: a caller asking "what is this token" is not asking for its full spelling. + + **The three answers exist to be told apart**, and the comment above `_HEX_TOKEN` + has named them since they were measured: over that sample the tokens split 11 + trees / 12 commits / 1 not-an-object. A token that resolves to a **commit** is an + ordinary reference to a revision a review read — "I read the diff at ``ed5aabe9``" + is what a review of a revision contains — and it is *not* a claim that a merge + lands that tree. A reader that could only ask "is this a tree?" had nowhere to put + that answer, and one did: `check-merge-landed.py` counted such a body as a token + its clone could not resolve, and told the voter its honest sentence was a shallow + clone's fault (#2555). + + The `MISSING` answer is **not** a claim that the token named a tree. It cannot tell + a tree this clone lacks from a token that was never a git object — a CI run id + matches the pattern and resolves to nothing — so a caller reporting it says what it + knows (the token did not resolve) and not what it guesses. + """ + out: list[tuple[str, str]] = [] + for token in dict.fromkeys(_HEX_TOKEN.findall(body)): + got = _git("cat-file", "-t", token) + out.append((token, got.stdout.strip() if got.returncode == 0 else MISSING)) + return out + def named_trees(body: str) -> list[str]: """Every git **tree** the body names, as full SHAs, de-duplicated in order. @@ -421,11 +455,16 @@ def named_trees(body: str) -> list[str]: `tree` is a claim about a merge. An abbreviated SHA is resolved to the full one so that a body naming `9188c497904b` and one naming the 40-char form are compared as the same tree rather than as a hit and a miss. + + Deliberately **not** widened to accept a commit: resolving `ed5aabe9^{tree}` + would turn "I read this revision" into "merging lands that tree", and a vote + whose base moved after it was cast would then be refused for a tree it never + claimed. What the extra answers are for is telling the situations apart when a + body names no tree — :func:`token_kinds`. """ out: list[str] = [] - for token in dict.fromkeys(_HEX_TOKEN.findall(body)): - kind = _git("cat-file", "-t", token) - if kind.returncode != 0 or kind.stdout.strip() != "tree": + for token, kind in token_kinds(body): + if kind != "tree": continue resolved = _git("rev-parse", f"{token}^{{tree}}") sha = resolved.stdout.strip() diff --git a/scripts/check-merge-landed.py b/scripts/check-merge-landed.py index ebd0c6f0..8f2ce923 100644 --- a/scripts/check-merge-landed.py +++ b/scripts/check-merge-landed.py @@ -241,21 +241,28 @@ def _tree_of(commit: str) -> tuple[str | None, str]: return got.stdout.strip(), "" -def tree_claims(number: int, repo: str) -> tuple[list[str], int]: - """`(every tree this PR's reviews name, how many bodies named a token that is not one)`. +def tree_claims(number: int, repo: str) -> tuple[list[str], int, int]: + """`(trees named, bodies naming nothing this clone resolves, bodies naming only a revision)`. Read through the counter's paginated helper (one request returns 30 reviews and the newest are the ones that count) and decoded by `cast-vote.py`'s `named_trees`, so the claim read here is the claim that was checked there. - The second element is the honest half of a limit `named_trees` carries by design: - it resolves each token against *this* clone, so a body naming a tree this checkout - does not have returns no claim — indistinguishable, in the bare list, from a body - naming nothing. Those are different situations with different repairs (a shallow - clone or pruned plan worktrees, versus a vote that was never about a landing tree), - so they are counted apart and reported apart. The token pattern is asked of the - sibling that owns it; a second regex here would be a second answer to "which token - is a claim". + `named_trees` answers "trees named here", so a body naming no tree returns an empty + list — and that empty list has **three** causes with three different repairs: + + * a token this clone cannot resolve at all (a shallow clone, pruned plan worktrees, + or a token that was never a git object) — the first count; + * a **revision**, the ordinary sentence a review of a revision contains, which is a + reference and not a claim about what a merge lands — the second count; + * no token at all, and the body was never about a landing tree — neither count. + + Counting the first and the second together made the report say a clone had failed at + a token it had resolved (#2555), which sends its reader to repair an environment that + is fine and teaches a voter that the audit is noise. Each count is per body, and a + body that names a tree is in neither. The token pattern and the object-kind reading + are both asked of the sibling that owns them; a second regex here would be a second + answer to "which token is a claim". """ reviews = counter()._gh_json_paginated( [ @@ -267,6 +274,7 @@ def tree_claims(number: int, repo: str) -> tuple[list[str], int]: ) claims: list[str] = [] unresolved = 0 + referenced = 0 for r in reviews: assert isinstance(r, dict) # The projection must have applied: without it the fields arrive under their @@ -280,12 +288,20 @@ def tree_claims(number: int, repo: str) -> tuple[list[str], int]: ) body = str(r.get("body") or "") found = cast_vote().named_trees(body) - if not found and cast_vote()._HEX_TOKEN.findall(body): - unresolved += 1 for tree in found: if tree not in claims: claims.append(tree) - return claims, unresolved + if found: + continue + kinds = [kind for _token, kind in cast_vote().token_kinds(body)] + if not kinds: + continue # no token at all: never about a landing tree + readable = [kind for kind in kinds if kind != cast_vote().MISSING] + if readable: + referenced += 1 # names objects this clone reads; none is a tree + else: + unresolved += 1 # names nothing readable: the shallow-clone case + return claims, unresolved, referenced def check_pr(number: int, repo: str) -> Verdict: @@ -316,23 +332,52 @@ def check_pr(number: int, repo: str) -> Verdict: if landed is None: raise RuntimeError(f"#{number}: {why}") - claimed, unresolved = tree_claims(number, repo) + claimed, unresolved, referenced = tree_claims(number, repo) if not claimed: + # Each count gets its own sentence because each has its own repair, and one + # suppressing the other hides the cause a reader most needs: measured on #2550, + # three bodies named the reviewed revision *and* a CI run id, so the + # shallow-clone sentence printed for bodies whose tokens resolved fine while the + # revision sentence — the true one — never appeared at all (#2555). + parts: list[str] = [] + if referenced: + parts.append( + f"{referenced} review body/bodies of #{number} name a revision but no " + "tree: a commit is what a review read, not a claim about what a merge " + "lands" + ) + if unresolved: + parts.append( + f"{unresolved} review body/bodies of #{number} name a token this clone " + "cannot resolve to a tree (a shallow clone or pruned plan worktrees read " + "this way; a token that was never a git object — a CI run id matches the " + "pattern — reads the same, and this tool has no question that separates " + "them)" + ) + if not parts: + parts.append(f"no review body of #{number} names a tree") + why = "; ".join(parts) + " - the landed tree was compared to nothing" + if referenced: + # The runner is bound to a name before it is printed, and that name is + # load-bearing rather than cosmetic: the family's guard reads a printed + # command's interpreter by reconstructing each interpolation as its own source + # (`tests/test_printed_commands_carry_the_python_runner.py`), so a *call* + # inside the f-string reconstructs as `{cast_vote().RUNNER}` - a text no runner + # matcher reads as a runner - while `{RUNNER}` reconstructs to the one spelling + # it knows. The constant keeps its single home: it is read from the sibling + # that declares it, so this file adds no second copy of the spelling. + RUNNER = cast_vote().RUNNER + why += ( + ". A vote that is about a landing tree names the one " + f"`{RUNNER} scripts/check-merge-plan-suite.py {number}` reports" + ) return Verdict( pr=number, state=state, landed_tree=landed, claimed=[], reading=UNCHECKED, - reason=( - f"{unresolved} review body/bodies of #{number} name a token this clone " - "cannot resolve to a tree, so no claim could be read (a shallow clone " - "or pruned plan worktrees read this way) - the landed tree was " - "compared to nothing" - if unresolved - else f"no review body of #{number} names a tree, so the landed tree was " - "compared to nothing" - ), + reason=why, ) if landed in claimed: return Verdict( diff --git a/tests/test_cast_vote.py b/tests/test_cast_vote.py index 7211423a..82f8ec8e 100644 --- a/tests/test_cast_vote.py +++ b/tests/test_cast_vote.py @@ -1591,6 +1591,33 @@ def test_the_discriminator_is_gits_answer_not_the_token_shape(mod): ) +def test_the_three_answers_are_told_apart(mod): + """`token_kinds` gives the three answers `named_trees` had to answer "no" to. + + `named_trees` answers one question — is this token a tree? — and answers "no" to a + commit and to a token that is not a git object at all. A consumer holding only that + answer could not tell those apart, and one did: `check-merge-landed.py` counted a + body naming the head as a token its clone could not resolve (#2555). This is the + reading those consumers ask instead, and it must keep all three answers — a + classifier that collapsed them again would put the defect back without touching the + sentence that reported it. + """ + tree = _tree_of() + head = _commit_of() + invented = "0123456789abcdef0123456789abcdef01234567" + + kinds = dict(mod.token_kinds(f"{CYCLE} — {tree} on {head}, absent {invented}")) + + assert kinds[tree] == "tree" + assert kinds[head] == "commit", ( + "the head resolves to an object; it is not a tree this clone lacks" + ) + assert kinds[invented] == mod.MISSING, "a token this clone lacks has no git kind" + assert mod.MISSING != "tree", ( + "the unresolvable answer must be spelled apart from the kind that IS a claim" + ) + + def test_a_tree_named_twice_is_one_claim(mod): """De-duplicated, so a body that quotes a tree twice is not reported as two trees.""" tree = _tree_of() diff --git a/tests/test_check_merge_landed.py b/tests/test_check_merge_landed.py index dc660707..6a9d0d49 100644 --- a/tests/test_check_merge_landed.py +++ b/tests/test_check_merge_landed.py @@ -344,6 +344,76 @@ def test_a_claim_this_clone_cannot_resolve_is_not_read_as_no_claim( ) +def test_a_body_naming_the_revision_it_read_is_not_a_clone_problem( + mod, merged_repo, monkeypatch +): + """A commit token is a reference to what the review read, not an unreadable tree. + + The most ordinary vote body there is names the revision it read — "I read the diff + at " — and that sha answers `commit`, not `tree`. Counting such a body as a + token the clone could not resolve told the voter its honest sentence was a shallow + clone's fault, and sent a reader to repair an environment that had just resolved + every token in the body: measured on #2550, three review bodies naming their head + produced "cannot resolve to a tree", aimed at a full clone (#2555). + + Both halves are pinned — the reason names the revision, and it does not name the + clone — because the absent-tree case beside this one keeps the other sentence, and + a tool that gave both the same one would pass either test alone. + """ + fixture = merged_repo + fake = FakeGh( + _view(fixture["merge_commit"]), + [_review(f"✅ LGTM — cycle cyc20260925-174000\n\nI read the diff at {fixture['head']}")], + ) + _install(mod, monkeypatch, fake) + + verdict = mod.check_pr(1613, "argszero/emrg") + + assert verdict.reading == mod.UNCHECKED, "nothing was compared, and that stays true" + assert "name a revision but no tree" in verdict.reason, verdict.reason + assert "cannot resolve to a tree" not in verdict.reason, ( + "this clone resolved the token; saying it could not is the defect, and it sends " + "its reader to deepen a clone that is already complete" + ) + assert "check-merge-plan-suite.py" in verdict.reason, ( + "the cause has a remedy and the remedy has to be in the sentence: name the " + "landing tree this script reports" + ) + + +def test_a_revision_beside_a_token_that_resolves_to_nothing_is_still_a_revision( + mod, merged_repo, monkeypatch +): + """The classification is per body, not per token — the case a real merge measured. + + Every review body on #2550 names the head **and** a CI run id. A run id matches + `_HEX_TOKEN` (eleven digits) and resolves to nothing, so a rule keyed on "did any + token fail" put all three bodies in the shallow-clone bucket and the true sentence + never printed at all. The question the two situations turn on is whether the body + named **any** object this clone can read: this one names the head, so it is a + reference, and the stray token changes nothing about that. + """ + fixture = merged_repo + fake = FakeGh( + _view(fixture["merge_commit"]), + [ + _review( + "✅ LGTM — cycle cyc20260925-174000\n\n" + f"read at {fixture['head']}, run 39961774876: test 3m41s" + ) + ], + ) + _install(mod, monkeypatch, fake) + + verdict = mod.check_pr(1613, "argszero/emrg") + + assert verdict.reading == mod.UNCHECKED + assert "name a revision but no tree" in verdict.reason, verdict.reason + assert "cannot resolve to a tree" not in verdict.reason, ( + "one unresolvable token must not reclassify a body that named a readable object" + ) + + def test_the_same_tree_named_by_a_later_review_still_counts(mod, merged_repo, monkeypatch): """Any page of reviews counts, so a claim on the newest one is not missed.""" fixture = merged_repo diff --git a/tests/test_printed_commands_carry_the_python_runner.py b/tests/test_printed_commands_carry_the_python_runner.py index 0d5109ca..b86d63bb 100644 --- a/tests/test_printed_commands_carry_the_python_runner.py +++ b/tests/test_printed_commands_carry_the_python_runner.py @@ -73,12 +73,12 @@ what keeps the clause off prose: "update `VERSION_SOURCES` in `scripts/bump-version.py`" ends at the file name and is not a thing to run, while `scripts/check-vote-count.py ` is. -What the clause reads on this tree, measured 2026-10-20: **38** command-shaped mentions, in -seven tools - `bump-version.py` 2, `cast-vote.py` 8, `check-merge-freshness.py` 8, -`check-merge-sequence.py` 1, `check-release-published.py` 1, `check-vote-count.py` 1, -`review-queue.py` 17. This number has gone down as well as up, and it goes down the way it -went up: a *printed remedy* is a printed command either way, so a remedy deleted together -with the tool that printed it takes its mention with it. +What the clause reads on this tree, measured 2026-10-24: **39** command-shaped mentions, in +eight tools - `bump-version.py` 2, `cast-vote.py` 8, `check-merge-freshness.py` 8, +`check-merge-landed.py` 1, `check-merge-sequence.py` 1, `check-release-published.py` 1, +`check-vote-count.py` 1, `review-queue.py` 17. This number has gone down as well as up, and +it goes down the way it went up: a *printed remedy* is a printed command either way, so a +remedy deleted together with the tool that printed it takes its mention with it. `review-queue.py` carried a second `abstain` branch - a head pushed below a window whose lower bound the record corpus could not measure, whose printed remedy named `scripts/check-vote-count.py ` and offered `--prev-cycle ` as the caller's way to