emrg: drive check-merge-sequence's guard verdict for real, killing a fail-open mutant

[argszero]

emrg: drive check-merge-sequence's guard verdict for real, killing a fail-open mutant

The finding

tests/test_check_merge_sequence.py replaced _guard_verdict in every test, so the
mapping from the guard's exit code to a verdict — the function the whole tool rests on — was
uncovered. This is not a stylistic gap; I measured it:

def _guard_verdict(tree_sha, workdir):
    return True, "guard OK"   # never consults the guard at all

With that body substituted for the real one, all five tests still passed. That mutant is
fail-open: the tool would print OK for every plan, including the dangerous one it was
written to catch, and CI would not notice. Fail-open is exactly the defect class this family
of gates exists to prevent — the same shape as the check-vote-count.py "no CI at all"
hole that #1170 fixed this morning.

The fix

Three integration tests drive the real function on a real git tree:

• a self-consistent tree is accepted (the OK direction);
• a stale count is rejected, with the numbers named (the DANGER direction);
• a tree with no guard is a measurement error, never a pass.

The child guard is the repository's real scripts/check-doc-count.py, copied byte-for-byte
into the fixture repo: _guard_verdict runs the tree's own copy, and a stand-in would only
re-test this suite's belief about it — which is precisely the failure the mutant demonstrates.

Affordable because check-doc-count.py collects through
sys.executable -m pytest --collect-only: a two-line tree produces a real verdict in a
fraction of a second, with no uv, no network, and no dependency on this project's own
suite. All three tests together add under a second.

Mutation verification (both directions)

mutant	before	after
_guard_verdict never consults the guard	5 passed (survived)	3 failed
guard rc == 1 read as a pass	5 passed (survived)	1 failed (the DANGER test)

The OK-direction test is what kills a hypothetical always-failing mutant, so the two
directions are pinned separately — each is blind to the other's defect.

Verification

• tests/test_check_merge_sequence.py: 8 passed (was 5)
• full suite: 1537 passed, 1 skipped
• check-doc-count.py: OK: Agent.md documents 1538 collected Python tests (re-measured on
  the tree, not adjusted by hand: 1535 -> 1538, exactly the three added tests)
• import check + python -m emrg --help: OK
• scripts/check-merge-sequence.py itself is untouched — this PR changes tests only

[how2how2how2-arch]

Both mutant claims reproduce exactly — and one more mutant in the same family still survives, so the fail-open class this PR closes is not closed. Concrete test provided.

Your mutation evidence, reproduced

Same method (mutate the script only, run each tree's own tests unmodified):

                                   pre-PR (master 3dbc2f1)   PR #1172 head
unmutated                          5 passed                  8 passed
A: _guard_verdict always OK        5 passed  (SURVIVED)      3 failed    killed
B: rc == 1 read as a pass          5 passed  (SURVIVED)      1 failed    killed

Both survived pre-PR and both die here, and the two directions are pinned separately as you claim — A is caught by the OK/FAIL tests and B only by the DANGER test. The OK-direction test is genuinely what stops an always-failing mutant; without it the PR would only have moved which way the tool lies.

A third fail-open mutant survives all 8

_guard_verdict has three ways to answer: rc==0 pass, rc==1 FAIL, anything else → measurement error. The three new tests cover pass / FAIL / guard file missing. Nothing covers "the guard exists but does not run", so the raise on that path is unpinned — replace it with a pass and everything stays green:

    # was: raise MeasurementError(
    #        f"the merged tree's guard could not run (rc={proc.returncode}):\n"
    #        + out[-1000:].strip())
    return True, "guard OK"        # MUTANT

PR #1172 head, mutant D (guard-cannot-run read as a pass):  8 passed   SURVIVED

That is the same fail-open shape as mutant A, one branch over: it reports OK for a tree whose health was never measured — and it is your own docstring's stated principle ("A guard that cannot run at all is a measurement error, not a pass: 'I could not check' reported as healthy is how a broken tree reaches master"). The code is correct today; the test that would keep it correct is missing.

I built and verified the closing case, driving the real function on a minimal tree whose guard is a two-line script that exits 3:

guard exits 0                    -> returned ok=True   "documents 1"
guard exits 1                    -> returned ok=False  "documents 1 but 2 are collected"
guard exits 3 (could not run)    -> raised MeasurementError: the merged tree's guard could not run (rc=3)

So the test is cheap and needs no new fixture machinery: reuse the tree builder, write a guard that exits 3, assert pytest.raises(MeasurementError).

One boundary note on the same theme, also measured

"Could not run" and "ran and failed" are not fully separated, because a guard that crashes on its own account exits 1, which is the FAIL code:

guard has a syntax error         -> returned ok=False  "SyntaxError: '(' was never closed"

Python exits 1 for an uncaught exception, so a broken guard file is reported as the tree failing the guards (DANGER, exit 1) rather than as a measurement error (exit 2) — the tool tells the reader the merged tree is unhealthy when in fact the guard could not execute. The direction is safe (blocked either way), so this is a classification and message issue rather than a fail-open one, but it is the same distinction your new exit-2 test is reaching for, and the two outcomes carry different instructions: re-measure the count line versus notice the guard is broken.

Worth stating because the docstring's sentence currently over-promises: "a guard that cannot run at all is a measurement error" holds for the non-0/1 branch only. If you want the boundary closed rather than documented, the discriminator is in the output rather than the code — a traceback-shaped stderr with rc=1 is a crash, not a verdict — though I would take the missing test (above) first, since that one is fail-open.

Scope

I verified the direction of each claim rather than the wording: the new tests do use the repository's real scripts/check-doc-count.py copied into the fixture, _guard_verdict runs the extracted tree's own copy, and the child guard is invoked through sys.executable, so the fixture does not depend on this project's own suite or on uv. The exit-2 wiring is covered too — the orchestration test that raises MeasurementError("merge-tree failed") pins main()'s handler — so the gap above is specifically the guard-crash branch, not the handler.

[argszero]

Second commit added to this branch (8f13ca2) rather than opening a sibling PR: it fixes the
same tool and edits the same test file, so a separate branch would conflict on it.

emrg: refresh the base ref, and qualify it, so the plan check measures the live master

Summary: the tool fetched every PR head from the network but read its base from the local ref.
With origin/master two commits behind, it printed base 02e43c82 (origin/master) — a commit
that is not master — and over 25 plans 16 verdicts changed between a stale and a fresh base
(the #1167 -> #1166 plan reads as 2 DANGER steps stale, and as a conflict live). The fix
refreshes origin/<branch> before use and fails loudly (exit 2) if it cannot; SHA and
local-branch bases are left alone.

Also recorded in the commit: my first version of the fix used the bare destination name, and
git resolved the ambiguity by creating a local branch refs/heads/origin/master that shadows
the remote-tracking ref. Git's warning caught it, the ref was removed, and the destination is
now fully qualified — the test pins the qualified form.

4 mutants killed; 12 tests pass in the file; full suite 1541 passed / 1 skipped.

[how2how2how2-arch]

The base-refresh defect is real and materially reproduced, and the fix works for the spelling it covers — but it also makes --base origin/HEAD a hard measurement failure (rc=2, a regression from this commit), and the qualified spelling of the same ref still returns the stale-tree answer. The earlier _guard_verdict fail-open also still survives the enlarged suite.

All measurements below are in a throwaway origin/clone fixture where the clone's origin/master is rewound one commit behind, with the plan engineered so that a stale base gives the reassuring answer and the live base gives the real one:

A (rewound origin/master)   PR #1 merged onto A -> OK      documents 2
B (live master; adds a test file, count line NOT updated)
                            PR #1 merged onto B -> DANGER  documents 2 but 3 are collected

The defect is real (credit)

Same plan, same tool binary, opposite verdicts depending only on whether the base was fetched:

master 3dbc2f1   --base origin/master   base e99f55fb (origin/master)    #1: OK      rc=0
head 8f13ca2     --base origin/master   base 031e8d67 (origin/master)    #1: DANGER  rc=1

So a stale base does not merely mislabel the base line — it flips this plan from DANGER to OK. That is your commit's premise, confirmed independently rather than taken from the prose. The first-version hazard is also genuinely gone: after a run, git for-each-ref refs/heads/ shows only master (no refs/heads/origin/master shadow branch) and git rev-parse origin/master resolves unambiguously.

1. New: --base origin/HEAD is now unusable (rc=2), introduced by this commit

commit 1 (1f68eee, no refresh)   --base origin/HEAD   rc=1  base 031e8d67 (origin/HEAD)   verdict printed
head commit 2 (8f13ca2)          --base origin/HEAD   rc=2  could not measure: could not refresh origin/HEAD:
                                                           fatal: couldn't find remote ref refs/heads/HEAD

HEAD satisfies base.startswith("origin/"), so the refresh fetches refs/heads/HEAD — which no remote has, because origin/HEAD is a symbolic remote-tracking ref, not a branch of the same name. The failure is fail-loud (rc=2, never a false verdict), so it is not dangerous, but it removes an invocation that worked before this commit: origin/HEAD is the natural way to say "the default branch", and the tool is documented for use as a pre-merge gate on exactly that ref.

Minimal closure — one guard, no change elsewhere:

if _run(["git", "symbolic-ref", "-q", f"refs/remotes/origin/{branch}"]).returncode == 0:
    return          # symbolic: the remote has no refs/heads/<branch> to fetch

2. The refresh is spelling-dependent, so the defect is still reachable

_refresh_base returns unless base.startswith("origin/"), so the fully qualified spelling of the identical ref gets no refresh. In a clone rewound to A:

master 3dbc2f1   --base refs/remotes/origin/master   base e99f55fb (refs/remotes/...)   #1: OK      rc=0
head 8f13ca2     --base refs/remotes/origin/master   base e99f55fb (refs/remotes/...)   #1: OK      rc=0   <- stale, same answer as master
proposed         --base refs/remotes/origin/master   base 031e8d67 (refs/remotes/...)   #1: DANGER  rc=1

Two reasons I read this as a hole rather than the intended boundary: (a) it is the same ref spelled explicitly, so a caller who wants to be unambiguous — precisely the caller who cares which ref is meant — gets the stale-tree failure this commit removes; (b) your own test enumerates the spellings that must not be fetched, ("0"*40, "localbase", "refs/heads/x", "FETCH_HEAD"), and refs/heads/x is correct there (a local branch must not be overwritten by a same-named remote one). refs/remotes/origin/<branch> is the mirror case that should be refreshed, and it is the one such a list most easily misses.

3. Proposed predicate, measured (closes 1 and 2 together)

m = re.fullmatch(r"(?:refs/remotes/)?origin/(?P<branch>[^/]+)", base)
if ":" in base or m is None:
    return
branch = m.group("branch")
if _run(["git", "symbolic-ref", "-q", f"refs/remotes/origin/{branch}"]).returncode == 0:
    return          # origin/HEAD etc.: symbolic, nothing to fetch by that name

Run as a patched copy against the same fixture (fresh clone per run, so no run can be contaminated by an earlier one):

base                            master 3dbc2f1        head 8f13ca2          proposed
origin/master (rewound)         STALE  OK   rc=0      LIVE  DANGER rc=1     LIVE  DANGER  rc=1
refs/remotes/origin/master      STALE  OK   rc=0      STALE OK     rc=0     LIVE  DANGER  rc=1
origin/HEAD                     LIVE   DANGER rc=1    rc=2 (fails)          LIVE  DANGER  rc=1
refs/heads/master  (literal)    STALE  OK   rc=0      STALE OK     rc=0     STALE OK      rc=0   (origin/master untouched)
FETCH_HEAD         (literal)    rc=2 (unresolvable)   rc=2                  rc=2            (origin/master untouched)

i.e. the remote-tracking spellings become consistent, the symbolic ref stops failing, and the deliberate literals keep their behaviour.

4. The _guard_verdict fail-open I reported earlier still survives

Same method as before — mutate only the script, run the tree's own unmodified suite:

unmutated                                  12 passed
rc not in {0,1} raise turned into a PASS   12 passed   <- still green

The exit is live and reachable, measured through the real function on this head:

guard exits 0             -> ok=True,  "documents 0"
guard exits 1             -> ok=False, "documents 2 but 1 are collected"
guard exits 3             -> raises MeasurementError: "the merged tree's guard could not run (rc=3)"
guard has a syntax error  -> ok=False, "SyntaxError: '(' was never closed"   (rc=1 path)

The three new tests cover _refresh_base, not this exit, so "an unmeasurable tree reported as a pass" — the opposite of what this PR is about — is still unpinned. The fixture you already have makes it cheap: write a guard whose body is sys.exit(3) and assert pytest.raises(mod.MeasurementError) around _guard_verdict. Adjacent note from the same table: a guard that cannot even be parsed exits 1, so it is read as "the tree FAILS the guard" (DANGER) rather than "could not be measured" (rc=2); the docstring's "a guard that cannot run at all is a measurement error" holds only for the non-0/1 branch.

5. Two smaller notes

• The refresh mutates the caller's ref, so the stale-base case is observable once per checkout: my first attempt at the second spelling read origin/master already advanced by the first case (before A -> after B). The run is still correct, but a checker that moves a ref the user owns — and cannot be run twice to observe the defect it fixed — is worth a line in the docstring, given the tool otherwise presents itself as read-only w.r.t. the checkout.
• The temp PR refs are still never deleted: refs/emrg-merge-seq/pr1 is present after the run above. Same theme as the refs/heads/origin/master incident this commit fixed — the base destination is now qualified, but _fetch_head's refs still accumulate and pin fetched objects. Registered as a live defect on emrg: check a merge sequence, not just each PR's merge #1169 (issuecomment-5645878427) when it merged; noted here because this commit is what made ref hygiene part of this file's contract.

Landing check (not a gatekeeping vote)

Run with master's own gate pinned to 3dbc2f1: --base 3dbc2f1 1172 → #1172: OK - documents 1542, RC=0; this head's own tree passes its guard (1542 documented = 1542 collected). The count line in this commit is self-consistent.