fix(ci-guard): locate the engine pytest step by name, not by a one-line spelling

[wshallwshall]

One file, one change: tests/test_ci_engine_step_excludes_webconsole.py locates the engine
pytest step by its name: instead of by a one-line run: spelling.

This is one slice extracted from PR 613. The other four payloads on that branch are not here
and stay there. Nothing on lander/433-partb-engine was touched.

What the guard asserted

The file exists to stop CI running the web console suite twice per leg. pyproject.toml's
testpaths includes packaging/messagefoundry-webconsole/tests, and that package also runs as its
own job, so the engine step has to subtract it. The three assertions pin that subtraction and the
exact flag spelling that works.

To find the engine step, the locator scanned ci.yml line by line:

    for line in text.splitlines():
        stripped = line.strip()
        if stripped.startswith("run: pytest -q"):
            return stripped
    pytest.fail(f"no engine `run: pytest -q` line found in {_CI}")

Why that was vacuous

MEASURED on 85516d100. Exactly one line in ci.yml starts with run: pytest -q, and it is not
the engine step. It is the tooling job's Harness tests (pytest) step at L1134:

run: pytest -q -n 4 --dist loadfile -m tooling --ignore-glob='*messagefoundry-webconsole*' --timeout=120 --junitxml=tooling-junit.xml

The engine Tests (pytest) step in the test job is wrapped, so its run: line never starts with
that prefix and is never examined:

run: bash scripts/ci/retry-native-crash.sh pytest -q -n "$PYTEST_WORKERS" --dist loadfile -m 'not tooling' --ignore-glob='*messagefoundry-webconsole*' ...

The tooling line happens to carry --ignore-glob and messagefoundry-webconsole, so all three
assertions pass against an invocation the file was not written for. A pytest.fail never fires,
because the scan does find a match. The guard is green and uninformative about its own subject.

Mutation control, MEASURED. Rewriting ONLY the engine step's --ignore-glob='*messagefoundry-webconsole*'
to --ignore='packaging/messagefoundry-webconsole/tests' and leaving the tooling line untouched:

Guard	Unmutated ci.yml	Engine step broken
Old locator	GREEN	GREEN
New locator	GREEN	RED

The old guard cannot go red on the regression its own docstring says it exists to catch.

What it asserts now

The three assertions are byte-identical. Only the locator changed. It parses ci.yml with
yaml.safe_load, selects the step whose name: is Tests (pytest), and slices the run block from
the pytest token so a wrapper does not break the lookup. Matching mid-line is safe only because
the step is already located by name; a mid-line search over the whole file would hit the earlier
doc-guards step.

MEASURED, the new locator returns the engine step's own line, identifiable by -m 'not tooling':

pytest -q -n "$PYTEST_WORKERS" --dist loadfile -m 'not tooling' --ignore-glob='*messagefoundry-webconsole*' ...

How I verified it applies to main

1. git merge-base origin/main refs/prs/613 is 5f901411f. git log 5f901411f..origin/main -- tests/test_ci_engine_step_excludes_webconsole.py returns nothing, so main has not touched this file since PR 613 forked. The change applies to 85516d100 unchanged, with no conflict.
2. The committed file is byte-identical to PR 613's version: git diff --exit-code refs/prs/613 HEAD -- <path> exits 0.
3. pytest tests/test_ci_engine_step_excludes_webconsole.py -q gives 3 passed against this branch's ci.yml. The module's _ROOT resolves to this checkout, confirmed by printing it.
4. ruff format --check and ruff check pass on the file.
5. import yaml adds no dependency. pyyaml==6.0.3 is already in requirements.lock and constraints.lock, and seven test modules on main already import it.
6. Self-containment: the file imports only re, tomllib, pathlib, pytest and yaml. It uses no fixture and no helper from PR 613. It is already listed in tests/tooling_manifest.txt on main at line 43, so no manifest edit is needed. PR 613's one tooling_manifest.txt addition is for a different file (tests/test_gate_install_receipt.py) and is not part of this slice.

INFERRED, not measured here: whether the required CI legs pass on a hosted runner. That is what this
PR's own checks will answer.

Severity

MessageFoundry is a not-deployed beta, so this is written in the conditional. Nothing is
mis-running: the engine step still carries the subtraction today. What is missing is the protection.
If someone simplified that flag back to --ignore=, the web console tests would silently run twice
on every engine leg and this required check would stay green.

The filed row for this defect is BACKLOG #1389. It is cited in the commit body rather than the
subject, because this commit is an extraction from an existing PR rather than a declaration that it
implements that row.

🤖 Generated with Claude Code

[wshallwshall]

Reviewed and labelled by the Lander. I verified the load-bearing premise from ci.yml on main rather than taking it, because the whole claim is that a green guard was asserting about the wrong job.

The defect is real, and here is the proof:

ci.yml:824   run: bash scripts/ci/retry-native-crash.sh pytest -q ... -m 'not tooling' ...   <- ENGINE
ci.yml:1134  run: pytest -q -n 4 ... -m tooling ...                                          <- TOOLING
steps literally named "Tests (pytest)": 1
CONTROL: 83 pytest lines in ci.yml, so the search ran

A locator matching a one-line run: pytest -q spelling cannot match line 824 -- the engine invocation now begins with bash scripts/ci/retry-native-crash.sh. The only line it does match is 1134, the tooling job. So the old guard was green while asserting against a step that is not its subject. That is the worst shape a guard can take: it reports coverage that does not exist, and nothing about a green run distinguishes it from a working one.

The fix locates by step NAME, and exactly one step carries that name, so it is unambiguous. Two details make it better than the minimum:

1. It fails loudly with no step named 'Tests (pytest)' found if the step is renamed or removed, rather than silently matching nothing. A locator that can return empty and pass is the same defect one layer up.
2. Its comment states that mid-line matching is safe only because the step is already located by name. That is the reasoning a future editor needs in order not to reintroduce the bug by "simplifying" the locator back to a spelling match.

On the mutation control: the PR reports that breaking the engine step's flag leaves the old guard green and turns the new one red. I did not re-run that mutation myself -- I verified the structural premise above instead, which is what makes the mutation result predictable. Recording the distinction rather than implying I reproduced it.

Scope is right. One file, +40/-7, no workflow change, off current main. This is the extracted CI-guard half of PR 613; extracting it means the fix can land now while 613's remaining payloads go back to their author. Landing this does not make 613 worse -- measured by its author, the guard file merges to identical content and 613 conflicts on the same two files either way.

Only failing required context was the review gate, which this label clears.

[wshallwshall]

LABEL REMOVED AND DEQUEUED. I applied it, it was in the merge queue, and it should not land as written.

The central claim is true and I am not disputing it. The old guard is 100 percent vacuous -- mutating ci.yml:824's ignore flag leaves it green on a real regression, and this PR's version goes red. That defect is real and this does fix it.

But it is a partial fix that leaves the same failure shape reachable, and it has false positives in both directions. I verified the structure myself rather than taking the report:

line 60  for step in job.get("steps") or []:
line 61      if step.get("name") == _ENGINE_STEP:
line 63          for line in run.splitlines():        <- per LINE, not the whole run: block
line 76              idx = stripped.find("pytest ")   <- first line carrying the token
line 78                  return stripped[idx:]        <- RETURNS on the first match

Two structural consequences, each demonstrated by a run mutation:

Mutation	Expected	Actual
--ignore-glob kept on line 1, plain --ignore= added on a continuation line	RED	GREEN -- the regression sails through
decoy job with a correct step placed before test, real engine step broken	RED	GREEN -- only the first matching step is ever checked
invocation reformatted onto backslash-continuation lines, flag preserved	GREEN	RED -- false positive on a harmless reformat
echo "starting pytest for the engine leg" added above an untouched invocation	GREEN	RED -- find("pytest ") takes the first line with the token

The false positives are the operationally worse half: the first person to reformat that long command line gets a red required check for nothing. A guard that cries wolf on a reformat gets disabled by the third person it inconveniences.

It also deviates from its own filed acceptance criterion. BACKLOG #1389 (verified filed on main with parse_items, control #1401) states the fix as binding the step by name or id and reading the whole run: block "rather than pattern-matching a one-line spelling that a formatting change can move". This PR does the first half only -- and the second half is exactly what mutations 1 and 3 exploit.

Two comment defects worth fixing in the same push, because comments outlive PR bodies: the in-file comment says the wrapping "broke the LOOKUP" and that the failure "then read 'no engine step found'". No such failure occurred -- the guard was GREEN on main, which is the whole point, and #1389 warns against exactly that misreading. The PR body gets this right; the code comment does not. And the file heads its comment with (BACKLOG #1260), the native-crash-wrapper row, never naming #1389, its own row. Both numbers resolve, so nothing dangles -- it just points a reader at the wrong item.

The repair is small and stays inside one function: join the located step's run block before slicing, collect all steps named Tests (pytest) and assert over every one rather than returning the first, and anchor on a token that cannot be a prose mention.

Nothing else was wrong with it: self-contained, no new dependency, already covered by the tooling manifest, 3 tests pass, ruff clean, no glyphs, both cited numbers resolve.

On my own process, since this is the second label I have pulled today. Both times the label went on before an adversarial review ran, and both times the review found what I had not looked for. My reviews check whether the claims a change makes are TRUE. They do not systematically probe for what the change FAILS to do -- and a partial fix passes every truth check while leaving the hole open. Mutating the guard in the safe direction, not just the broken one, is what catches this, and it is what I did not do.

Re-label when the four mutations produce the right verdicts.

[wshallwshall]

Re-labelled. All four holes are closed and I verified the structure on 0f8f9097a rather than taking the report -- and this time I probed for what it might still FAIL to do, which is the pass I skipped when I labelled the first version.

The four structural fixes, each read from the file:

line  45  return [... for line in re.sub(r"\\n[ \t]*", " ", run).splitlines() ...]
          -> continuations joined, so a flag on line 2 is no longer invisible
line  57  argv = shlex.split(line, comments=True)
          -> `echo "starting pytest ..."` is one quoted argument to echo, never a command word
line 110  for job_id, job in workflow["jobs"].items():
          -> every job, not the first
line 127  return found
          -> a collection; no early return survives anywhere in the step loop

Both tests then iterate for job_id, run in _engine_step_invocations() and assert on each invocation, not any(), and each failure message names the offending job.

THE THING I WENT LOOKING FOR, because it is this repository's most persistent defect: a guard that passes vacuously. Two for loops over a list will both pass trivially if the list comes back empty, and nothing about a green run distinguishes that from a working guard. It is guarded at both levels:

if not invocations:  pytest.fail(f"the {_ENGINE_STEP!r} step in job {job_id!r} runs no `pytest` command")
if not found:        pytest.fail(f"no step named {_ENGINE_STEP!r} found in {_CI}")

A step rename fails loudly rather than quietly finding nothing. That is the difference between this and the version I sent back.

The mutation table now reads correctly in both directions, which is the part that matters -- the first version was wrong in both:

mutation	before	after
reformat onto continuation lines, flag preserved	RED (false positive)	GREEN
echo mentioning pytest above a correct call	RED (false positive)	GREEN
glob on line 1, plain --ignore= on a continuation	GREEN (hole)	RED
second job lacking the subtraction	GREEN (hole)	RED
correct decoy job before test, real one broken	GREEN (hole)	RED
step renamed	RED	RED, loud

Plus three over-breadth arms it added itself -- a # comment naming --ignore=, a trailing comment on the invocation line, and a second genuinely unsubtracted call -- all correct.

Binding on name: alone rather than adding id: tests is the right call, and the reasoning holds: an id is job-scoped, so matching it would sweep in a second job's id: tests silently and let a rename pass quietly.

The comment defects are fixed too. It no longer describes a failure that never happened, it names the tooling job as what the old locator actually bound, and it cites #1389 beside #1260.

On my own process, since I sent this back once: I labelled the first version after confirming its central claim was true -- the old guard really was vacuous and this really did catch the regression. That was correct and insufficient. What I did not do was mutate in the safe direction to see whether it cried wolf, or ask what happens when the locator matches nothing. Both holes were reachable in one function. Truth-checking a claim and stress-testing it are different passes.