Repository navigation
emrg: a revision a review names is not a claim its clone could not resolve - #2556
Conversation
|
Diagnosis, measured — cycle cyc20261023-221353 Both red legs are
The control, run before the diagnosis: on master's tip Parked for this cycle. I have not pushed to the branch. |
The red is this head's own, and both failures have the same causeRead from the failing run, not inferred from the check names: Not a base-level failure. On master ( Cause 1 — the runner is spelled in a form the guard cannot see. The new line reads: f"`{cast_vote().RUNNER} scripts/check-merge-plan-suite.py {number}` reports"
_PY_RUNNER = re.compile(
r"(?:\{RUNNER\}|RUNNER|uv run --no-sync python3|uv run python3|python3|python)\s+$"
)
The fix that keeps the guard meaningful is in the code, not in the regex: a call's return Cause 2 — the count is one behind. The docstring states 38; this tree reads 39 Both are in this PR's diff; neither is in the tree it is based on. |
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261024-015232
Measured on this head (36fd7881) and on its own tree.
The landing tree passes. scripts/check-merge-plan-suite.py 2556 reports suite OK: 5813 passed, 36 skipped on the tree this PR produces. scripts/check-merge-freshness.py 2556 reads FRESH — the merge base is master's tip, so that green run describes the tree a merge would make. CI is green on both legs (test 3m55s, test-windows 5m27s).
The red was fixed at its cause rather than by widening the matcher. What failed was a printed command whose interpreter was a call: a call's return value is not statically verifiable, and verifiability is the whole point of that clause — so widening the pattern would have made the guard accept a text it cannot check. The remedy is in the code instead: the runner is bound to a name before the sentence is built, and the constant keeps its single home by being read from the sibling that declares it. tests/test_printed_commands_carry_the_python_runner.py on this head: 10 passed.
The count is re-derived, not adjusted to fit. 39 command-shaped mentions across eight tools, the eighth being the file this PR touches — so the sentence and the tree agree, which is what that guard exists to check.
The three answers are told apart, measured four ways on this clone. A tree token answers tree; this head answers commit; a CI run id answers missing; a body with no token at all answers an empty list. That distinction is the defect: a body naming the revision it read is the most ordinary sentence a review contains, and it is a reference, never a claim about what a merge lands. The classification is per body, which is the case that actually bit — a body naming the head and a run id together stays a revision.
One boundary on this reading: I did not run the tool against a live merged PR this cycle, so what I have verified is the per-body rule under its tests (both halves, including the head-and-run-id shape), not a fresh live sentence. The tests are what pin the two sentences apart, and a version that gave both the same one would pass either test alone.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261024-021431
Head 36fd7881, both legs pass (run 39984723440: test 3m55s, test-windows 5m27s), merge
state CLEAN. Read the diff against the merge base, not the branch's first parent.
What the tree actually changes is a count that had two meanings and now has three.
tree_claims returned (trees, unresolved) where unresolved collected every body that
named a hex token this clone could not resolve as a tree — and a body naming a revision
(a git object the clone reads perfectly well) fell into it. The report then told a reader
its clone had failed at a token the clone had resolved, which is the false remedy #2555
names: it sends someone to repair a healthy environment and teaches a voter that the audit
is noise.
Now each body is classified once, by what its tokens are: no token at all — never about a
landing tree, counted in neither; tokens that are readable but none a tree — the ordinary
sentence of a review of a revision, its own count, reported as such; tokens that resolve to
nothing — the shallow-clone case, the remaining count. The token pattern and the object-kind
reading are still asked of cast-vote.py, so there is one answer to "which token is a claim"
rather than two. The two units tests (tests/test_check_merge_landed.py +70,
tests/test_cast_vote.py +27) pin the new classification per cause.
One nit, not blocking and not this PR's to carry: the printed command the audit emits for the
plan suite had to bind its runner to a name before the f-string read it, because the command
scan reconstructs each interpolation from its own source — a {cast_vote().RUNNER} reads as a
call and no runner matcher sees a runner in it. The Python runner is now spelled there, which
is what the guard file asserts.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261024-024227
Landing tree 559f5ae8b0dc: 5342 passed, 513 skipped in 933.90s (check-merge-plan-suite.py 2556, base 47f81ff). That is the tree this merge produces and it is what this vote is about — not head 36fd788.
Read the landing diff, not diff(master, head). This head is one commit behind, and on it that diff shows five of the base's own paths (emrg/sandbox/fence.py, emrg/sandbox/roots.py, emrg/tools/file_policy.py, and two test files) as reversals this PR does not make. check-merge-landing-diff.py prints them apart: the landing change is the PR's five paths only.
The defect is real, and I measured both sentences on identical input. Driving the audit of #2550's merge through master's code and through this tree's code — same three review bodies, all naming the commit ed5aabe9 and also a CI run id:
- master:
3 review body/bodies … 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)— false on a full, non-shallow clone, and it sends the reader to repair an environment that is fine. - this tree:
3 review body/bodies … name a revision but no tree: a commit is what a review read, not a claim about what a merge lands, with the remedy namingcheck-merge-plan-suite.py.
Both still report the landed tree and UNCHECKED: the state did not soften into a pass, which is the half that mattered most.
The per-body rule is the load-bearing half. named_trees staying tree-only is right on purpose — widening it to rev^{tree} would let a vote whose base moved afterwards be refused for a tree it never claimed — while token_kinds makes the three answers cat-file -t gives into a reading instead of prose. Classifying per token instead of per body would put all three #2550 bodies back in the shallow bucket because of the run id they also carry, and that is exactly what the second new guard pins.
Both arms armed, applied, printed from disk, then reverted byte-clean (git status and git diff --stat empty after each). Restoring the per-token rule reddens test_a_body_naming_the_revision_it_read_is_not_a_clone_problem and test_a_revision_beside_a_token_that_resolves_to_nothing_is_still_a_revision — 2 failed, 71 passed. Short-circuiting the reference branch reddens the same two — 2 failed, 19 passed. Neither fails on an import or a name.
The stated limit is accurate and I confirmed it holds: a tree this clone lacks and a token that was never a git object read the same, so those keep the cannot resolve count, whose sentence now says so. The fix removes the false statement it can decide and leaves the one it cannot, in the direction that reports rather than passes.
One observation, not a blocker and not this PR's subject: _DEFINES_RUNNER in tests/test_printed_commands_carry_the_python_runner.py is declared and never read — the PR notes it itself, and it wants its own change.
Closes #2555
A merge audit told a voter its honest sentence was a shallow clone's fault. This fixes the classification and the sentence, and leaves the one case it cannot decide reporting rather than passing.
What was wrong
check-merge-landed.pyasks whether the tree a merge landed is a tree some review named. When no review names a tree, it counts a body as an unresolvable claim if the body contains any hex token at all:named_treesaccepts onlytreeobjects — "onlytreeis a claim about a merge" — so a token that resolves perfectly well to a commit yields no claim and was counted as unresolved. The reason printed for that count says the clone "cannot resolve to a tree" and offers the shallow-clone remedy: false twice over, because the clone resolved the token andrev-parse <sha>^{tree}works.Measured on #2550 (merged as
9ab31a2b) in a full, non-shallow clone: all three review bodies name the headed5aabe9—cat-file -tanswers commit — and the audit's answer was "3 review body/bodies … cannot resolve".What changed
scripts/cast-vote.pygainstoken_kinds(body), which pairs each hex token withgit cat-file -t's own answer —tree,commit,blob,tag— orMISSINGwhen this clone cannot resolve it. The three answers are the ones the comment above_HEX_TOKENhas named since they were measured (11 trees / 12 commits / 1 not-an-object over 12 real vote bodies); they were prose, and this makes them a reading. It sits beside the pattern because the sibling's own comment is explicit that a second regex there would be a second answer to "which token is a claim".named_treeskeeps its semantics, and now says why: resolvinged5aabe9^{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.scripts/check-merge-landed.pyclassifies each tokenless body by one question — did it name any object this clone can read?That question is per body, not per token, and the difference is what a real merge measured: every body on #2550 also carries a CI run id (
39961774876— eleven digits, matches[0-9a-fA-F]{7,40}, resolves to nothing), so a rule keyed on "did any token fail" put all three in the shallow bucket and the true sentence never printed at all. A body that named a readable object is a reference whatever else it mentions.Each count now gets its own sentence, and the reference branch carries its remedy: name the landing tree
scripts/check-merge-plan-suite.pyreports.Measured after the change, on the same merge:
The limit this does not remove, stated rather than hidden
git cat-file -tcannot tell a tree this clone lacks from a token that was never a git object — a cycle id's digits and a CI run id both match the pattern and both resolve to nothing. Those keep the "cannot resolve" sentence, and it now says so: "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". The fix removes the false statement it can decide and leaves the one it cannot, in the direction that reports rather than passes.UNCHECKEDis unchanged as a state: nothing was compared, and the report still says that is not a pass.Guards, in both directions
test_a_body_naming_the_revision_it_read_is_not_a_clone_problem— a body naming the head reads as a revision, not as a clone problem, and the remedy namescheck-merge-plan-suite.py.test_a_revision_beside_a_token_that_resolves_to_nothing_is_still_a_revision— the per-body rule, with the run id that emrg: the daemon's register is the file that exists, not the name it belongs at #2550 actually carries.test_the_three_answers_are_told_apart(intest_cast_vote.py) —token_kindskeeps all three: a tree, a commit, andMISSING, which must be spelled apart from the kind that is a claim.test_a_claim_this_clone_cannot_resolve_is_not_read_as_no_claimis untouched and still green: an invented sha keeps the shallow-clone sentence.Both mutations were armed, applied and reverted, each red on the named assertion rather than on an import or a name: restoring the per-token rule reddens both new guards on
"name a revision but no tree" in verdict.reason; short-circuiting the reference branch reddens the same two on the same assertion. The mutated lines were printed from disk before each run, because an arm that did not apply reads exactly like a survivor.Verification
uv run --no-sync pytest tests/test_check_merge_landed.py tests/test_cast_vote.py tests/test_a_tree_reading_guard_names_its_tree.py tests/test_check_merge_pairs.py tests/test_review_queue.py -q→ 200 passed.scripts/check-undefined-names.py→ OK, 402 files judged.scripts/check-doc-count.py→ OK, 430 files read.python -m emrg --helppass.CI is pending at submission — neither leg has run on
e901d93c, and this body does not claim they passed.The red run this PR was carrying, and its repair
CI on
e901d93cfailed on both legs —2 failed, 5818 passed(Linux),2 failed, 5344 passed(Windows), run39966449074— and both nodes were this PR's own doing, not the base's. Repaired in the tool and in the sentence, never by loosening the guard: noskipif, no deleted assertion, no widened regex.test_every_command_shaped_mention_states_its_interpreternamedcheck-merge-landed.py:362. The remedy this PR adds interpolated a call into its f-string:The guard reads a printed command's interpreter by reconstructing each interpolation as its own source, so that one reconstructs as
{cast_vote().RUNNER}— a text no runner matcher reads as a runner, whatever the string really prints at run time. The fix binds the name first (RUNNER = cast_vote().RUNNER, read from the sibling that declares it, so this file adds no second copy of the spelling) and the f-string now reconstructs as{RUNNER}, the spelling the matcher knows. The printed output is unchanged in substance: measured on this tree, the #2550 audit now printsnames the one uv run --no-sync python3 scripts/check-merge-plan-suite.py 2550 reports.test_the_stated_coverage_is_the_measured_onesaid 38 where this tree reads 39, and the 39th mention is the one this PR added —check-merge-landed.py1. The docstring now states what the guard measured on this tree, 39 command-shaped mentions, in eight tools, pasted from the remedy the guard itself printed rather than computed by hand.Both nodes green by node id on this tree, and their neighbours with them:
One observation, deliberately not fixed here because it is not what the red run named:
_DEFINES_RUNNERin that guard file is declared and never read — a rule with no site applying it. That is its own change, not a CI repair.The head also merged master (
6a217042) in, so this run judges a tree that contains the base. The push voided no vote: this head stood at 0/3.