From 5e8826d2a7f52ab114f47b238158ea2fae206813 Mon Sep 17 00:00:00 2001 From: Blaise deB Frederick Date: Sun, 27 Sep 2026 16:16:22 -0400 Subject: [PATCH] Guard every generic sweep over MAPPINGS against a keyless mapping Making `Mapping.ascconv_key` optional for the preview-only `1st Signal/Mode` entry needed three guards in mappings.py, and those are the three the feature itself traverses. Four other sites read the field generically and only one of them failed. `test_every_enum_choice_agrees_with_the_corpus` selected its enums as `m.choices and m.bit is None`, so it asked the ASCCONV block for a key that by construction does not exist and reported the mapping as disagreeing with the corpus on 418 readings over 261 scans. It now excludes keyless mappings and names the preview-side sweep that covers them. The two duplicate-key checks are the expensive ones, because they passed: with one preview-only mapping their identity tuples stay unique, and with a second they would pair the two on their shared empty key and report a collision between mappings that write nothing at all. Entry identity is now the `preview_path` where there is no key, and the write-race check leaves out a mapping that never writes. `probe._is_mapped("")` had become True, safe only because no real ASCCONV key splits to an empty stem; it now answers for real keys only. Verified: full suite 3743 passed, 26 skipped with -n auto; the enum sweep still fails on a mutated choice, so the exclusion did not gut it; the duplicate-key fix clears two distinct preview-only mappings and still catches two sharing a preview_path. CLAUDE.md records the finding, and separately that piping a suite run through `tail` discards pytest's exit code -- which is why the failing run was first reported as green. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 75 ++++++++++++++++++- .../analysis/generate/probe.py | 13 +++- tests/test_exar_patch.py | 26 ++++++- 3 files changed, 108 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 01bdfdb..3e05804 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -122,6 +122,23 @@ See `Design.md` for the design and `README.md` for usage. parametrized cases is what would. `--durations=30` names the current worst offenders. +- **Redirect a suite run to a file; never pipe it.** A shell pipeline reports + the *last* command's status, so `pytest -n auto | tail -60` exits 0 on a + failing suite and the summary line saying `1 failed` scrolls past inside + output the caller reads as a success. `> suite.log 2>&1` keeps pytest's own + exit code. `tail` also buffers, so nothing reaches the log until the process + ends, which defeats polling the file for progress -- and a wait loop written + as `until [ -s log ]` is then satisfied by whatever writes first, which on + one occasion was `black` while pytest still had eleven minutes to run. Poll + for the summary line itself (`grep -qE "[0-9]+ (passed|failed)"`), not for + the file being non-empty. + + Checking that `-n auto` really took effect has the same shape. There are no + `popen-gw` process names to grep for on macOS -- an xdist worker is a bare + interpreter path with no distinguishing argv -- so a name-based check reports + zero workers for a run that has twelve. Ask the process tree instead: + `ps ax -o pid,ppid,command | awk '$2=='. + ### The GUI - It is a page served to the browser by a stdlib `http.server`, not a toolkit. @@ -2453,7 +2470,9 @@ drives. Six rounds have run; they took `MAPPINGS` from 115 to 249 and settled four questions this file had recorded as open. The last 13 of those came from no scanner trip at all -- `Finding.attributable` re-read returns already in hand, which is the cheapest round there is and the one to try -before building another archive. Rounds 4 to 6 are +before building another archive. The table stands at 250: `1st Signal/Mode` +is the one entry no probe round produced, so do not reconcile the 249 to a +count of `MAPPINGS` -- it is a figure about the rounds, not about the table. Rounds 4 to 6 are where the method stopped adding mappings one at a time: 553 probes over twenty archives, and -- more usefully for planning the next round -- 163 elements shown to print nothing and 95 the sequence refuses to have @@ -2724,6 +2743,60 @@ that a `ConversionNeeded` protocol is stale rather than orphaned. that prints an asterisk after every scan name, so the first join missed all 14 of its readings and the correspondence looked unconfirmable; it needs `build.match_name` on both sides, exactly as that entry says. +- **Making a `Mapping` field optional turns every sweep over the table into a + reader of a value it was never written for, and the sweeps are in the test + file rather than beside the table.** Giving `ascconv_key` a default of `""` + needed three guards in `mappings.py` -- `display` reads the preview + instead, `resolve` refuses with a reason, `is_churn` is not asked -- and + those are the three a person writing the feature looks at, because they are + the ones the feature needs. Four *other* sites read the field generically, + and only one of them failed. + + `test_every_enum_choice_agrees_with_the_corpus` selected its enums as + `m.choices and m.bit is None`, which is every enum including the + preview-only one, and then asked the ASCCONV block for a key that by + construction does not exist -- so it reported the mapping as disagreeing + with the corpus on **418 readings over 261 scans**, every scan that prints + the label. That one is the cheap failure: loud, immediate, and naming the + mapping in its message. + + **The two duplicate-key checks are the expensive ones, because they + passed.** `test_no_two_mappings_claim_the_same_ascconv_key` keys an entry's + identity on `(ascconv_key, sequences, when, bit)` and + `test_no_protocol_has_two_mappings_writing_one_key` on `(ascconv_key, + bit)`; with one preview-only mapping both tuples stay unique and both tests + are green. With a *second* they would pair the two on their shared empty + key and report a collision -- between two mappings that write nothing at + all, `resolve` refusing both. A check that inverts on the second instance + of a thing is worse than one that fails on the first, since the first + instance is when someone is looking. The identity of a preview-only mapping + is its `preview_path`, and the write-race check simply has no business + counting a mapping that never writes. + + The remaining two are the shapes worth recognising because nothing is wrong + yet. `probe._is_mapped("")` became `True`: the empty key joined the claimed + set, which put the empty *stem* in `stems` too, and no real ASCCONV key + splits to an empty stem -- so nothing is skipped today and the reason it is + safe is a fact about key spelling rather than anything the function says. + And `build.stored_display` was saved by accident: it returns early on + `mapping.choices`, so it never reaches the key. A keyless mapping *without* + choices would have gone straight through it. + + This is the sibling of the round-5 widening bug -- a scoping rule enforced + in the table and not in the thing that edits the table is enforced nowhere + -- with the sweeps standing in for the editor. The discipline it argues for + is mechanical: after widening a `Mapping` field, grep every reader of that + field and ask of each one what it does with the new value, rather than + auditing the call sites the feature happens to traverse. `grep -rn + ascconv_key src/ tests/` less the table's own definitions is 49 lines, and + the four that mattered are not the four a reading of the feature suggests. + + **Only the whole suite finds this, which is the concrete case for the rule + that says to run it.** The mapping's own three tests passed, and were + falsified three ways -- wrong code, swapped labels, writer stops refusing -- + all of which exercise the preview side the feature added and none of which + touch a generic sweep in another part of the test file. Falsification proves + the test you wrote works; it says nothing about the tests you did not run. - **`clean` refuses a finding whose scan time moved, and that costs twelve readings across the two rounds.** The rule requires that the console recomputed *nothing*, which is deliberately stricter than the question diff --git a/src/siemens_protocol/analysis/generate/probe.py b/src/siemens_protocol/analysis/generate/probe.py index 3e70763..990550b 100644 --- a/src/siemens_protocol/analysis/generate/probe.py +++ b/src/siemens_protocol/analysis/generate/probe.py @@ -839,11 +839,20 @@ def _is_mapped(key: str) -> bool: ------- bool True when some mapping claims it, its wildcard form, or its stem. + + Notes + ----- + A mapping with no ``ascconv_key`` claims no assignment at all -- it reads + its label out of ``Preview`` -- so it contributes nothing here. Letting the + empty key into the claimed set makes the empty *stem* a member too, and a + probe target is then compared against it; no ASCCONV key spells itself that + way, so nothing is skipped today, which is exactly why it would go + unnoticed. """ wild = re.sub(r"\[\d+\]", "[*]", key) - claimed = {mapping.ascconv_key for mapping in mappings.MAPPINGS} + claimed = {mapping.ascconv_key for mapping in mappings.MAPPINGS if mapping.ascconv_key} stems = {name.split("[")[0] for name in claimed} - return key in claimed or wild in claimed or key.split("[")[0] in stems + return bool(key) and (key in claimed or wild in claimed or key.split("[")[0] in stems) # -------------------------------------------------------------------------- diff --git a/tests/test_exar_patch.py b/tests/test_exar_patch.py index 463a102..9040ad8 100644 --- a/tests/test_exar_patch.py +++ b/tests/test_exar_patch.py @@ -1223,11 +1223,18 @@ def test_no_two_mappings_claim_the_same_ascconv_key() -> None: different things -- so the identity of an entry is the target together with the scope it applies in. + A preview-only mapping's target is its ``preview_path``: keying it on the + empty ``ascconv_key`` instead would let one such entry pass and report the + second as a duplicate of the first, which is the opposite of what this + check is for. + Returns ------- None """ - identity = [(m.ascconv_key, m.sequences, m.when, m.bit) for m in mappings.MAPPINGS] + identity = [ + (m.ascconv_key or m.preview_path, m.sequences, m.when, m.bit) for m in mappings.MAPPINGS + ] assert len(identity) == len(set(identity)) @@ -1240,6 +1247,10 @@ def test_no_protocol_has_two_mappings_writing_one_key(protocol_archive_path: str ``alFree[0]`` and touch a bit each. What would race is two mappings in scope for one protocol writing the same *whole* assignment. + A mapping with no ``ascconv_key`` writes nothing -- ``resolve`` refuses it + -- so it cannot race with anything and is left out. Counting it would pair + two such mappings on their shared empty key and call it a collision. + Parameters ---------- protocol_archive_path : str @@ -1255,7 +1266,9 @@ def test_no_protocol_has_two_mappings_writing_one_key(protocol_archive_path: str continue protocol = step.protocol keys = [ - (m.ascconv_key, m.bit) for m in mappings.MAPPINGS if mappings.applies_to(m, protocol) + (m.ascconv_key, m.bit) + for m in mappings.MAPPINGS + if m.ascconv_key and mappings.applies_to(m, protocol) ] assert len(keys) == len(set(keys)), ( f"{step.name} ({ascconv.sequence_of(protocol)}) has two mappings " @@ -1703,11 +1716,18 @@ def test_every_enum_choice_agrees_with_the_corpus() -> None: choice ``Mapping.absent_choice`` names -- ``Protocol filename`` shows ``Generic`` that way, which is stored as ``1`` whenever it is stored. + A mapping with no ``ascconv_key`` is outside this sweep entirely, not an + absent assignment: it reads its code from ``Preview`` because no single + assignment holds the label, so asking the ASCCONV block for one finds + nothing on every scan and reports the whole mapping as disagreeing. + ``test_the_composite_signal_mode_agrees_with_every_printout_that_names_it`` + is the same check on the side those mappings do carry. + Returns ------- None """ - enums = [m for m in mappings.MAPPINGS if m.choices and m.bit is None] + enums = [m for m in mappings.MAPPINGS if m.choices and m.bit is None and m.ascconv_key] checked, wrong = 0, [] for path, _version in EXAR_PROTOCOL_FILES + [(a, "XA60") for a, _p in PARAMCHECK_PAIRS]: pdf = os.path.splitext(path)[0] + ".pdf"