test: sort sample_path() calls into real source order in the tier guard

[JarryShaw]

The defect

tests/_tiers.py's sample_path_calls() documents that it returns every sample_path(...) call "in source order", but collects them with ast.walk, which is breadth-first. A call written near the top of a file but nested inside a method is therefore yielded after one written at the bottom at module level.

Measured independently of the fix:

ast.walk order                 : 14, 11, 7, 7   dhcp_big_endian.pcapng, http.cap, http6.cap, test.pcap
sorted by lineno alone         :  7,  7, 11, 14  http6.cap, test.pcap, http.cap, dhcp_big_endian.pcapng
sorted by (lineno, col_offset) :  7,  7, 11, 14  test.pcap, http6.cap, http.cap, dhcp_big_endian.pcapng

audit_module() emits one finding per call in this order, and the guard's whole value is a diagnostic a reader can act on. Breadth-first order is not the order anything appears in the file, and it shifts when unrelated code moves between nesting levels — so the same set of violations lists differently from one edit to the next, and a reader comparing two runs cannot distinguish a reordering from a new finding.

The fix

Sort by (lineno, col_offset). col_offset breaks the tie for two calls on one line — though not for a tuple: ast.Tuple keeps its elements in one field list, so (sample_path('a.pcap'), sample_path('b.pcap')) already walks left to right and needs no tie-break. That was the original review's finding, and it is why the regression fixture now uses a conditional expression instead: ast.IfExp stores test, body, orelse while source writes body if test else orelse, so sample_path('a.pcap') if sample_path('b.pcap') else None is walked b, a. ast.Dict is the other shape that mis-orders a single line, since all of its keys are walked before any values.

Sorted rather than replaced with a hand-written lexical-order visitor deliberately: the sort is the whole fix in one expression and needs nothing kept in step with it, whereas a second traversal is another thing that can disagree with ast.walk about where calls live.

Also in this change

• A regression test that builds a module whose walk order and source order genuinely differ, so the sort cannot silently regress.
• The last hand-built capture path in tests/protocols/misc/test_pcapng_unit.py now goes through sample_path(), inside the try/except FileNotFoundError idiom explain() recommends. The _tiers module docstring cited that line as the outstanding example of a capture read the guard cannot see, so the docstring is rewritten to match — and it now records the measurement behind not generalising the rule: a check as broad as "any string literal ending in .pcap" matched 16 distinct literal values in the unit-tier modules — 18 matched in total across 85 occurrences, and 199 occurrences across all of tests/ — of which only two are sample_path arguments at all and both are already legal, so the broad rule would catch nothing this one misses, and a false positive here aborts collection for everybody.

Verification

Test-only change; no library code touched.

suite	result
tests/test_tier_guard.py	25 passed, 19 subtests passed
tests/protocols/misc/test_pcapng_unit.py	37 passed, 139 subtests passed

Both exit 0, run against a tree with all 20 fixture captures present — the 18 examples/generators/make_samples.py writes, plus the committed dhcp.pcapng and in.pcap.

[JarryShaw]

Review verdict: CHANGES NEEDED

The fix is correct and its regression test genuinely fails without it — reverting only the sort gives AssertionError: Lists differ: [(14, …), (11, …), (7, …), (7, …)] != [(7, …), (7, …), (11, …), (14, …)].

One finding blocks, and it is the same defect class this PR exists to fix — a docstring asserting a property that measurement disproves:

• tests/_tiers.py:458 — the col_offset half of the sort key is pinned by nothing. Replacing key=lambda node: (node.lineno, node.col_offset) with key=lambda node: node.lineno leaves the suite green at 25 passed / 19 subtests, while tests/test_tier_guard.py:305-307 claims "The two calls sharing line 7 pin the col_offset tie-break". Cause: ast.walk already yields a Tuple's elts left-to-right, so the cited example never needed a tie-break. Measured walk col_offsets — tuple 1, 9 (already sorted); dict literal 1, 19, 10, 28 against sorted 1, 10, 19, 28; conditional expression 17, 4, 32 against 4, 17, 32. The shapes that need it are ast.Dict (keys and values are separate field lists) and ast.IfExp (test, body, orelse against source order). Not academic: a one-line dict violation through the real conftest abort path listed arp.pcap http.pcap tcp.pcap ipv4.pcap without col_offset against the correct arp.pcap tcp.pcap http.pcap ipv4.pcap with it.

Non-blocking:

• tests/_tiers.py:446-447 — the docstring's tie-break example does not exhibit the tie (same cause).
• tests/_tiers.py:59 — "No module in the suite does this today" is true only for the unit tier; tests/protocols/test_option_coverage_runtime.py:39,80 joins SAMPLE_ROOT and a name itself in fixture tier.
• tests/_tiers.py:66-68 — "about fifteen false positives" reproduces only as distinct literal values in unit-tier modules (18); it is 85 occurrences in unit tier and 199 across tests/. Worth naming the unit.
• tests/protocols/misc/test_pcapng_unit.py:3028-3029 — credits GeneratedFixtureInUnitTierError for something the handled-line opt-out does; check_unit_tier_read returns None at tests/_tiers.py:606-607 before the subclass could be constructed.
• The PR body's [6,5,4] probe does not match the committed 14, 11, 7, 7; and it says 19 fixture captures where a freshly generated tree has 20.

Verified correct and worth recording: the sort matches independent ground truth (character offsets in raw source) across all four probed shapes; the ordering is genuinely load-bearing end-to-end via audit_module (tests/_tiers.py:561-565) → tests/conftest.py:66 → :104; the absent-fixture path is intact (fixture moved → 1 skipped naming the file and the regenerate command; whole file → 36 passed, 1 skipped, collection not aborted); and unit-tier selection as CI runs it is 938 passed, 5 skipped, 1988 subtests, exit 0.

And a silent hole this closes that the PR does not claim: run from /tmp, this version passes while base 27bb315d5 reports SKIPPED … run examples/generators/make_samples.py first. Before this change the test quietly skipped for anyone not invoking pytest from the repo root.

Not verified: Python 3.10/3.11 (measured on 3.14.7 only), a git-less checkout, and the fixture-dependent tier.

[JarryShaw]

Review feedback addressed — pushed e6416a3a7

All findings fixed. The central one was correct and I reproduced it before fixing it: col_offset was pinned by nothing.

The synthetic module's line-7 pair was a tuple, and ast.Tuple._fields == ('elts', 'ctx') keeps its elements in one field list — so those calls already come out of the walk left to right, and the suite stayed green with col_offset dropped. Replaced with a conditional expression, since ast.IfExp._fields == ('test', 'body', 'orelse') while source writes body if test else orelse.

Both halves of the key now fail independently when reverted, measured on the pushed tree:

sort key	result
(node.lineno, node.col_offset) — as shipped	25 passed, 19 subtests, exit 0
node.lineno only	1 failed — test_calls_and_findings_come_out_in_source_order
node.col_offset only	1 failed

The lineno-only failure reports (7, 'http6.cap') ahead of (7, 'test.pcap'): a stable sort leaves a tie in walk order, so only col_offset restores it. The findings assertion now matches capture names as well as lines, because the two line-7 calls are indistinguishable by lineno — that is what pins the diagnostic order a reader of a failed collection actually sees.

Three corrections to figures in the review and in this PR's description

• False positives now carry a unit: a rule as broad as "any string literal ending in .pcap" matches 16 distinct literal values in the unit-tier modules (18 matched in total, 85 occurrences, 199 across all of tests/). Of the 18, only two are sample_path arguments — in.pcap, which is committed, and test.pcap, read inside the sanctioned try/except — and both are already legal. So the broad rule would catch nothing the narrow one misses and invent 16. The earlier "about fifteen" was this figure read loosely.
• The file count in the review is wrong: find tests -name '*.py' | wc -l gives 133, not 176. Identical in the worktree and the reference checkout. It affects no docstring, since no count of modules is quoted, but it should not be repeated.
• Fixture count is 20, not 19 — the 18 examples/generators/make_samples.py writes, plus committed dhcp.pcapng and in.pcap.

ast.Dict is named in both docstrings as the sibling shape that also mis-walks a single line, since all of its keys are visited before any of its values. ast.IfExp was used in the test because it reverses with only two calls and fits the file's line-length norm; a reversing Dict needs four calls on one line.

Suite unchanged either side of the change: 938 passed, 5 skipped, 1988 subtests, exit 0, run as CI does. Zero TierGuardWarning in both logs, so the guard genuinely ran.

[JarryShaw]

Re-review of e6416a3a7: GOOD TO GO

Second cross-review, on a different model, scoped to the delta b72fdd5ca..e6416a3a7 rather than the whole PR.

Both ablations reproduced exactly

sort key	result
(node.lineno, node.col_offset) as shipped	25 passed, 19 subtests, exit 0
node.lineno only	1 failed — tests/test_tier_guard.py:344, first differing element (7, 'http6.cap') against expected (7, 'test.pcap')
node.col_offset only	1 failed, 24 passed — same test, 14 sorting first

It also reproduced the original finding directly: parsing the pre-fix fixture's line 7 with ast.walk unsorted already yields test.pcap (col 11) before http6.cap (col 37) — so ast.Tuple never needed the tie-break, which is exactly why the old tuple-shaped fixture could not have caught a dropped col_offset.

It went well past the claim: 36 AST shapes probed, no counterexample

Asked to hunt for a shape the fixed key still mis-orders, it confirmed ast.Dict behaves as the docstrings say (lineno-only gives a, c, b, d; the shipped key gives a, b, c, d) and then probed 36 further single-line shapes where field-visit order could disagree with text order: Set, nested IfExp, 2- and 3-way BoolOp, Compare, BinOp, call args / kwargs / *args / **kwargs, Subscript, 3-part Slice, list/dict/generator comprehensions, f-strings including format specs and a multi-line expression, walrus, lambda body and lambda default, starred tuple, UnaryOp, assert, with, chained and multi-target assignment, AnnAssign, backslash continuation, triple-quoted multi-line string argument, semicolon-joined statements, decorator plus default on adjacent lines, and match/case guards.

All 36 came out in true source order. And the reason it generalises is worth stating: col_offset is a direct positional marker, independent of the order ast.walk happens to visit a node's fields, so sorting on position rather than on traversal is robust across shapes.

Secondary claims confirmed independently

The false-positive census re-derived with its own AST scan using the real _tiers.is_unit_tier: 18 distinct .pcap-suffixed literals in unit-tier modules across 85 occurrences, 199 occurrences across all of tests/, and exactly in.pcap and test.pcap appearing as sample_path arguments — both already legal. find tests -name '*.py' | wc -l = 133. Fixture count = 20.

One nuance it found, and it corrects something I wrote

I said the names-in-findings assertion "pins the diagnostic order a reader of a failed collection sees." That overstates its role here. audit_module (tests/_tiers.py:589-593) iterates sample_path_calls(...) with no re-sort, so it is order-identical to the calls list asserted two lines earlier — and in both ablations the AssertionError fired at that earlier calls-level assertEqual (line 344), before audit_module was called at all. So the findings-level regex is genuine additional coverage, of a different bug class (name/line desync during message construction), but it is not the mechanism that closes the originally-flagged gap. That gap is closed by the calls-level expectation now carrying names, together with the fixture using IfExp rather than Tuple. Non-blocking, but the narrative should not claim otherwise.

Also verified

The corrected comment in tests/protocols/misc/test_pcapng_unit.py is accurate: check_unit_tier_read returns None for a call on a handled_lines-recognised line before sample_path can raise GeneratedFixtureInUnitTierError, so the plain FileNotFoundError from the existence check is what fires when a capture is genuinely absent — matching that exception's own docstring. And the SAMPLE_ROOT line references at tests/protocols/test_option_coverage_runtime.py:39,80 are exact.

Description corrected

It flagged that this PR's description still carried the stale claims even though the in-code docstrings were fixed — the [6, 5, 4]/[4, 5, 6] probe, "about fifteen false positives", "all 19 fixture captures", and the tuple example given as needing the tie-break, which it measured as precisely the shape that does not. All four are now corrected above.

Not verified

Python 3.10/3.11 (measured on 3.14.7 only), a git-less checkout, and the full-suite figure — the last deliberately, under host-care constraints after an earlier OOM incident on this machine.

[JarryShaw]

✅ GOOD TO MERGE — cross-reviewed twice, both halves of the sort key now provably pinned, 36 further AST shapes probed with no counterexample found.