From 8b80ef30732c399d139477ec37107cf7a6c996b9 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 23 Aug 2026 17:54:59 -0500 Subject: [PATCH 1/6] test(uploads): the retention runner's audit attribution had no guard (BACKLOG #1224) Two paths prune expired uploads and both audit as actor="system": the save-time sweep in upload_file, and the background UploadRetentionRunner wired in create_managed_app's lifespan. ONLY THE FIRST HAD A TEST. MEASURED, WHICH IS WHY THE ROW WAS RIGHT TO ASK FOR THIS. Reverting the lifespan closure to actor=meta.uploader left the ENTIRE suite green -- 13578 passed, byte-identical to baseline -- while the same mutation on the request-path site reddened exactly one test. THE CODE IS CORRECT AT BOTH SITES AND I CHANGED NEITHER. The item's other limbs are already shipped; this is the third, and it is a test. WHY THE SUITE MISSED IT, AND IT IS NOT THAT NOTHING DRIVES A LIFESPAN. Tests DO construct the real closure and DO reach run_once -- test_asvs_gcm_invocation_bound.py enters a managed app's lifespan. It prunes ZERO files, because nothing in those fixtures is aged, so the loop body carrying actor="system" never executes. A PATH THAT RUNS BUT NEVER ENTERS ITS BRANCH IS INVISIBLE TO COVERAGE-BY-EXECUTION. TWO LIFESPANS ARE LOAD-BEARING. _run calls run_once BEFORE its first sleep, so the sweep happens at startup; the file has to be aged BETWEEN two startups. One lifespan cannot both create and prune. PROVED ABLE TO FAIL, with the anchor verified UNIQUE before mutating so it could not hit the wrong site: lifespan site -> actor=meta.uploader MY GUARD REDS, assert 'op' == 'system' the SIBLING request-path test STAYS GREEN -- the control proving the two sites are independently covered, and that the old suite genuinely could not see this one restored both pass The sweep runs in a task the lifespan does not await, so the test POLLS for the row rather than sleeping a fixed amount -- a fixed sleep is either flaky or slow. 25 passed in tests/test_upload_api.py, ruff check and format clean. --- tests/test_upload_api.py | 88 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/tests/test_upload_api.py b/tests/test_upload_api.py index 964eead8..c40cc2b0 100644 --- a/tests/test_upload_api.py +++ b/tests/test_upload_api.py @@ -7,6 +7,7 @@ from __future__ import annotations +import asyncio import json from collections.abc import AsyncIterator from pathlib import Path @@ -870,3 +871,90 @@ async def test_uploaded_browse_moves_the_needle_into_a_post_body( browse = [a for a in await engine.store.list_audit() if a["action"] == "upload.browse"] joined = " ".join(str(a["detail"] or "") for a in browse) assert "MRN123" not in joined # the needle value is still never audited + + +async def test_the_RUNNER_prune_audit_row_also_names_the_system(tmp_path: Path) -> None: + """BACKLOG #1224, THE SECOND SITE -- the one the suite could not see. + + Two code paths prune expired uploads and BOTH audit as ``actor="system"``: the save-time sweep + inside ``upload_file`` (guarded by the sibling test above) and the background + ``UploadRetentionRunner`` wired in ``create_managed_app``'s lifespan. Only the first had a test. + + **MEASURED, WHICH IS WHY THIS EXISTS.** Reverting the lifespan closure to + ``actor=meta.uploader`` left the ENTIRE suite green -- 13578 passed, byte-identical to baseline -- + while the same mutation on the request-path site reddened exactly one test. So the runner's + attribution was unguarded, and the item's own scope names a test for it as the open limb. + + **WHY THE SUITE MISSED IT, and it is not that nothing drives a lifespan.** Tests DO construct the + real closure and DO reach ``run_once`` -- ``tests/test_asvs_gcm_invocation_bound.py`` enters a + managed app's lifespan. It prunes ZERO files, because nothing in those fixtures is aged, so the + loop body carrying ``actor="system"`` never executes. **A path that runs but never enters its + branch is invisible to coverage-by-execution**, which is the same green-and-blind shape the + surrounding items are about. + + **TWO LIFESPANS ARE LOAD-BEARING, not incidental.** ``_run`` calls ``run_once`` BEFORE its first + sleep, so the sweep happens at startup. The file therefore has to be aged BETWEEN two startups: + the first app creates it (the runner's sweep has already passed), the second app's sweep finds it + expired. One lifespan cannot both create and prune. + """ + import dataclasses + import time + + pytest.importorskip("psutil") + from messagefoundry.api import create_managed_app + from messagefoundry.store.crypto import generate_key + + uploads = tmp_path / "runner-uploads" + settings = StoreSettings( + path=str(tmp_path / "runner.db"), + encryption_key=generate_key(), + uploads_dir=str(uploads), + max_upload_bytes=1_000_000, + uploads_retention_days=30, + ) + + # PASS 1 -- create the file, then age it past the window. The runner's startup sweep for THIS app + # already ran before the file existed, so nothing is pruned here. + app1 = create_managed_app(store_settings=settings, poll_interval=0.05) + async with app1.router.lifespan_context(app1): + us = app1.state.upload_store + assert us is not None, "[store].uploads_dir was set -- the subsystem must be wired" + meta = await us.save( + data=BATCH.encode(), filename="acme.hl7", uploader="op", uploader_id="u-1" + ) + aged = dataclasses.replace(meta, uploaded_at=time.time() - 31 * 86_400) + (uploads / f"{meta.file_id}.meta").write_text( + us._encrypt_meta(aged), # noqa: SLF001 -- mirrors the sibling test's backdating + encoding="utf-8", + ) + assert await app1.state.engine.store.list_audit() is not None + + # PASS 2 -- a fresh app over the SAME db and uploads dir. Its startup sweep finds the aged file + # and calls the REAL _audit_upload_prune closure. + app2 = create_managed_app(store_settings=settings, poll_interval=0.05) + rows: list[dict[str, object]] = [] + async with app2.router.lifespan_context(app2): + # The sweep runs in a task the lifespan does not await, so poll rather than sleeping a fixed + # amount -- a fixed sleep is either flaky or slow, and this is neither. + for _ in range(200): + rows = [ + a + for a in await app2.state.engine.store.list_audit() + if a["action"] == "upload.prune" + ] + if rows: + break + await asyncio.sleep(0.02) + + assert len(rows) == 1, ( + f"the runner's sweep should have pruned exactly the aged file, got {rows}" + ) + assert rows[0]["actor"] == "system", ( + "the retention runner has no operator and no request behind it; naming the pruned file's " + "uploader attributes an automated deletion to someone who did not perform it" + ) + detail = json.loads(str(rows[0]["detail"])) + assert detail["uploader"] == "op", ( + "the owner must survive as DATA in detail -- dropping it would trade a false attribution " + "for an unreadable row" + ) From 16ef31ea755e04b8f72f4adc3131bb583eaf1396 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 23 Aug 2026 18:08:28 -0500 Subject: [PATCH 2/6] fix(ci): a native crash in the main suite reported as a test failure (BACKLOG #1260) A segfault kills the interpreter, so pytest returns 139 with no verdict -- and THREE layers of naming then say "tests failed": the check name, the step name `Tests (pytest)`, and `steps.tests.outcome`, which is what step_margin.py consumes. NOT ONE OF THEM IS TRUE. The engine was fine; a process died. LEGIBILITY BEFORE COVERAGE, and the order is the item's, not mine. Wrapping the step makes the failure RETRY; it does not make it LEGIBLE, and a retry that then succeeds erases the crash from view entirely. So the annotations changed first: NATIVE CRASH is now the leading token in both the retry warning and the persisted-crash error. THE ATTRIBUTION WAS THE PART I DID NOT EXPECT. The wrapper hard-coded "likely the pyodbc py3.14 parameter-binding segfault" into every message. That class is ESTABLISHED for the nine database call sites and is NOT established for the main suite -- the item explicitly refuses to conclude it. Wrapping the step as-is would have fixed a legibility defect by printing a mechanism nobody has measured onto the busiest leg in the repo. So the clause is now per-caller. It DEFAULTS to the pyodbc text, which keeps all nine existing call sites saying exactly what they say today; only a caller that has not established the class opts out, and the main suite does. Making it opt-IN would have silently stripped nine correct attributions to fix one wrong one. MY OWN FIRST DRAFT PLANTED THE TOKEN ANYWAY and its own test caught it: the opt-out message read "do not assume the pyodbc class", which puts the word in the annotation, so a log grep for pyodbc would match the one leg where the class is explicitly not established. It now names no class. CITATION DRIFT, RE-DERIVED FROM THE SYMBOL RATHER THAN INHERITED. The row's banner cites ci.yml:780 and is CORRECT -- that is the run line. Its body cites :648 and :673 for the step, which is now :728; it puts the wrapper invocations in the 1156-1374 band, which is now 1644-1860; and it says ten invocations, which is now nine. PROVED ABLE TO FAIL, four mutations, each anchor verified unique first: unwrap the main-suite step -> the coverage arm reds, alone drop the empty-cause declaration -> the cause-unproven arm reds, alone retry on ANY non-zero exit -> the never-retry-a-failure arm reds, alone always use the pyodbc clause -> the not-established arm reds, alone TWO TRAPS THE WORK HIT ITSELF. Passing the bare name "bash" to subprocess let Windows resolve it against the child's PATH, which found WSL's bash rather than the Git Bash `shutil.which` reported -- so the skipif guarded one interpreter while the test ran another. And the control-character gate caught a real BACKSPACE byte in a comment, where `System32\bash` in a non-raw string became 0x08. Both are fixed and the first is written into the test as a comment. 145 passed across tests/test_ci_*.py plus the partition gate; manifest line included. --- .github/workflows/ci.yml | 16 ++- scripts/ci/retry-native-crash.sh | 26 ++++- tests/test_ci_retry_native_crash.py | 165 ++++++++++++++++++++++++++++ tests/tooling_manifest.txt | 1 + 4 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 tests/test_ci_retry_native_crash.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ec7aed61..405c7e67 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -740,6 +740,9 @@ jobs: FAULT_TIMEOUT: ${{ matrix.fault_timeout }} PYTEST_TIMEOUT: ${{ matrix.pytest_timeout }} PYTEST_WORKERS: ${{ matrix.pytest_workers }} + # BACKLOG #1260: empty on purpose -- see the comment above the run line. The wrapper's + # default clause names the pyodbc class, which is not established for this leg. + RETRY_NATIVE_CRASH_CAUSE: "" # `--ignore-glob` subtracts the web console package, which runs as its OWN step below. BACKLOG # #1027 added that package to the root `testpaths` so a bare local `pytest` stops silently # excluding it; without this subtraction the SAME 356 tests would then run TWICE on every leg. @@ -777,7 +780,18 @@ jobs: # what happens when the selection lives in the run body -- the obvious spelling (`--ignore`) was # the one that silently did nothing. One list, read in one place, asserted by # tests/test_tooling_partition.py, which also pins the two halves of this wiring. - run: pytest -q -n "$PYTEST_WORKERS" --dist loadfile -m 'not tooling' --ignore-glob='*messagefoundry-webconsole*' -o faulthandler_timeout="$FAULT_TIMEOUT" --timeout="$PYTEST_TIMEOUT" + # BACKLOG #1260: WRAPPED SO A NATIVE CRASH IS NOT REPORTED AS A TEST FAILURE. A segfault kills + # the interpreter, so pytest returns 139/134 with no verdict and THREE layers of naming then + # say "tests failed" -- the check name, the step name, and `steps.tests.outcome`, which is what + # `step_margin.py` below consumes. None of them is true: the engine was fine and a process died. + # The wrapper re-runs ONLY on 139/134 and re-raises exit 1 immediately, so it cannot mask a + # regression. + # + # RETRY_NATIVE_CRASH_CAUSE IS DELIBERATELY EMPTY HERE. The wrapper's default clause names the + # pyodbc py3.14 class, which is ESTABLISHED for the database legs and is NOT established for + # this one -- the item filing this refuses to conclude it. An empty value makes the annotation + # say CAUSE NOT ESTABLISHED rather than assert a mechanism nobody has measured on this leg. + run: bash scripts/ci/retry-native-crash.sh pytest -q -n "$PYTEST_WORKERS" --dist loadfile -m 'not tooling' --ignore-glob='*messagefoundry-webconsole*' -o faulthandler_timeout="$FAULT_TIMEOUT" --timeout="$PYTEST_TIMEOUT" # THE MARGIN CHECK (BACKLOG #344 proposal 1). Last in the job so a LOW margin cannot skip a suite # that has not run yet -- a step `if:` with no status function carries an implicit `success()`, diff --git a/scripts/ci/retry-native-crash.sh b/scripts/ci/retry-native-crash.sh index 37e976d1..1b91b2f2 100644 --- a/scripts/ci/retry-native-crash.sh +++ b/scripts/ci/retry-native-crash.sh @@ -22,17 +22,37 @@ # codes below. A genuine test failure exits 1 and is re-raised immediately, never retried. # Our own Python cannot cause a native segfault — a real logic regression surfaces as a # pytest assertion (exit 1), so this wrapper can never hide one. Each retry emits a visible -# ::warning:: (grep CI logs for "Native crash" to track the flake frequency against #1459). +# ::warning:: (grep CI logs for "NATIVE CRASH" to track the flake frequency against #1459). # # REMOVE THIS WRAPPER once #1459 ships a fix and pyproject's pyodbc floor moves to the fixed # release (the throughput-invariant step in .github/workflows/ci.yml calls this). # +# THE ATTRIBUTION IS PER-CALLER, AND THAT IS THE POINT (BACKLOG #1260). The pyodbc class above is +# ESTABLISHED for the database legs and is NOT established anywhere else. A wrapper that names it +# unconditionally would print a cause it has not measured onto every leg it is ever added to -- a +# true observation (a native crash happened) carrying an invented mechanism, which is the harder +# error to catch because the part a reader checks is true. So callers where the class is NOT known +# set RETRY_NATIVE_CRASH_CAUSE="" and the message says so in words. +# # Usage: scripts/ci/retry-native-crash.sh [args...] # Env: RETRY_NATIVE_CRASH_ATTEMPTS (default 3) +# RETRY_NATIVE_CRASH_CAUSE attribution clause; default names the pyodbc class (correct for +# the database legs). Set to "" on any leg where it is unproven. set -uo pipefail attempts="${RETRY_NATIVE_CRASH_ATTEMPTS:-3}" +# Defaulting to the pyodbc clause keeps the nine database-leg call sites saying exactly what they +# say today; only a caller that has NOT established the class has to opt out. +default_cause=" -- likely the pyodbc py3.14 parameter-binding segfault (mkleehammer/pyodbc#1459)" +cause="${RETRY_NATIVE_CRASH_CAUSE-$default_cause}" +if [ -z "$cause" ]; then + # DELIBERATELY NAMES NO CLASS, NOT EVEN TO WARN AGAINST ONE. An earlier draft said "do not + # assume the pyodbc class" and its own test caught it: that puts the token in the annotation, + # so a log grep for pyodbc matches the ONE leg where the class is explicitly not established. + cause=" -- CAUSE NOT ESTABLISHED for this leg; do not infer the database-leg crash class" +fi + # A process killed by signal N exits with 128+N. 139 = 128+SIGSEGV(11) (the observed # segfault); 134 = 128+SIGABRT(6) (the param/TVP path can abort() with a core dump instead). is_native_crash() { @@ -51,9 +71,9 @@ for n in $(seq 1 "$attempts"); do exit "$rc" fi if [ "$n" -lt "$attempts" ]; then - echo "::warning::Native crash (exit ${rc}) on attempt ${n}/${attempts} — likely the pyodbc py3.14 parameter-binding segfault (mkleehammer/pyodbc#1459); retrying." + echo "::warning::NATIVE CRASH (exit ${rc}) on attempt ${n}/${attempts}${cause}; retrying." fi done -echo "::error::Command still crashing after ${attempts} attempts (exit ${rc}); see mkleehammer/pyodbc#1459." +echo "::error::NATIVE CRASH persisted: still crashing after ${attempts} attempts (exit ${rc})${cause}." exit "$rc" diff --git a/tests/test_ci_retry_native_crash.py b/tests/test_ci_retry_native_crash.py new file mode 100644 index 00000000..adc18bf8 --- /dev/null +++ b/tests/test_ci_retry_native_crash.py @@ -0,0 +1,165 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""A native crash must not report as a test failure, and a test failure must never be retried (#1260). + +THE FILED DEFECT IS A NAMING ONE. A segfault kills the interpreter, so pytest returns 139 with no +verdict -- and THREE layers of naming then say "tests failed": the check name ``test (windows-2025, +py3.14)``, the step name ``Tests (pytest)``, and ``steps.tests.outcome``, which is what +``scripts/ci/step_margin.py`` consumes. **Not one of them is true.** The engine was fine; a process +died. + +THE ORDER OF THE TWO HALVES IS LOAD-BEARING, and the item says so. Wrapping the step makes the +failure RETRY; it does not make it LEGIBLE. If the retry then succeeds, the crash vanishes from view +entirely -- so the legibility arms below are the point, and the coverage arm is what makes them +reachable from the main suite at all. + +WHY ``exit 139`` RATHER THAN A REAL SEGFAULT. A process killed by SIGSEGV exits 128+11 = 139, and +139 is precisely what the wrapper branches on. Faulting a real process would test the operating +system; these arms test the contract the wrapper actually implements, on every runner, in +milliseconds. The one thing they cannot show is that a real crash produces 139 -- that is the +wrapper's own documented premise and is not re-derived here. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[1] +_WRAPPER = _ROOT / "scripts" / "ci" / "retry-native-crash.sh" +_CI = _ROOT / ".github" / "workflows" / "ci.yml" + +#: RESOLVE BASH ONCE AND INVOKE THAT EXACT BINARY. Passing the bare name "bash" to subprocess lets +#: Windows resolve it against the child's PATH, which on a machine with WSL installed finds +#: System32/bash.exe -- a DIFFERENT interpreter that cannot see a C:/... path and reports +#: the wrapper as "No such file or directory". Measured here: bare `bash` launched WSL while +#: shutil.which returned Git Bash. So a skipif on shutil.which would have guarded one interpreter +#: while the test exercised another -- a control witnessing something other than what runs. +_BASH = shutil.which("bash") + +pytestmark = pytest.mark.skipif(_BASH is None, reason="the wrapper is a bash script") + + +def _run( + exit_code: int, *, attempts: str = "3", cause: str | None = None +) -> subprocess.CompletedProcess[str]: + """Drive the REAL wrapper against a command with a known exit code, counting invocations.""" + env = dict(os.environ, RETRY_NATIVE_CRASH_ATTEMPTS=attempts) + if cause is not None: + env["RETRY_NATIVE_CRASH_CAUSE"] = cause + return subprocess.run( + # as_posix, NOT str: a backslash path reaches bash as escapes and collapses to one + # mangled word. Forward slashes survive, and _BASH pins WHICH bash reads them. + [str(_BASH), _WRAPPER.as_posix(), "bash", "-c", f"echo ran; exit {exit_code}"], + capture_output=True, + text=True, + timeout=60, + check=False, + env=env, + ) + + +# --------------------------------------------------------------------------------------------- +# THE RETRY PAIR -- it must fire on a crash and must NEVER fire on a failure +# --------------------------------------------------------------------------------------------- + + +@pytest.mark.parametrize("code", [139, 134]) +def test_a_native_crash_retries_and_says_so(code: int) -> None: + """MUST FIRE. 139 is 128+SIGSEGV and 134 is 128+SIGABRT -- the two the wrapper branches on.""" + r = _run(code, attempts="3") + assert r.stdout.count("ran") == 3, f"expected 3 attempts, got {r.stdout!r}" + assert r.returncode == code, "the crash exit must survive the retries, not be flattened" + assert "NATIVE CRASH" in r.stdout + + +def test_an_ORDINARY_FAILURE_IS_NEVER_RETRIED(exit_code: int = 1) -> None: + """MUST NOT FIRE, AND THIS IS THE ARM THAT KEEPS THE WRAPPER FROM LAUNDERING A REGRESSION. + + Without it, "retry on crash" could be implemented as "retry on anything" and every arm above + would still pass -- while a real test failure got three chances to flake green.""" + r = _run(exit_code, attempts="3") + assert r.stdout.count("ran") == 1, "a test failure must run exactly once" + assert r.returncode == 1 + assert "not a native crash" in r.stdout + assert "NATIVE CRASH" not in r.stdout + + +def test_success_passes_straight_through() -> None: + """MUST NOT FIRE. A wrapper that re-ran a passing command would triple every green leg.""" + r = _run(0, attempts="3") + assert r.stdout.count("ran") == 1 + assert r.returncode == 0 + + +# --------------------------------------------------------------------------------------------- +# THE ATTRIBUTION PAIR -- the wrapper must not assert a cause it has not measured +# --------------------------------------------------------------------------------------------- + + +def test_a_leg_with_no_established_cause_says_CAUSE_NOT_ESTABLISHED() -> None: + """MUST NOT NAME pyodbc. The class is established for the DATABASE legs and for nothing else. + + A wrapper that named it unconditionally would print a mechanism it has not measured onto every + leg it is ever added to -- a true observation (a native crash happened) carrying an invented + cause, which is the harder error to catch because the part a reader checks is true.""" + r = _run(139, attempts="1", cause="") + assert "CAUSE NOT ESTABLISHED" in r.stdout + assert "pyodbc" not in r.stdout, "an unproven attribution must not reach the annotation" + + +def test_the_database_legs_keep_their_ESTABLISHED_attribution() -> None: + """MUST NAME pyodbc -- the twin, and the reason the default is not simply blank. + + Nine call sites have that class established against an upstream issue. Making the attribution + opt-IN would have silently stripped nine correct annotations to fix one wrong one.""" + r = _run(139, attempts="1") + assert "pyodbc" in r.stdout + assert "1459" in r.stdout + assert "CAUSE NOT ESTABLISHED" not in r.stdout + + +# --------------------------------------------------------------------------------------------- +# THE COVERAGE ARM -- the gap the item was filed for +# --------------------------------------------------------------------------------------------- + + +def test_the_main_suite_step_is_wrapped() -> None: + """THE FILED GAP. Every retry-native-crash invocation sat in the database legs; the main suite's + run line was a bare pytest, so the one leg most people read could not tell a crash from a + failure.""" + text = _CI.read_text(encoding="utf-8") + step = text.split("- name: Tests (pytest)", 1) + assert len(step) == 2, "the Tests (pytest) step is gone -- re-derive this from the workflow" + body = step[1].split("- name:", 1)[0] + assert "retry-native-crash.sh" in body, ( + "the main suite's run line is unwrapped again; a native crash there reports as a test failure" + ) + + +def test_the_main_suite_step_declares_its_cause_unproven() -> None: + """The coverage arm alone would let the pyodbc clause reach a leg where it is not established -- + which would fix the legibility defect by introducing an attribution one.""" + text = _CI.read_text(encoding="utf-8") + body = text.split("- name: Tests (pytest)", 1)[1].split("- name:", 1)[0] + assert 'RETRY_NATIVE_CRASH_CAUSE: ""' in body + + +def test_the_wrappers_own_grep_hint_matches_what_it_prints() -> None: + """The header tells a reader which string to grep CI logs for. If the annotation's wording drifts + from that hint, the documented search returns nothing and reads as 'no crashes ever happened' -- + the same silent-zero shape the surrounding items are about.""" + src = _WRAPPER.read_text(encoding="utf-8") + hint = [ln for ln in src.splitlines() if "grep CI logs for" in ln] + assert len(hint) == 1, hint + token = hint[0].split('"')[1] + assert f"::warning::{token}" in src or f"::warning::{token}" in src.replace( + "::warning::", "::warning::" + ) + assert token in src.split("is_native_crash", 1)[1], ( + "the hint names a string the messages do not print" + ) diff --git a/tests/tooling_manifest.txt b/tests/tooling_manifest.txt index 80876ee9..6740cf59 100644 --- a/tests/tooling_manifest.txt +++ b/tests/tooling_manifest.txt @@ -39,6 +39,7 @@ tests/test_blanket_stage_guard.py tests/test_ci_docs_only_detector.py tests/test_ci_engine_step_excludes_webconsole.py tests/test_ci_leg_data_class.py +tests/test_ci_retry_native_crash.py tests/test_ci_step_margin.py tests/test_ci_tooling_gate.py tests/test_ci_venv_pinning.py From 01c03cee3f94196ff0fc02948fdf59d1091daae5 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 23 Aug 2026 18:24:39 -0500 Subject: [PATCH 3/6] fix(ci): removing the crash wrapper must not silently strip a leg it was never about (BACKLOG #1260) THE DEFECT IS A FUTURE-READER ONE AND NOTHING AT RUNTIME CAN CATCH IT. The wrapper's header tells whoever fixes upstream pyodbc #1459 to remove it. That is right for the database legs and wrong for any caller whose crash cause is not that class -- and my own commit had just made the main engine suite exactly such a caller. So I fixed the ANNOTATION so it would not assert an unmeasured cause, and left the REMOVAL INSTRUCTION able to delete that leg's crash handling on the strength of an upstream fix that does not address it. THE NOTE WAS ALREADY UNDERSTATING ITSELF BEFORE I TOUCHED IT: it named one call site, "the throughput-invariant step", when ci.yml had TEN. THE DISCRIMINATOR IS NOW IN THE WORKFLOW RATHER THAN IN A MEMORY. Any caller setting RETRY_NATIVE_CRASH_CAUSE="" has declared its cause is not established as the pyodbc class, so a #1459 fix does not license removing the wrapper there. The note says to remove it from the pyodbc callers and decide each opted-out caller separately. PROVED ABLE TO FAIL, and the second arm is the one I care about: revert the note to its one-caller wording -> only the new guard reds remove the opt-out from ci.yml entirely -> the guard reds TOO, refusing to pass vacuously That second mutation is the point. A guard conditioned on "if any caller opts out" would quietly become a no-op the day nobody does, so it asserts the precondition it depends on rather than skipping when the precondition is absent. FOUND BY THE LANDER, from a 2026-08-14 commit on PR 433 that implements this item differently and argues in its own comment against wrapping this leg at all. Their design and mine disagree; the row's 2026-08-20 re-score still names the bare pytest as the gap, so the wrapper stays. This is the one objection of theirs that neither design answered. 22 passed across the CI retry, engine-step and partition suites. --- scripts/ci/retry-native-crash.sh | 16 +++++++++++++-- tests/test_ci_retry_native_crash.py | 31 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/scripts/ci/retry-native-crash.sh b/scripts/ci/retry-native-crash.sh index 1b91b2f2..68e1bbac 100644 --- a/scripts/ci/retry-native-crash.sh +++ b/scripts/ci/retry-native-crash.sh @@ -24,8 +24,20 @@ # pytest assertion (exit 1), so this wrapper can never hide one. Each retry emits a visible # ::warning:: (grep CI logs for "NATIVE CRASH" to track the flake frequency against #1459). # -# REMOVE THIS WRAPPER once #1459 ships a fix and pyproject's pyodbc floor moves to the fixed -# release (the throughput-invariant step in .github/workflows/ci.yml calls this). +# REMOVING THIS WRAPPER IS NOT A BLANKET DELETION -- READ THE CALLER SET FIRST (BACKLOG #1260). +# #1459 covers the DATABASE legs. It says nothing about any other caller, and this note previously +# named one call site ("the throughput-invariant step") when ci.yml had TEN. +# +# THE DISCRIMINATOR IS IN THE WORKFLOW, NOT IN A MEMORY: any caller setting +# RETRY_NATIVE_CRASH_CAUSE="" has declared that ITS crash cause is NOT established as the pyodbc +# class. A fix to #1459 therefore does not license removing the wrapper from that leg -- doing so +# would silently strip its crash handling on the strength of an upstream fix that does not address +# it. Today the engine test suite is such a caller. +# +# SO: when #1459 ships and pyproject's pyodbc floor moves, remove the wrapper from the pyodbc +# callers, and decide each opted-out caller SEPARATELY on its own evidence. +# `tests/test_ci_retry_native_crash.py` fails if an opted-out caller exists and this note stops +# saying so, because the whole defect is a future reader deleting one line in good faith. # # THE ATTRIBUTION IS PER-CALLER, AND THAT IS THE POINT (BACKLOG #1260). The pyodbc class above is # ESTABLISHED for the database legs and is NOT established anywhere else. A wrapper that names it diff --git a/tests/test_ci_retry_native_crash.py b/tests/test_ci_retry_native_crash.py index adc18bf8..5498a64c 100644 --- a/tests/test_ci_retry_native_crash.py +++ b/tests/test_ci_retry_native_crash.py @@ -163,3 +163,34 @@ def test_the_wrappers_own_grep_hint_matches_what_it_prints() -> None: assert token in src.split("is_native_crash", 1)[1], ( "the hint names a string the messages do not print" ) + + +def test_the_removal_note_cannot_silently_strip_an_opted_out_caller() -> None: + """THE DEFECT NEITHER DESIGN ADDRESSED, and it is a future-reader defect rather than a runtime one. + + The wrapper's header instructs whoever fixes upstream pyodbc #1459 to remove it. That is correct + for the database legs and WRONG for any caller whose crash cause is not that class -- and the + note previously named ONE call site while ci.yml had ten. + + So the failure mode is somebody deleting one line in good faith, on the strength of an upstream + fix that does not address the leg they are stripping. Nothing at runtime can catch that; the + only guard is that the instruction stays honest about its own caller set. + + THE PREDICATE IS THE WORKFLOW, NOT A HARDCODED NAME: if any step opts out of the pyodbc + attribution, the removal note must say removal is caller-dependent. Add such a caller without + updating the note and this reds.""" + ci = _CI.read_text(encoding="utf-8") + src = _WRAPPER.read_text(encoding="utf-8") + opted_out = 'RETRY_NATIVE_CRASH_CAUSE: ""' in ci + assert opted_out, ( + "no caller opts out any more -- if that is deliberate this arm is now vacuous and should be " + "deleted with a reason, not left passing over nothing" + ) + note = src.split("REMOVING THIS WRAPPER", 1) + assert len(note) == 2, ( + "the removal instruction no longer warns that removal depends on the caller set; a reader " + "fixing #1459 would strip the opted-out leg's crash handling silently (BACKLOG #1260)" + ) + assert "RETRY_NATIVE_CRASH_CAUSE" in note[1].split("set -uo", 1)[0], ( + "the note must name the DISCRIMINATOR a reader can check, not just caution them in prose" + ) From fb75da860bd1446b5c958af286aeac2324d5e6ca Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 23 Aug 2026 18:11:14 -0500 Subject: [PATCH 4/6] backlog: close #1224 and #1260 -- both rows misdescribed their own defect #1224 says no test drives a lifespan. False at HEAD: tests construct the real closure and reach run_once, but prune zero files, so the loop body never executes. A path that runs but never enters its branch is invisible to coverage-by-execution. #1260 named two limbs and there were three. retry-native-crash.sh hard-coded the pyodbc attribution into every message, so wrapping the main suite as written would have printed an unmeasured mechanism onto the busiest leg in the repo. The clause is per-caller now, defaulting to the pyodbc text so the nine database sites are unchanged. Both verified by content, not from the builders' reports. Co-Authored-By: Claude Opus 5 --- docs/BACKLOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index dca04f74..9b62af94 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -10996,7 +10996,7 @@ gate is the wrong shape, validation of the walk is the right one. ## 1224. The `upload.prune` audit row names the file's owner as the actor of an automated sweep -> 🔢 **Re-scored 2026-08-20 -> P3.** Value **2/10** · Difficulty **2/10** · _fill-in_. Attacked the shipped claim at both cited sites and it survived on the code limb. Site 1 (request-path opportunistic sweep) is now messagefoundry/api/app.py:3918-3936: `actor="system"`, `client` argument removed entirely, and detail carries file_id + uploader + uploader_id, with a comment naming BACKLOG #1224 and citing ADR 0150 decision 4 for dropping the client. Site 2 (background UploadRetentionRunner) is messagefoundry/api/app.py:5891-5907: the lifespan-owned `_audit_upload_prune` closure also writes `actor="system"` with uploader/uploader_id in detail. Enumerated prune call sites repo-wide: `prune_expired(` is called at app.py:3918 and uploads.py:648 (inside `run_once`) only, so at least those two are the whole surface and both are attributed to the system. Where the claim fails is the item's explicit third limb -- "assert the emitted row's actor is the system principal and its uploader detail is unchanged, for BOTH the request-path prune and the background runner". Only the request-path assertion exists (tests/test_upload_api.py:794-814, which also asserts the null client). The runner-path test (tests/test_uploads.py:389) injects a list-appending stub for `audit=` and asserts `[m.file_id for m in audited]`; it never constructs or inspects an audit row, so it would pass unchanged if `_audit_upload_prune` were reverted to `actor=meta.uploader`. That closure is reachable only from the API lifespan, and no test drives a lifespan, so the fixed line has no execution coverage at all. Under the not-deployed rule this is worded conditionally: the false attribution would no longer be written on a first deployment from either path, but the path the item singled out as the one a partial fix abandons is guarded by nothing. _(was 6/10 · 2/10.)_ +> ✅ **SHIPPED 2026-08-23 -- verified by content at `8b80ef30`, tests only, no engine code changed.** Both call sites were already correct; the open limb was coverage. **THIS ROW'S OWN ACCOUNT OF THE GAP IS FALSE AT HEAD.** It says no test drives a lifespan. Tests DO construct the real closure and DO reach `run_once` -- `test_asvs_gcm_invocation_bound.py` enters a managed app's lifespan. It prunes ZERO files, so the loop body carrying `actor="system"` never executes. **A PATH THAT RUNS BUT NEVER ENTERS ITS BRANCH IS INVISIBLE TO COVERAGE-BY-EXECUTION**, which is a different defect from an unreached path and the reason the old suite could not see it. **MUTATION-VERIFIED, ANCHOR CHECKED UNIQUE FIRST so it could not land on the wrong site:** the lifespan closure re-pointed to `actor=meta.uploader` reds the new guard while the sibling request-path test stays green; restoring makes both pass. Reverting the fix previously left 13,578 tests green. 🔢 **Re-scored 2026-08-20 -> P3.** Value **2/10** · Difficulty **2/10** · _fill-in_. Attacked the shipped claim at both cited sites and it survived on the code limb. Site 1 (request-path opportunistic sweep) is now messagefoundry/api/app.py:3918-3936: `actor="system"`, `client` argument removed entirely, and detail carries file_id + uploader + uploader_id, with a comment naming BACKLOG #1224 and citing ADR 0150 decision 4 for dropping the client. Site 2 (background UploadRetentionRunner) is messagefoundry/api/app.py:5891-5907: the lifespan-owned `_audit_upload_prune` closure also writes `actor="system"` with uploader/uploader_id in detail. Enumerated prune call sites repo-wide: `prune_expired(` is called at app.py:3918 and uploads.py:648 (inside `run_once`) only, so at least those two are the whole surface and both are attributed to the system. Where the claim fails is the item's explicit third limb -- "assert the emitted row's actor is the system principal and its uploader detail is unchanged, for BOTH the request-path prune and the background runner". Only the request-path assertion exists (tests/test_upload_api.py:794-814, which also asserts the null client). The runner-path test (tests/test_uploads.py:389) injects a list-appending stub for `audit=` and asserts `[m.file_id for m in audited]`; it never constructs or inspects an audit row, so it would pass unchanged if `_audit_upload_prune` were reverted to `actor=meta.uploader`. That closure is reachable only from the API lifespan, and no test drives a lifespan, so the fixed line has no execution coverage at all. Under the not-deployed rule this is worded conditionally: the false attribution would no longer be written on a first deployment from either path, but the path the item singled out as the one a partial fix abandons is guarded by nothing. _(was 6/10 · 2/10.)_ > > **Filed 2026-08-11 -- read off `origin/main`, TWO sites not one.** The retention sweep is **automated and owner-blind**, but its audit row attributes the deletion to the pruned file's uploader: > ``` @@ -12371,7 +12371,7 @@ BUILDS it.* ## 1260. a native crash in the main suite reports as a test failure, and the crash-retry wrapper does not cover that leg -> 🔢 **Re-scored 2026-08-20 -> P1.** Value **6/10** · Difficulty **2/10** · _quick win_. Gap stands on both halves: the main suite's run line at ci.yml:780 is a bare pytest with no wrapper while every retry-native-crash invocation sits in the database legs, and the only downstream signal is steps.tests.outcome fed to step_margin.py (:801-814), which cannot distinguish exit 139 from a test failure (value 6). The remainder is two small workflow edits, wrapping the step and surfacing the crash exit distinctly, plus the must-trip and must-not-retry arms the item specifies (difficulty 2). _(was 6/10 · 2/10.)_ +> ✅ **SHIPPED 2026-08-23 -- verified by content at `16ef31ea`.** **THE ROW NAMED TWO LIMBS AND THERE WERE THREE.** The third changed the shape of the fix: `scripts/ci/retry-native-crash.sh` HARD-CODED the pyodbc attribution into every message it emits. That class is established for the nine database call sites (`ci.yml:1644`-`:1860`) and is NOT established for the main suite -- as this row itself refuses to conclude. **Wrapping the step as written would have fixed a legibility defect by printing an unmeasured mechanism onto the busiest leg in the repo:** a true observation carrying an invented cause. The clause is now per-caller and DEFAULTS to the pyodbc text, so all nine existing sites read exactly as before and only the main suite opts out; opt-IN would have silently stripped nine correct attributions to fix one wrong one. **AND THE OPT-OUT MESSAGE NAMES NO CLASS AT ALL** -- an earlier draft said "do not assume the pyodbc class", which puts the token in the annotation so a log grep for `pyodbc` matches the ONE leg where the class is explicitly not established. Its own test caught that. **CITATION DRIFT, re-derived from the symbol: the BANNER is right and the BODY has drifted** -- `ci.yml:780` is the run line as cited, but the step is at `:728` not `:648`/`:673`, the invocations are at `1644`-`1860` not the `1156`-`1374` band, and there are NINE, not ten. **FOUR MUTATIONS, each anchor verified unique**, each reddening one arm alone. **STATED LIMIT:** a process killed by `SIGSEGV` exits `139` and `139` is what the wrapper branches on, so the tests drive the contract it implements -- they do NOT show that a real crash produces `139`, which stays the wrapper's own documented premise. 🔢 **Re-scored 2026-08-20 -> P1.** Value **6/10** · Difficulty **2/10** · _quick win_. Gap stands on both halves: the main suite's run line at ci.yml:780 is a bare pytest with no wrapper while every retry-native-crash invocation sits in the database legs, and the only downstream signal is steps.tests.outcome fed to step_margin.py (:801-814), which cannot distinguish exit 139 from a test failure (value 6). The remainder is two small workflow edits, wrapping the step and surfacing the crash exit distinctly, plus the must-trip and must-not-retry arms the item specifies (difficulty 2). _(was 6/10 · 2/10.)_ > > **Filed 2026-08-14 - not started. THE COMPENSATING CONTROL EXISTS, IS CORRECTLY WRITTEN, AND DOES NOT COVER THE PATH THAT FAILED.** Two independent halves: a **reporting** defect that misnames a crash as a test failure, and a **coverage** gap in the retry wrapper. Either can be fixed without the other, and fixing only the second leaves every future crash still misreported. > **THREE LAYERS OF NAMING SAID "TESTS FAILED" AND NOT ONE OF THEM WAS TRUE.** Observed on a pull request whose Windows leg went red: the check is named `test (windows-2025, py3.14)`, the step is named `Tests (pytest)`, and the process exited **139** -- `128 + SIGSEGV(11)` -- printing *"Segmentation fault"*. **Zero tests failed.** The reporting seat proved the log was searchable before concluding absence: `pytest` appears **28 times** in a 100 KB log while there is **no pytest summary line and no FAILED test id anywhere in it**, because the process died before it could produce one. A reader at any of those three layers reaches for a test regression that does not exist. From 2d0a51e94ce4465a06ec8937902b7dc8ea407617 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 23 Aug 2026 18:35:28 -0500 Subject: [PATCH 5/6] backlog: #1260 had a fourth limb, and it is the one a future reader trips The wrapper's removal note said "remove once #1459 ships" and named one call site when ci.yml had ten. #1459 covers the database legs only, so a good-faith deletion would have silently stripped the main suite's crash handling -- the one leg whose cause is explicitly not established. My banner said three limbs. 01c03cee is the fourth, and it landed after the branch I wrote that banner on had already forked. Co-Authored-By: Claude Opus 5 --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 9b62af94..709e5437 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -12371,7 +12371,7 @@ BUILDS it.* ## 1260. a native crash in the main suite reports as a test failure, and the crash-retry wrapper does not cover that leg -> ✅ **SHIPPED 2026-08-23 -- verified by content at `16ef31ea`.** **THE ROW NAMED TWO LIMBS AND THERE WERE THREE.** The third changed the shape of the fix: `scripts/ci/retry-native-crash.sh` HARD-CODED the pyodbc attribution into every message it emits. That class is established for the nine database call sites (`ci.yml:1644`-`:1860`) and is NOT established for the main suite -- as this row itself refuses to conclude. **Wrapping the step as written would have fixed a legibility defect by printing an unmeasured mechanism onto the busiest leg in the repo:** a true observation carrying an invented cause. The clause is now per-caller and DEFAULTS to the pyodbc text, so all nine existing sites read exactly as before and only the main suite opts out; opt-IN would have silently stripped nine correct attributions to fix one wrong one. **AND THE OPT-OUT MESSAGE NAMES NO CLASS AT ALL** -- an earlier draft said "do not assume the pyodbc class", which puts the token in the annotation so a log grep for `pyodbc` matches the ONE leg where the class is explicitly not established. Its own test caught that. **CITATION DRIFT, re-derived from the symbol: the BANNER is right and the BODY has drifted** -- `ci.yml:780` is the run line as cited, but the step is at `:728` not `:648`/`:673`, the invocations are at `1644`-`1860` not the `1156`-`1374` band, and there are NINE, not ten. **FOUR MUTATIONS, each anchor verified unique**, each reddening one arm alone. **STATED LIMIT:** a process killed by `SIGSEGV` exits `139` and `139` is what the wrapper branches on, so the tests drive the contract it implements -- they do NOT show that a real crash produces `139`, which stays the wrapper's own documented premise. 🔢 **Re-scored 2026-08-20 -> P1.** Value **6/10** · Difficulty **2/10** · _quick win_. Gap stands on both halves: the main suite's run line at ci.yml:780 is a bare pytest with no wrapper while every retry-native-crash invocation sits in the database legs, and the only downstream signal is steps.tests.outcome fed to step_margin.py (:801-814), which cannot distinguish exit 139 from a test failure (value 6). The remainder is two small workflow edits, wrapping the step and surfacing the crash exit distinctly, plus the must-trip and must-not-retry arms the item specifies (difficulty 2). _(was 6/10 · 2/10.)_ +> ✅ **SHIPPED 2026-08-23 -- verified by content at `16ef31ea`.** **THE ROW NAMED TWO LIMBS AND THERE WERE FOUR.** The third changed the shape of the fix: `scripts/ci/retry-native-crash.sh` HARD-CODED the pyodbc attribution into every message it emits. That class is established for the nine database call sites (`ci.yml:1644`-`:1860`) and is NOT established for the main suite -- as this row itself refuses to conclude. **Wrapping the step as written would have fixed a legibility defect by printing an unmeasured mechanism onto the busiest leg in the repo:** a true observation carrying an invented cause. The clause is now per-caller and DEFAULTS to the pyodbc text, so all nine existing sites read exactly as before and only the main suite opts out; opt-IN would have silently stripped nine correct attributions to fix one wrong one. **AND THE OPT-OUT MESSAGE NAMES NO CLASS AT ALL** -- an earlier draft said "do not assume the pyodbc class", which puts the token in the annotation so a log grep for `pyodbc` matches the ONE leg where the class is explicitly not established. Its own test caught that. **CITATION DRIFT, re-derived from the symbol: the BANNER is right and the BODY has drifted** -- `ci.yml:780` is the run line as cited, but the step is at `:728` not `:648`/`:673`, the invocations are at `1644`-`1860` not the `1156`-`1374` band, and there are NINE, not ten. **FOUR MUTATIONS, each anchor verified unique**, each reddening one arm alone. **STATED LIMIT:** a process killed by `SIGSEGV` exits `139` and `139` is what the wrapper branches on, so the tests drive the contract it implements -- they do NOT show that a real crash produces `139`, which stays the wrapper's own documented premise. 🔢 **Re-scored 2026-08-20 -> P1.** Value **6/10** · Difficulty **2/10** · _quick win_. Gap stands on both halves: the main suite's run line at ci.yml:780 is a bare pytest with no wrapper while every retry-native-crash invocation sits in the database legs, and the only downstream signal is steps.tests.outcome fed to step_margin.py (:801-814), which cannot distinguish exit 139 from a test failure (value 6). The remainder is two small workflow edits, wrapping the step and surfacing the crash exit distinctly, plus the must-trip and must-not-retry arms the item specifies (difficulty 2). _(was 6/10 · 2/10.)_ **AMENDED: THERE WERE FOUR, and the fourth is a FUTURE-READER defect rather than a runtime one** (`01c03cee`). The wrapper's own removal note read *"remove this wrapper once #1459 ships"* and named ONE call site -- the throughput-invariant step -- when `ci.yml` had TEN. **`#1459` covers the DATABASE legs and says nothing about any other caller**, so a good-faith deletion on the strength of that upstream fix would have SILENTLY STRIPPED the main suite's crash handling: the one leg whose cause is explicitly NOT established. The note now states the discriminator IN THE WORKFLOW rather than in a memory -- any caller setting `RETRY_NATIVE_CRASH_CAUSE=""` has declared its cause unproven, so `#1459` does not license removing the wrapper from it -- and `test_the_removal_note_cannot_silently_strip_an_opted_out_caller` FAILS if an opted-out caller exists while the note stops saying so. > > **Filed 2026-08-14 - not started. THE COMPENSATING CONTROL EXISTS, IS CORRECTLY WRITTEN, AND DOES NOT COVER THE PATH THAT FAILED.** Two independent halves: a **reporting** defect that misnames a crash as a test failure, and a **coverage** gap in the retry wrapper. Either can be fixed without the other, and fixing only the second leaves every future crash still misreported. > **THREE LAYERS OF NAMING SAID "TESTS FAILED" AND NOT ONE OF THEM WAS TRUE.** Observed on a pull request whose Windows leg went red: the check is named `test (windows-2025, py3.14)`, the step is named `Tests (pytest)`, and the process exited **139** -- `128 + SIGSEGV(11)` -- printing *"Segmentation fault"*. **Zero tests failed.** The reporting seat proved the log was searchable before concluding absence: `pytest` appears **28 times** in a 100 KB log while there is **no pytest summary line and no FAILED test id anywhere in it**, because the process died before it could produce one. A reader at any of those three layers reaches for a test regression that does not exist. From a3392f9773b098f28376e53d6aa005958b11987b Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 24 Aug 2026 10:14:25 -0500 Subject: [PATCH 6/6] docs(backlog): qualify the pyodbc issue citation so it cannot arm later Line 12409 cited the upstream pyodbc segfault three times as a bare `#1459`. In `docs/BACKLOG.md` a bare `#N` reads as a ledger citation, and 1459 is above the allocator's high-water mark of 1352 -- so the reference resolves to nothing today and would silently begin resolving to unrelated work the day somebody allocates that number, with nothing anywhere reporting a problem. All three now read `mkleehammer/pyodbc#1459`. This is the form the same branch already uses in `scripts/ci/retry-native-crash.sh`, and the form `docs/testing/master-test-plan/` uses, so it is the house convention rather than a new one. `dangling_citation_check.py` exempts it via _FOREIGN_REPO, which keys on the character preceding the hash, so it works inside backticks. The sentences are correct as written; only the citation was unqualified. Nothing else on the line changed: every other line is byte-identical and the CRLF count is unchanged at 16112. Ledger prose authored by the Dispatcher seat; applied by the Lander seat. Co-Authored-By: Claude Opus 5 --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index c0432a30..29206429 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -12406,7 +12406,7 @@ BUILDS it.* ## 1260. a native crash in the main suite reports as a test failure, and the crash-retry wrapper does not cover that leg -> ✅ **SHIPPED 2026-08-23 -- verified by content at `16ef31ea`.** **THE ROW NAMED TWO LIMBS AND THERE WERE FOUR.** The third changed the shape of the fix: `scripts/ci/retry-native-crash.sh` HARD-CODED the pyodbc attribution into every message it emits. That class is established for the nine database call sites (`ci.yml:1644`-`:1860`) and is NOT established for the main suite -- as this row itself refuses to conclude. **Wrapping the step as written would have fixed a legibility defect by printing an unmeasured mechanism onto the busiest leg in the repo:** a true observation carrying an invented cause. The clause is now per-caller and DEFAULTS to the pyodbc text, so all nine existing sites read exactly as before and only the main suite opts out; opt-IN would have silently stripped nine correct attributions to fix one wrong one. **AND THE OPT-OUT MESSAGE NAMES NO CLASS AT ALL** -- an earlier draft said "do not assume the pyodbc class", which puts the token in the annotation so a log grep for `pyodbc` matches the ONE leg where the class is explicitly not established. Its own test caught that. **CITATION DRIFT, re-derived from the symbol: the BANNER is right and the BODY has drifted** -- `ci.yml:780` is the run line as cited, but the step is at `:728` not `:648`/`:673`, the invocations are at `1644`-`1860` not the `1156`-`1374` band, and there are NINE, not ten. **FOUR MUTATIONS, each anchor verified unique**, each reddening one arm alone. **STATED LIMIT:** a process killed by `SIGSEGV` exits `139` and `139` is what the wrapper branches on, so the tests drive the contract it implements -- they do NOT show that a real crash produces `139`, which stays the wrapper's own documented premise. 🔢 **Re-scored 2026-08-20 -> P1.** Value **6/10** · Difficulty **2/10** · _quick win_. Gap stands on both halves: the main suite's run line at ci.yml:780 is a bare pytest with no wrapper while every retry-native-crash invocation sits in the database legs, and the only downstream signal is steps.tests.outcome fed to step_margin.py (:801-814), which cannot distinguish exit 139 from a test failure (value 6). The remainder is two small workflow edits, wrapping the step and surfacing the crash exit distinctly, plus the must-trip and must-not-retry arms the item specifies (difficulty 2). _(was 6/10 · 2/10.)_ **AMENDED: THERE WERE FOUR, and the fourth is a FUTURE-READER defect rather than a runtime one** (`01c03cee`). The wrapper's own removal note read *"remove this wrapper once #1459 ships"* and named ONE call site -- the throughput-invariant step -- when `ci.yml` had TEN. **`#1459` covers the DATABASE legs and says nothing about any other caller**, so a good-faith deletion on the strength of that upstream fix would have SILENTLY STRIPPED the main suite's crash handling: the one leg whose cause is explicitly NOT established. The note now states the discriminator IN THE WORKFLOW rather than in a memory -- any caller setting `RETRY_NATIVE_CRASH_CAUSE=""` has declared its cause unproven, so `#1459` does not license removing the wrapper from it -- and `test_the_removal_note_cannot_silently_strip_an_opted_out_caller` FAILS if an opted-out caller exists while the note stops saying so. +> ✅ **SHIPPED 2026-08-23 -- verified by content at `16ef31ea`.** **THE ROW NAMED TWO LIMBS AND THERE WERE FOUR.** The third changed the shape of the fix: `scripts/ci/retry-native-crash.sh` HARD-CODED the pyodbc attribution into every message it emits. That class is established for the nine database call sites (`ci.yml:1644`-`:1860`) and is NOT established for the main suite -- as this row itself refuses to conclude. **Wrapping the step as written would have fixed a legibility defect by printing an unmeasured mechanism onto the busiest leg in the repo:** a true observation carrying an invented cause. The clause is now per-caller and DEFAULTS to the pyodbc text, so all nine existing sites read exactly as before and only the main suite opts out; opt-IN would have silently stripped nine correct attributions to fix one wrong one. **AND THE OPT-OUT MESSAGE NAMES NO CLASS AT ALL** -- an earlier draft said "do not assume the pyodbc class", which puts the token in the annotation so a log grep for `pyodbc` matches the ONE leg where the class is explicitly not established. Its own test caught that. **CITATION DRIFT, re-derived from the symbol: the BANNER is right and the BODY has drifted** -- `ci.yml:780` is the run line as cited, but the step is at `:728` not `:648`/`:673`, the invocations are at `1644`-`1860` not the `1156`-`1374` band, and there are NINE, not ten. **FOUR MUTATIONS, each anchor verified unique**, each reddening one arm alone. **STATED LIMIT:** a process killed by `SIGSEGV` exits `139` and `139` is what the wrapper branches on, so the tests drive the contract it implements -- they do NOT show that a real crash produces `139`, which stays the wrapper's own documented premise. 🔢 **Re-scored 2026-08-20 -> P1.** Value **6/10** · Difficulty **2/10** · _quick win_. Gap stands on both halves: the main suite's run line at ci.yml:780 is a bare pytest with no wrapper while every retry-native-crash invocation sits in the database legs, and the only downstream signal is steps.tests.outcome fed to step_margin.py (:801-814), which cannot distinguish exit 139 from a test failure (value 6). The remainder is two small workflow edits, wrapping the step and surfacing the crash exit distinctly, plus the must-trip and must-not-retry arms the item specifies (difficulty 2). _(was 6/10 · 2/10.)_ **AMENDED: THERE WERE FOUR, and the fourth is a FUTURE-READER defect rather than a runtime one** (`01c03cee`). The wrapper's own removal note read *"remove this wrapper once mkleehammer/pyodbc#1459 ships"* and named ONE call site -- the throughput-invariant step -- when `ci.yml` had TEN. **`mkleehammer/pyodbc#1459` covers the DATABASE legs and says nothing about any other caller**, so a good-faith deletion on the strength of that upstream fix would have SILENTLY STRIPPED the main suite's crash handling: the one leg whose cause is explicitly NOT established. The note now states the discriminator IN THE WORKFLOW rather than in a memory -- any caller setting `RETRY_NATIVE_CRASH_CAUSE=""` has declared its cause unproven, so `mkleehammer/pyodbc#1459` does not license removing the wrapper from it -- and `test_the_removal_note_cannot_silently_strip_an_opted_out_caller` FAILS if an opted-out caller exists while the note stops saying so. > > **Filed 2026-08-14 - not started. THE COMPENSATING CONTROL EXISTS, IS CORRECTLY WRITTEN, AND DOES NOT COVER THE PATH THAT FAILED.** Two independent halves: a **reporting** defect that misnames a crash as a test failure, and a **coverage** gap in the retry wrapper. Either can be fixed without the other, and fixing only the second leaves every future crash still misreported. > **THREE LAYERS OF NAMING SAID "TESTS FAILED" AND NOT ONE OF THEM WAS TRUE.** Observed on a pull request whose Windows leg went red: the check is named `test (windows-2025, py3.14)`, the step is named `Tests (pytest)`, and the process exited **139** -- `128 + SIGSEGV(11)` -- printing *"Segmentation fault"*. **Zero tests failed.** The reporting seat proved the log was searchable before concluding absence: `pytest` appears **28 times** in a 100 KB log while there is **no pytest summary line and no FAILED test id anywhere in it**, because the process died before it could produce one. A reader at any of those three layers reaches for a test regression that does not exist.