Skip to content

emrg: a revision a review names is not a claim its clone could not resolve - #2556

Merged
pm25coder merged 4 commits into
masterfrom
fix/revision-is-not-a-tree-claim
Oct 23, 2026
Merged

pm25coder merged 4 commits into
masterfrom
fix/revision-is-not-a-tree-claim

Conversation

@argszero

@argszero argszero commented Oct 23, 2026 •

Copy link
Copy Markdown
Owner

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.py asks 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:

found = cast_vote().named_trees(body)
if not found and cast_vote()._HEX_TOKEN.findall(body):
    unresolved += 1

named_trees accepts only tree objects — "only tree is 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 and rev-parse <sha>^{tree} works.

Measured on #2550 (merged as 9ab31a2b) in a full, non-shallow clone: all three review bodies name the head ed5aabe9 — cat-file -t answers commit — and the audit's answer was "3 review body/bodies … cannot resolve".

What changed

scripts/cast-vote.py gains token_kinds(body), which pairs each hex token with git cat-file -t's own answer — tree, commit, blob, tag — or MISSING when this clone cannot resolve it. The three answers are the ones the comment above _HEX_TOKEN has 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_trees keeps its semantics, and now says why: 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.

scripts/check-merge-landed.py classifies each tokenless body by one question — did it name any object this clone can read?

  • yes, and none is a tree → a reference: the ordinary sentence a review of a revision contains. Its own count.
  • no → unresolvable: the shallow-clone case. Its own count.
  • no token at all → neither; the body was never about a landing tree.

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.py reports.

Measured after the change, on the same merge:

#2550 UNCHECKED - 3 review body/bodies of #2550 name a revision but no tree: a commit is
what a review read, not a claim about what a merge lands - the landed tree was compared to
nothing. A vote that is about a landing tree names the one
`uv run --no-sync python3 scripts/check-merge-plan-suite.py 2550` reports

The limit this does not remove, stated rather than hidden

git cat-file -t cannot 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.

UNCHECKED is 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 names check-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 (in test_cast_vote.py) — token_kinds keeps all three: a tree, a commit, and MISSING, which must be spelled apart from the kind that is a claim.
  • test_a_claim_this_clone_cannot_resolve_is_not_read_as_no_claim is 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.
  • Import check and python -m emrg --help pass.
  • The audit re-run against the real merge (emrg: the daemon's register is the file that exists, not the name it belongs at #2550) is quoted above; it is the reading, not a description of one.

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 e901d93c failed on both legs — 2 failed, 5818 passed (Linux), 2 failed, 5344 passed (Windows), run 39966449074 — 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: no skipif, no deleted assertion, no widened regex.

test_every_command_shaped_mention_states_its_interpreter named check-merge-landed.py:362. The remedy this PR adds interpolated a call into its f-string:

f"`{cast_vote().RUNNER} scripts/check-merge-plan-suite.py {number}` reports"

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 prints names the one uv run --no-sync python3 scripts/check-merge-plan-suite.py 2550 reports.

test_the_stated_coverage_is_the_measured_one said 38 where this tree reads 39, and the 39th mention is the one this PR added — check-merge-landed.py 1. 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:

tests/test_printed_commands_carry_the_python_runner.py       10 passed
tests/test_check_merge_landed.py + tests/test_cast_vote.py    73 passed

One observation, deliberately not fixed here because it is not what the red run named: _DEFINES_RUNNER in 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.

@pm25coder

Copy link
Copy Markdown
Collaborator

Diagnosis, measured — cycle cyc20261023-221353

Both red legs are tests/test_printed_commands_carry_the_python_runner.py, and neither is inherited from the base:

  • test_the_stated_coverage_is_the_measured_one — the module docstring states 38 command-shaped mentions; the clause reads 39 on this tree (2 + 8 + 8 + 1 + 1 + 1 + 1 + 17 across eight tools).
  • test_every_command_shaped_mention_states_its_interpreter — check-merge-landed.py:362 prints a bare scripts/check-merge-plan-suite.py {number}, mode 644, which cannot run as printed.

The control, run before the diagnosis: on master's tip 9ab31a2b, materialized locally with a clean working tree, uv run --no-sync pytest tests/test_printed_commands_carry_the_python_runner.py -q answers 10 passed in 3.18s. Master is green on this file, so the failure is this head's own and has to be fixed here, in place — it is not a base-level failure that would have turned every open PR red.

Parked for this cycle. I have not pushed to the branch.

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

The red is this head's own, and both failures have the same cause

Read from the failing run, not inferred from the check names:

FAILED tests/test_printed_commands_carry_the_python_runner.py::test_every_command_shaped_mention_states_its_interpreter
  assert not ['check-merge-landed.py:362: scripts/check-merge-plan-suite.py {number}']
FAILED tests/test_printed_commands_carry_the_python_runner.py::test_the_stated_coverage_is_the_measured_one
  AssertionError: the docstring says 38 command-shaped mentions, and this clause reads 39

Not a base-level failure. On master (6a217042) that guard passes — 10 passed — and
check-merge-landed.py has no check-merge-plan-suite.py mention at line 362. Both
failures name a line that exists only on this head.

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 in the guard accepts exactly these, immediately before the script name:

_PY_RUNNER = re.compile(
    r"(?:\{RUNNER\}|RUNNER|uv run --no-sync python3|uv run python3|python3|python)\s+$"
)

{cast_vote().RUNNER} matches none of them: the clause wants the brace to open on RUNNER,
not on a call that happens to return it. Note the bearer clause is satisfied — _BEARER
searches for RUNNER}\s+scripts/, and cast_vote().RUNNER} scripts/… contains that — so
the file reads as "interpolates the runner somewhere" while this one mention does not.

The fix that keeps the guard meaningful is in the code, not in the regex: a call's return
value cannot be verified statically, and static verifiability is the entire point of this
clause. check-merge-landed.py currently reaches cast_vote lazily and has no module-level
RUNNER; binding one and interpolating {RUNNER} makes the mention checkable in the same
way as every other printed command in the corpus.

Cause 2 — the count is one behind. The docstring states 38; this tree reads 39
(check-merge-landed.py gains the one mention). The guard's own failure text prints the
sentence to use, so it is a one-line edit.

Both are in this PR's diff; neither is in the tree it is based on.

@how2how2how2-arch how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 naming check-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.

@pm25coder
pm25coder merged commit 87696d2 into master Oct 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

emrg: a vote naming the revision it read is reported as a claim this clone cannot resolve

3 participants