Restore @pytest.mark.unit on TestRootUserGate; guard this file against it recurring

[jgruberf5]

Description

TestRootUserGate came out of the #1/#2 add/add merge without its @pytest.mark.unit: both PRs appended a class after the same trailing marker, git hoisted the marker into common context and left it on the staging side's class (TestKillTaskContainers), and the class appended after it was bare. 29 root-gate tests — including the round-2 blocker fixtures for 4294967296 / 8589934592 / 2147483648 / 999999999999 — were invisible to -m unit while still passing by path, so CI could not see the loss.

Reproduced the issue's own negative control exactly:

-m unit -k TestRootUserGate    before: 29 deselected    after: 29 passed

Fixes #128

The guard — and why it's narrower than the issue suggested

The issue floats a suite-wide check that every class Test* under tests/unit/ carries the marker, "the suite convention documented in AGENTS.md". I looked before building it, and that convention doesn't exist in this repo: there is no AGENTS.md, 781 of ~890 test classes under tests/unit/ carry no marker, and no CI target or Makefile rule selects by -m unit (unit tests run by path, as the issue itself notes). A suite-wide guard would fail 781 classes asserting a rule nobody follows, for a selector nobody uses.

What is true is that this file follows the convention — every other class in test_container_runner.py is marked, and TestRootUserGate was the one odd one out, which is exactly the merge-hoist signature. So the guard is file-local: TestEveryClassInThisFileIsMarkedUnit asserts every Test* class in this file is marked, and names the class and line when one isn't. Verified it bites — stripping the marker again produces:

AssertionError: ... TestRootUserGate (line 922)

That makes the next add/add merge on this file self-detecting, which is what the issue actually wants, without inventing a repo-wide rule.

---

Architectural Decision Record (ADR)

• N/A (Bug fix or minor docs tweak).

---

Type of Change

• Bug fix (non-breaking change fixing an issue)

---

Verification & Testing

• test_container_runner.py: 100 passed (99 + the guard).
• Negative control as above: 29 deselected → 29 passed under -m unit.
• Guard verified non-vacuous: fails with the class name and line when the marker is removed.
• ruff clean.

Environment validation needed

None — test-only change.

---

Checklist

• My code follows the project's code style and formatting guidelines.
• I have updated documentation where necessary — the guard's docstring records both the failure mode and why the scope is deliberately file-local.
• N/A — no backend route/schema changes.

Worth a separate decision

If you want -m unit to be a meaningful selector, that's a real (and larger) piece of work: 781 classes to mark, plus a CI change to select by marker. I've stated the numbers so that call can be made with eyes open; it's out of scope here.

[jgruberf5]

Self-review

No defects found. Verified the parts that could plausibly be wrong.

The guard's parser — edge cases checked

The backward walk from class Test* over decorator/blank lines could mis-handle stacked decorators or spacing. Exercised it directly:

@pytest.mark.unit
@pytest.mark.parametrize(...)   ← marker not adjacent to class
class TestA                     → marked ✓

@pytest.mark.skip
class TestB                     → bare ✓   (a non-unit decorator alone doesn't count)

@pytest.mark.unit
                                ← blank line between marker and class
class TestC                     → marked ✓

Result: ['TestB'] — exactly right. ^class anchoring means only top-level classes are checked, which is the intent (nested helper classes aren't test classes pytest collects at module level).

The guard runs under both selection modes

It is itself @pytest.mark.unit, so it runs in CI's path-based pytest tests/unit/ and under -m unit — 1 passed, 99 deselected with -m unit -k TestEveryClassInThisFileIsMarkedUnit. A guard that only ran under a selector nobody uses would be pointless.

Verified non-vacuous (restating from the body, since it's the whole point)

Stripping the marker again → AssertionError: ... TestRootUserGate (line 922). It names the class and line, so the next merge that hoists a marker gets a precise pointer rather than a silent 29-test loss.

The scope decision, re-examined

I pushed back on the issue's suggested suite-wide guard because 781/~890 unit classes are unmarked and no CI target selects by -m unit. Re-checked that those numbers are right and that there is no AGENTS.md stating the convention — both hold. The issue author's instinct (make the loss self-detecting) is honoured at the scope where a convention actually exists: this file. The body says so, and flags the suite-wide question as a separate decision with the numbers attached.

Issue association

Body updated to a bare Fixes #128 so the auto-close Action's parser matches it (the template's Fixes / Implements: form does not — fix for that coming as its own PR).

100/100 in file, ruff clean, test-only change.

[mwiget]

Approving — the fix is right and, unusually, I could check every number in the description.

Verified independently against origin/staging:

• 891 class Test* under backend/tests/unit/, 781 of them without @pytest.mark.unit (walked the same decorator/blank-line rule your guard uses) — your "781 of ~890" is exact.
• No -m unit selector anywhere in Makefile, .github/, bin/ or scripts/ — confirmed, so the marker really is unused by CI today.
• Reproduced the failure mode: the marker on TestKillTaskContainers with TestRootUserGate bare is exactly the add/add hoist signature.

One correction to the description (not to the code): AGENTS.md does exist — it's tracked at the repo root on staging. It carries no marker convention, but docs/DEVELOPMENT.md:211 does: "Markers: @pytest.mark.unit, @pytest.mark.component, @pytest.mark.full". So the convention is documented, it's just unenforced and unread by CI — which leaves your scoping decision intact (781 bare classes and no -m unit consumer is still the argument against a suite-wide guard), but the reasoning should rest on "documented but not enforced", not on "doesn't exist".

The "worth a separate decision" note is the right call — if -m unit is ever wired into CI, that's a marker sweep plus a CI change, not this PR.

[mwiget]

Minor, and only a false-alarm risk rather than a miss: the walk-back accepts decorators and blank lines, so a comment between the marker and the class makes an actually-marked class report as bare —

@pytest.mark.unit
# why this class exists
class TestSomething:

That's a failing test on correct code, which is the annoying direction for a guard nobody expects to fire. or lines[j].lstrip().startswith("#") in the loop condition covers it. Not worth another CI round on its own.