From ee4de8c65201cdf76fdaa3ad1b9e579b2a8d8d1c Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 14:00:06 -0500 Subject: [PATCH 1/8] chore(gitignore): ignore the quality-advisory generated artifacts before they can be staged The advisory quality jobs produce coverage.xml, a jscpd report dir, a mutmut cache and (with the C901 delta landing next) two ruff JSON files -- none of which were ignored. The ruff JSON is the one that matters. Its `filename` values are ABSOLUTE local paths, so anyone reproducing the delta locally and running `git add -A` stages a home directory path, which is precisely what the leak scanner's home-path detector fires on. Ignoring these first removes the window entirely rather than relying on remembering not to stage them. --- .gitignore | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.gitignore b/.gitignore index a7ff2432..cd2a0aa4 100644 --- a/.gitignore +++ b/.gitignore @@ -59,6 +59,19 @@ secrets/ .vscode/ .idea/ +# CI-only quality artifacts (generated by .github/workflows/quality-advisory.yml, and by anyone +# reproducing those jobs locally). None of these were ignored before, which mattered: the C901 delta +# inputs are ruff JSON carrying ABSOLUTE local paths, so a local `git add -A` would stage a home +# directory path straight into a commit -- exactly what scripts/security/scan_forbidden.py fires on. +coverage.xml +.coverage +diff-cover.md +jscpd-report/ +.mutmut-cache +c901-head.json +c901-base.json +base-tree/ + # Claude Code: settings.json is shared/tracked; settings.local.json is machine-local (never commit) .claude/settings.local.json From c28e15f1309bdc4f8e36fb902b9fd01ea96956cf Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 14:16:24 -0500 Subject: [PATCH 2/8] feat(quality): report the C901 complexity a PR actually caused, not the 122-finding backlog Raw C901 cannot be surfaced on a diff, and measuring it is what settles the question: all 122 findings on this tree are SINGLE-LINE regions anchored on the function-name token of the `def`. GitHub renders a finding inline only when its lines are in the diff, so the raw signal is wrong in both directions -- a PR adding branching inside an existing function moves no anchored line and produces nothing, while a PR reflowing a signature fires an annotation for debt it did not cause. So compare two ruff runs instead and report only what changed: functions this PR introduced over the threshold, or whose complexity it increased. On this tree that turns 122 findings into 0-3. Notes on the implementation: * The complexity number exists ONLY inside ruff's human-readable message, so parsing it is load-bearing. An unrecognised message raises rather than reporting zero findings -- a silent zero is indistinguishable from a clean PR and would leave the report permanently, invisibly green. * Keys are (repo-relative path, function name); ruff emits ABSOLUTE paths, and the "before" run comes from a second working tree at the merge base, so its paths carry an extra directory. If that leaked into the key nothing would ever match and all 122 functions would report as new -- the exact flood this exists to prevent. Verified end to end against two trees rooted in completely different directories, and regression-tested. * Collisions (two same-named methods in one file) take the max complexity, so the delta can under-report an increase but never invent one. Measured today: 122 findings, 122 unique keys. --- scripts/quality/c901_delta.py | 271 ++++++++++++++++++++++++++++ tests/test_c901_delta.py | 326 ++++++++++++++++++++++++++++++++++ 2 files changed, 597 insertions(+) create mode 100644 scripts/quality/c901_delta.py create mode 100644 tests/test_c901_delta.py diff --git a/scripts/quality/c901_delta.py b/scripts/quality/c901_delta.py new file mode 100644 index 00000000..ebd750dc --- /dev/null +++ b/scripts/quality/c901_delta.py @@ -0,0 +1,271 @@ +"""Report the cyclomatic-complexity (ruff C901) findings a pull request actually CAUSED. + +Raw C901 cannot be surfaced on a diff, and measuring it is what proves that: every C901 finding +anchors on a SINGLE line -- the function-name token of its `def` -- so on this tree all 122 +findings are single-line regions. GitHub renders a finding inline only when its lines are inside +the diff, which makes the raw signal wrong in both directions: a PR that adds 150 lines of +branching INSIDE an existing function touches no `def` line and produces nothing, while a PR that +reflows a signature or adds a type hint fires an annotation for debt it did not cause. + +So compare instead. This reads two ruff JSON runs -- the merge base and HEAD -- keys findings on +(repo-relative file, function name), and reports only functions this PR INTRODUCED over the +threshold or whose complexity it INCREASED. Typical output is 0-3 entries, every one attributable +to the PR under review, which also keeps the count far below any annotation ceiling. + +Output is GitHub workflow-command annotations on stdout (no token, no permission grant, identical +behaviour on fork PRs) plus a markdown table for the run summary. Decreases are reported in the +summary only, so an improvement is visible without spending an annotation on it. + +Usage: + python3 scripts/quality/c901_delta.py --base c901-base.json --head c901-head.json \ + --repo-root . --summary-file "$GITHUB_STEP_SUMMARY" +""" + +import argparse +import json +import os +import re +import sys +from typing import NamedTuple + +# ruff has NO machine-readable field for the measured complexity -- the number exists only inside the +# human-readable message, e.g. "`_serve` is too complex (95 > 10)". Parsing it is therefore load-bearing, +# and a ruff wording change MUST fail loudly rather than silently classifying every finding as absent +# (which would render this script a no-op that still reports success). See _parse_message. +_MESSAGE_RE = re.compile( + r"^`(?P[^`]+)` is too complex \((?P\d+) > (?P\d+)\)$" +) + +# Default annotation ceiling. GitHub's per-step annotation cap is undocumented (the commonly cited +# ~10 traces to a 2020 reviewdog issue citing a now-dead thread) and surplus annotations are dropped +# silently, with no error. The delta keeps real output at 0-3, so this only ever bounds a pathological +# run -- and the markdown summary is always complete regardless of what the cap drops. +_DEFAULT_MAX_ANNOTATIONS = 20 + + +class Key(NamedTuple): + """Identity of a complex function: repo-relative path plus the bare function name. + + Ruff's message carries only the bare name, not a qualified path, so two same-named methods in + different classes in one file collapse to one key. Measured on this tree today: 122 findings -> + 122 unique keys, zero collisions. A collision takes the MAX complexity (see _parse_findings), + which makes the delta conservative -- it can under-report an increase, never invent one. + """ + + path: str + function: str + + +class Finding(NamedTuple): + complexity: int + threshold: int + line: int + + +def _normalise_path(filename: str, repo_root: str, scan_root: str) -> str: + """Return `filename` as a repo-relative POSIX path. + + Ruff emits ABSOLUTE paths, and they may be Windows-style even when this script runs elsewhere + (a fixture captured on a developer machine, or a test), so os.path.relpath cannot be trusted to + cross that boundary -- normalise separators and strip textually instead. + + The base run comes from a SECOND checkout of the merge base (`base-tree/`), so its absolute + paths differ from HEAD's by more than the root. Stripping only the repo root would yield + `base-tree/messagefoundry/a.py` against HEAD's `messagefoundry/a.py`, giving every function two + non-matching keys -- every one would report as new AND removed, i.e. the exact 122-annotation + flood this script exists to prevent. So: strip the repo root, and if what remains is not already + anchored at the scanned package, cut at the last occurrence of it. + """ + normalised = filename.replace("\\", "/") + root = repo_root.replace("\\", "/").rstrip("/") + anchor = f"{scan_root}/" + + # Windows paths are case-insensitive; comparing casefolded avoids a C:/ vs c:/ miss. + if root and normalised.casefold().startswith(f"{root.casefold()}/"): + stripped = normalised[len(root) + 1 :] + if stripped.startswith(anchor): + return stripped + + # Either a different checkout root, or an intervening directory (the base tree). Cut at the LAST + # occurrence of the scanned package so a nested copy resolves to the inner one. + index = normalised.rfind(f"/{anchor}") + if index != -1: + return normalised[index + 1 :] + + # Already relative, or a shape we cannot anchor. Return it normalised rather than guessing. + return normalised.lstrip("./") + + +def _parse_message(message: str, filename: str) -> tuple[str, int, int]: + """Extract (function, complexity, threshold) from ruff's C901 message text. + + Raises ValueError on an unrecognised message. This is deliberate and tested: reporting zero + findings is indistinguishable from a clean PR, so a ruff format change must surface as a loud + traceback in the job log rather than as a silently empty -- and permanently green -- report. + """ + match = _MESSAGE_RE.match(message) + if match is None: + raise ValueError( + f"unrecognised ruff C901 message {message!r} (from {filename!r}). " + "Ruff's message format has probably changed; update _MESSAGE_RE in " + "scripts/quality/c901_delta.py and its test before trusting this report." + ) + return match["name"], int(match["actual"]), int(match["threshold"]) + + +def _parse_findings(path: str, repo_root: str, scan_root: str) -> dict[Key, Finding]: + """Load one ruff JSON run into a {Key: Finding} map.""" + with open(path, encoding="utf-8") as handle: + raw = json.load(handle) + + findings: dict[Key, Finding] = {} + for entry in raw: + if entry.get("code") != "C901": + continue + filename = entry["filename"] + function, complexity, threshold = _parse_message(entry["message"], filename) + key = Key(_normalise_path(filename, repo_root, scan_root), function) + line = int(entry["location"]["row"]) + + previous = findings.get(key) + if previous is None or complexity > previous.complexity: + findings[key] = Finding(complexity=complexity, threshold=threshold, line=line) + return findings + + +class Change(NamedTuple): + key: Key + before: int | None # None => the function is new over the threshold + after: int + threshold: int + line: int + + +def classify( + base: dict[Key, Finding], head: dict[Key, Finding] +) -> tuple[list[Change], list[Change], list[Change]]: + """Split into (new, increased, decreased). Unchanged findings are dropped entirely.""" + new: list[Change] = [] + increased: list[Change] = [] + decreased: list[Change] = [] + + for key, finding in sorted(head.items()): + previous = base.get(key) + if previous is None: + new.append(Change(key, None, finding.complexity, finding.threshold, finding.line)) + elif finding.complexity > previous.complexity: + increased.append( + Change( + key, previous.complexity, finding.complexity, finding.threshold, finding.line + ) + ) + elif finding.complexity < previous.complexity: + decreased.append( + Change( + key, previous.complexity, finding.complexity, finding.threshold, finding.line + ) + ) + return new, increased, decreased + + +def _escape_data(value: str) -> str: + return value.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + +def _escape_property(value: str) -> str: + return _escape_data(value).replace(":", "%3A").replace(",", "%2C") + + +def _annotation(change: Change) -> str: + if change.before is None: + title = "New function over the complexity threshold" + detail = ( + f"`{change.key.function}` is new at complexity {change.after} " + f"(mccabe threshold {change.threshold})" + ) + else: + title = "Complexity increased" + detail = ( + f"`{change.key.function}` complexity {change.before} -> {change.after} " + f"(mccabe threshold {change.threshold})" + ) + return ( + f"::warning file={_escape_property(change.key.path)}," + f"line={change.line},endLine={change.line}," + f"title={_escape_property(title)}::{_escape_data(detail)}" + ) + + +def _markdown(new: list[Change], increased: list[Change], decreased: list[Change]) -> str: + lines = ["## Cyclomatic complexity (C901) — changed by this PR", ""] + + if not new and not increased: + lines.append("No function was introduced over the threshold or made more complex. ✅") + else: + lines += ["| Function | File | Complexity | Threshold |", "| --- | --- | --- | --- |"] + for change in new + increased: + before = "new" if change.before is None else str(change.before) + lines.append( + f"| `{change.key.function}` | `{change.key.path}`:{change.line} " + f"| {before} → **{change.after}** | {change.threshold} |" + ) + + if decreased: + lines += ["", "### Improved (summary only, not annotated)", ""] + lines += ["| Function | File | Complexity |", "| --- | --- | --- |"] + for change in decreased: + lines.append( + f"| `{change.key.function}` | `{change.key.path}`:{change.line} " + f"| {change.before} → **{change.after}** |" + ) + + lines += [ + "", + "Advisory only — never gates a merge. Pre-existing complexity is deliberately not " + "reported: only what this PR changed appears here.", + "", + ] + return "\n".join(lines) + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--base", required=True, help="ruff C901 JSON from the merge base") + parser.add_argument("--head", required=True, help="ruff C901 JSON from HEAD") + parser.add_argument("--repo-root", default=".", help="checkout root, for relative paths") + parser.add_argument("--scan-root", default="messagefoundry", help="scanned package name") + parser.add_argument("--max-annotations", type=int, default=_DEFAULT_MAX_ANNOTATIONS) + parser.add_argument( + "--summary-file", default=None, help="append markdown here (GITHUB_STEP_SUMMARY)" + ) + args = parser.parse_args(argv) + + repo_root = os.path.abspath(args.repo_root) + base = _parse_findings(args.base, repo_root, args.scan_root) + head = _parse_findings(args.head, repo_root, args.scan_root) + new, increased, decreased = classify(base, head) + + annotated = new + increased + for change in annotated[: args.max_annotations]: + print(_annotation(change)) + if len(annotated) > args.max_annotations: + dropped = len(annotated) - args.max_annotations + print( + f"::notice title=Complexity delta truncated::{dropped} further change(s) in the summary" + ) + + markdown = _markdown(new, increased, decreased) + if args.summary_file: + with open(args.summary_file, "a", encoding="utf-8") as handle: + handle.write(markdown) + else: + sys.stderr.write(markdown) + + # Always 0. This is an advisory reporter; a non-zero exit here would be a merge-blocking signal + # in disguise. Genuine breakage (an unparseable ruff message) raises instead, which is visible + # in the log without reddening the job. + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_c901_delta.py b/tests/test_c901_delta.py new file mode 100644 index 00000000..313a4615 --- /dev/null +++ b/tests/test_c901_delta.py @@ -0,0 +1,326 @@ +"""Tests for scripts/quality/c901_delta.py -- the PR-caused complexity reporter. + +The script exists because raw C901 is unshippable as a diff signal (all 122 findings on this tree +anchor on a single `def` line). Its whole value is that it reports ONLY what a PR changed, so the +tests that matter are: pre-existing findings never appear, and a ruff wording change fails loudly +instead of quietly reporting "nothing changed" forever. +""" + +import importlib.util +import json +import sys +from pathlib import Path +from types import ModuleType + +import pytest + +_ROOT = Path(__file__).resolve().parents[1] + + +def _load() -> ModuleType: + path = _ROOT / "scripts" / "quality" / "c901_delta.py" + assert path.is_file(), f"delta script not found at {path}" + spec = importlib.util.spec_from_file_location("c901_delta", path) + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = mod + spec.loader.exec_module(mod) + return mod + + +delta = _load() + + +def _finding(filename: str, function: str, complexity: int, row: int, threshold: int = 10) -> dict: + """One ruff C901 result, in ruff's real JSON shape (captured from ruff 0.15.22).""" + return { + "code": "C901", + "message": f"`{function}` is too complex ({complexity} > {threshold})", + "filename": filename, + "location": {"column": 5, "row": row}, + "end_location": {"column": 5 + len(function), "row": row}, + "fix": None, + "url": "https://docs.astral.sh/ruff/rules/complex-structure", + } + + +def _write(tmp_path: Path, name: str, findings: list[dict]) -> str: + path = tmp_path / name + path.write_text(json.dumps(findings), encoding="utf-8") + return str(path) + + +# -------------------------------------------------------------------------------------------- +# Classification: the core promise -- unchanged pre-existing debt must never be reported. +# -------------------------------------------------------------------------------------------- + + +def _parse(tmp_path: Path, name: str, findings: list[dict], root: str = "/repo") -> dict: + return delta._parse_findings(_write(tmp_path, name, findings), root, "messagefoundry") + + +def test_unchanged_findings_are_never_reported(tmp_path: Path) -> None: + """The single most important assertion: 122 untouched findings produce zero output.""" + same = [_finding("/repo/messagefoundry/a.py", "f", 30, 10)] + base = _parse(tmp_path, "base.json", same) + head = _parse(tmp_path, "head.json", same) + + new, increased, decreased = delta.classify(base, head) + + assert new == [] + assert increased == [] + assert decreased == [] + + +def test_new_function_over_threshold_is_reported(tmp_path: Path) -> None: + base = _parse(tmp_path, "base.json", []) + head = _parse(tmp_path, "head.json", [_finding("/repo/messagefoundry/a.py", "fresh", 14, 42)]) + + new, increased, decreased = delta.classify(base, head) + + assert [c.key.function for c in new] == ["fresh"] + assert new[0].before is None + assert new[0].after == 14 + assert new[0].line == 42 + assert increased == [] and decreased == [] + + +def test_increased_complexity_is_reported_with_both_numbers(tmp_path: Path) -> None: + """The body-edit case: the `def` line never moved, but complexity rose. This is the case + SARIF/code-scanning cannot see at all, and the reason the delta exists.""" + base = _parse(tmp_path, "base.json", [_finding("/repo/messagefoundry/a.py", "grow", 12, 7)]) + head = _parse(tmp_path, "head.json", [_finding("/repo/messagefoundry/a.py", "grow", 27, 7)]) + + new, increased, decreased = delta.classify(base, head) + + assert new == [] and decreased == [] + assert len(increased) == 1 + assert (increased[0].before, increased[0].after) == (12, 27) + + +def test_decreased_complexity_is_summary_only(tmp_path: Path) -> None: + base = _parse(tmp_path, "base.json", [_finding("/repo/messagefoundry/a.py", "shrink", 40, 3)]) + head = _parse(tmp_path, "head.json", [_finding("/repo/messagefoundry/a.py", "shrink", 11, 3)]) + + new, increased, decreased = delta.classify(base, head) + + assert new == [] and increased == [] + assert len(decreased) == 1 + markdown = delta._markdown(new, increased, decreased) + assert "Improved" in markdown + assert "shrink" in markdown + + +def test_a_function_dropping_below_threshold_is_not_a_regression(tmp_path: Path) -> None: + """Fixed entirely: it leaves ruff's output, so it must not read as new/increased.""" + base = _parse(tmp_path, "base.json", [_finding("/repo/messagefoundry/a.py", "fixed", 12, 5)]) + head = _parse(tmp_path, "head.json", []) + + new, increased, decreased = delta.classify(base, head) + + assert new == [] and increased == [] and decreased == [] + + +# -------------------------------------------------------------------------------------------- +# Path normalisation -- ruff emits ABSOLUTE paths; annotations need repo-relative POSIX ones. +# -------------------------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("filename", "repo_root", "expected"), + [ + ("/repo/messagefoundry/api/app.py", "/repo", "messagefoundry/api/app.py"), + (r"C:\co\messagefoundry\api\app.py", r"C:\co", "messagefoundry/api/app.py"), + # Casefolded compare: a drive letter differing only in case must still strip. + (r"c:\co\messagefoundry\a.py", r"C:\co", "messagefoundry/a.py"), + # JSON captured from a DIFFERENT checkout: fall back to the scan-root segment. + (r"D:\other\tree\messagefoundry\a.py", "/repo", "messagefoundry/a.py"), + ("messagefoundry/a.py", "/repo", "messagefoundry/a.py"), + # THE BASE-TREE CASE. The merge-base run happens in a second checkout under the repo root, + # so its paths carry an extra segment. If this did not collapse to the same key as HEAD, + # every function would report as new -- a 122-annotation flood on the very first PR. + ( + "/home/runner/work/r/r/base-tree/messagefoundry/a.py", + "/home/runner/work/r/r", + "messagefoundry/a.py", + ), + # A repo root that itself ends in the scanned package name must not confuse the anchor. + ("/src/messagefoundry/messagefoundry/a.py", "/src/messagefoundry", "messagefoundry/a.py"), + ], +) +def test_paths_normalise_to_repo_relative_posix( + filename: str, repo_root: str, expected: str +) -> None: + assert delta._normalise_path(filename, repo_root, "messagefoundry") == expected + + +def test_base_tree_and_head_paths_produce_the_SAME_key(tmp_path: Path) -> None: + """Regression guard for the flood bug: the merge-base checkout lives one directory deeper, and + if that leaks into the key then nothing ever matches and every finding reads as new.""" + root = "/home/runner/work/r/r" + head = _parse( + tmp_path, "head.json", [_finding(f"{root}/messagefoundry/a.py", "f", 30, 10)], root + ) + base = _parse( + tmp_path, + "base.json", + [_finding(f"{root}/base-tree/messagefoundry/a.py", "f", 30, 10)], + root, + ) + + assert set(head) == set(base) + assert delta.classify(base, head) == ([], [], []) + + +def test_annotation_carries_a_relative_path_and_the_def_line(tmp_path: Path) -> None: + base = _parse(tmp_path, "base.json", []) + head = _parse( + tmp_path, "head.json", [_finding(r"C:\repo\messagefoundry\x.py", "g", 13, 99)], "C:/repo" + ) + new, _, _ = delta.classify(base, head) + + line = delta._annotation(new[0]) + + assert line.startswith("::warning file=messagefoundry/x.py,") + assert "line=99,endLine=99" in line + assert "C:" not in line # an absolute local path must never reach the annotation + assert "13" in line + + +# -------------------------------------------------------------------------------------------- +# Fail-loud parsing -- a silent zero-finding report is indistinguishable from a clean PR. +# -------------------------------------------------------------------------------------------- + + +def test_unparseable_message_raises_rather_than_reporting_nothing(tmp_path: Path) -> None: + broken = [ + { + "code": "C901", + "message": "`f` has a cyclomatic complexity of 30, over the limit of 10", + "filename": "/repo/messagefoundry/a.py", + "location": {"column": 5, "row": 1}, + "end_location": {"column": 6, "row": 1}, + } + ] + with pytest.raises(ValueError, match="unrecognised ruff C901 message"): + _parse(tmp_path, "head.json", broken) + + +def test_non_c901_results_are_ignored(tmp_path: Path) -> None: + mixed = [ + { + "code": "E501", + "message": "Line too long", + "filename": "/repo/messagefoundry/a.py", + "location": {"column": 1, "row": 1}, + "end_location": {"column": 2, "row": 1}, + }, + _finding("/repo/messagefoundry/a.py", "only", 12, 4), + ] + parsed = _parse(tmp_path, "head.json", mixed) + assert [k.function for k in parsed] == ["only"] + + +def test_key_collision_takes_the_max_complexity(tmp_path: Path) -> None: + """Two same-named methods in one file share a key. Taking the max keeps the delta + conservative -- it can under-report an increase, never invent one.""" + collide = [ + _finding("/repo/messagefoundry/a.py", "run", 12, 10), + _finding("/repo/messagefoundry/a.py", "run", 40, 80), + ] + parsed = _parse(tmp_path, "head.json", collide) + assert len(parsed) == 1 + assert next(iter(parsed.values())).complexity == 40 + + +# -------------------------------------------------------------------------------------------- +# Annotation cap + end-to-end main() +# -------------------------------------------------------------------------------------------- + + +def test_annotations_are_capped_and_the_remainder_is_announced( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + head = [_finding(f"/repo/messagefoundry/f{i}.py", f"fn{i}", 12, i + 1) for i in range(25)] + summary = tmp_path / "summary.md" + + rc = delta.main( + [ + "--base", + _write(tmp_path, "base.json", []), + "--head", + _write(tmp_path, "head.json", head), + "--repo-root", + "/repo", + "--max-annotations", + "20", + "--summary-file", + str(summary), + ] + ) + out = capsys.readouterr().out + + assert rc == 0 + assert out.count("::warning ") == 20 + assert "::notice title=Complexity delta truncated::5 further change(s)" in out + # The summary is ALWAYS complete, even when annotations are dropped. + assert summary.read_text(encoding="utf-8").count("| `fn") == 25 + + +def test_main_reports_clean_when_nothing_changed( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + same = [_finding("/repo/messagefoundry/a.py", "f", 30, 10)] + summary = tmp_path / "summary.md" + + rc = delta.main( + [ + "--base", + _write(tmp_path, "base.json", same), + "--head", + _write(tmp_path, "head.json", same), + "--repo-root", + "/repo", + "--summary-file", + str(summary), + ] + ) + + assert rc == 0 + assert "::warning" not in capsys.readouterr().out + assert "No function was introduced over the threshold" in summary.read_text(encoding="utf-8") + + +def test_main_appends_rather_than_truncating_the_summary(tmp_path: Path) -> None: + """GITHUB_STEP_SUMMARY accumulates across steps; overwriting would eat another step's output.""" + summary = tmp_path / "summary.md" + summary.write_text("## existing content\n", encoding="utf-8") + + delta.main( + [ + "--base", + _write(tmp_path, "base.json", []), + "--head", + _write(tmp_path, "head.json", []), + "--repo-root", + "/repo", + "--summary-file", + str(summary), + ] + ) + + assert "## existing content" in summary.read_text(encoding="utf-8") + + +def test_workflow_command_metacharacters_are_escaped() -> None: + change = delta.Change( + key=delta.Key(path="messagefoundry/a,b.py", function="weird%name"), + before=1, + after=2, + threshold=10, + line=3, + ) + line = delta._annotation(change) + + assert "file=messagefoundry/a%2Cb.py" in line + assert "weird%25name" in line From 49b55190a89b90171aa386be0bf2d1bf7ee352e7 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 14:16:50 -0500 Subject: [PATCH 3/8] feat(ci): surface the advisory quality signals in the PR review UI, with no write scope The four advisory quality jobs computed real signals and then printed them to a log nobody opens. Wire each one to a reviewer, using GitHub workflow-command annotations rather than code scanning. Why annotations and not SARIF -- measured, and recorded in the workflow header so a future session does not "helpfully" add an upload back: 1. All 122 C901 findings anchor on a single `def` line, so body edits surface nothing and signature edits surface pre-existing debt. 2. jscpd emits ONE location per clone pair, chosen by scan order, so ~8 files can never be the anchor and about half of newly-introduced clones would anchor on the untouched twin. 3. Uploading on pull_request only never builds a default-branch baseline, so every PR forever would report all ~161 findings as new -- not a one-off first-PR blip. Workflow commands need no token and no permission grant and behave identically on fork PRs, so every job keeps `contents: read` and the workflow grants NO write scope anywhere. That, rather than mere absence from branch protection, is what makes it structurally unable to gate a merge. Per signal: * diff-coverage -> INLINE `::notice` annotations on the Files changed tab. Verified against a real run: adjacent uncovered lines coalesce into ranges (lines 2,3,4 become one annotation), paths are relative, the console report still prints, exit 0. * complexity -> the merge-base delta, PR-gated (there is no base ref on the cron). * clones -> markdown step summary; annotating it would be a coin flip (see 2 above). * mutation -> step summary + artifact. The job never runs on PRs, so it has no PR surface; saying so plainly beats inventing one. Also fixed while here, each a silent failure: * `.mutmut-cache` is a DOTFILE and upload-artifact skips hidden files by default -- the upload would have logged "No files were found", uploaded nothing, and reported success. * diff-cover and pytest-cov were installed under a `--constraint constraints.lock` that contains neither, so both floated to whatever PyPI served that day. diff-cover is now pinned exactly, because the annotation surface depends on this version's `--format` semantics. * the job pinned ruff 0.15.19 while all four locks pin 0.15.22. That drift is now load-bearing (the delta parses ruff's message text), so it is aligned and asserted against the lock. Every summary write is truncated: $GITHUB_STEP_SUMMARY is capped at 1 MiB per step and an oversized write is dropped ENTIRELY, which would silently lose the whole surface. tests/test_quality_advisory_invariants.py pins all of it -- no write scope, no SARIF, no pull_request_target, no expression interpolation into a run body, SHA-pinned actions, the tool pins, and every analysis step non-failing. Each assertion was negative-probed against a mutated copy of the workflow; all 17 mutations were caught. --- .github/workflows/quality-advisory.yml | 141 +++++++++++- tests/test_quality_advisory_invariants.py | 247 ++++++++++++++++++++++ 2 files changed, 378 insertions(+), 10 deletions(-) create mode 100644 tests/test_quality_advisory_invariants.py diff --git a/.github/workflows/quality-advisory.yml b/.github/workflows/quality-advisory.yml index 18be46f6..90691135 100644 --- a/.github/workflows/quality-advisory.yml +++ b/.github/workflows/quality-advisory.yml @@ -7,12 +7,31 @@ name: quality-advisory # raw complexity, coverage %, and clone counts are weak / gameable signals (rubric section 4.1) and must # never be a hard gate. This workflow is NOT in branch protection; adding a job here cannot freeze the repo. # -# Signals from the rubric (renumbered by tier -- docs/Code_Quality_Standards.md v0.7): -# * signal 11 - complexity triage (ruff C901) -> BUILT (complexity job) -# * signal 9 - clone / duplication (jscpd) -> BUILT (clone job) -# * signal 8 - diff-coverage visibility -> BUILT (coverage job; PR-only) -# * signal 7 - mutation testing (highest leverage) -> BUILT (mutation job; mirror-nightly + workflow_dispatch) +# Signals from the rubric (renumbered by tier -- docs/Code_Quality_Standards.md v0.7), each with how +# it reaches a reviewer: +# * signal 11 - complexity triage (ruff C901) -> BUILT (complexity job); PR-caused DELTA as +# annotations + step summary, full list in the log +# * signal 9 - clone / duplication (jscpd) -> BUILT (clone job); step summary only, see below +# * signal 8 - diff-coverage visibility -> BUILT (coverage job; PR-only); INLINE `::notice` +# annotations on the Files changed tab + summary +# * signal 7 - mutation testing (highest leverage) -> BUILT (mutation job; mirror-nightly + workflow_dispatch); +# step summary + artifact, no PR surface (never runs on PRs) # signal 10 (lint breadth) is a coordinated one-shot sweep, not an advisory job; DORA is a context caveat, not a gate. +# +# WHY THIS USES WORKFLOW-COMMAND ANNOTATIONS AND *NOT* CODE SCANNING / SARIF. This was measured, not +# assumed, and the measurements are recorded here so a future session does not "helpfully" add a SARIF +# upload back: +# 1. All 122 C901 findings on this tree are SINGLE-LINE regions anchored on the function-name token of +# the `def`. GitHub renders an alert inline only when its lines are in the diff, so a PR adding +# branching INSIDE a function surfaces nothing, while a signature reflow fires on pre-existing debt. +# 2. jscpd emits ONE location per clone pair, chosen by scan order, so ~8 files can never be the anchor +# and roughly half of newly-introduced clones would anchor on the untouched twin. +# 3. Uploading on pull_request only leaves NO default-branch baseline, so every PR forever would report +# all ~161 findings as new -- not a one-off first-PR blip. +# Workflow commands need no token and no permission grant, and behave identically on fork PRs. Every job +# below therefore holds `contents: read` and nothing else; the workflow grants NO write scope anywhere, +# which is what actually guarantees it cannot gate a merge. tests/test_quality_advisory_invariants.py +# pins that, and pins the absence of any SARIF upload. on: pull_request: @@ -40,15 +59,48 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + # The delta step needs the base branch's history for `git merge-base`. + fetch-depth: 0 - name: Install ruff (pinned) - run: pipx install ruff==0.15.19 + # Held to the SAME version as constraints.lock / requirements.lock / uv.lock (previously 0.15.19 + # against their 0.15.22 -- unguarded drift). This is now load-bearing: c901_delta.py parses ruff's + # human-readable C901 message because ruff exposes the complexity number nowhere else, so a skew + # can change the wording under the parser. Nothing automates this -- DEP-1 byte-diffs the exported + # locks and cannot see a version inside a `run:` body. + run: pipx install ruff==0.15.22 - name: Ruff C901 complexity triage (advisory - never gates) + continue-on-error: true run: | echo "== functions over mccabe complexity 10 (advisory triage; NEVER a merge gate) ==" ruff check --select C901 --output-format=concise --exit-zero messagefoundry echo "" echo "== summary ==" ruff check --select C901 --statistics --exit-zero messagefoundry + - name: Complexity delta vs the merge base (advisory - never gates) + # PR-only: there is no base ref to diff against on the cron or on workflow_dispatch, where the + # whole-repo console triage above remains the full picture. + if: github.event_name == 'pull_request' + continue-on-error: true + env: + BASE_REF: ${{ github.base_ref }} + run: | + echo "== complexity this PR CHANGED (advisory; pre-existing findings deliberately omitted) ==" + git fetch --no-tags origin "$BASE_REF" || true + MERGE_BASE="$(git merge-base HEAD "origin/$BASE_REF")" + echo "merge base: $MERGE_BASE" + ruff check --select C901 --output-format=json --exit-zero messagefoundry > c901-head.json + # A second checkout of the merge base, so the "before" numbers come from real ruff output + # rather than a reconstruction. Its absolute paths differ from HEAD's by the extra directory; + # c901_delta.py collapses both to repo-relative keys (regression-tested -- if it did not, + # every function would report as new). + git worktree add --detach base-tree "$MERGE_BASE" + ( cd base-tree && ruff check --select C901 --output-format=json --exit-zero messagefoundry ) \ + > c901-base.json + python3 scripts/quality/c901_delta.py \ + --base c901-base.json \ + --head c901-head.json \ + --repo-root . \ + --summary-file "$GITHUB_STEP_SUMMARY" clone: # Signal 9 - duplication / clone detection. Flags copy-pasted blocks (the "copy-instead-of-abstract" @@ -65,13 +117,32 @@ jobs: with: persist-credentials: false - name: jscpd clone report (advisory - never gates) + continue-on-error: true run: | echo "== copy-pasted blocks in messagefoundry/ (advisory; justified store-backend parity ignored) ==" + # Summary only, DELIBERATELY. jscpd ships a SARIF reporter and it works, but each result + # carries exactly ONE location -- whichever file jscpd scanned first -- so the case this + # signal exists for (a PR copy-pasting an existing block into a new place) anchors on the + # untouched twin about half the time. That is a coin flip, not a diff-scoped signal. + # Keep the 4.x pin: npm `latest` is a 5.x Rust rewrite with a different CLI. npx --yes jscpd@4.0.5 messagefoundry \ --min-tokens 60 \ --ignore "**/store/sqlserver.py,**/store/postgres.py" \ - --reporters console \ + --reporters console,markdown \ + --output ./jscpd-report \ || true + if [ -f jscpd-report/jscpd-report.md ]; then + { + echo "## Duplicate blocks (jscpd)" + echo "" + # $GITHUB_STEP_SUMMARY is capped at 1 MiB per step and an oversized write is DROPPED + # ENTIRELY, so truncate rather than silently lose the whole summary. + head -c 900000 jscpd-report/jscpd-report.md + echo "" + echo "Advisory only. Clone locations are in the job log — see the note in the" + echo "workflow for why these are not annotated on the diff." + } >> "$GITHUB_STEP_SUMMARY" + fi coverage: # Signal 8 - diff-coverage visibility. Runs the suite under coverage, then reports coverage of the @@ -104,18 +175,42 @@ jobs: - name: Install project + coverage tools run: | uv pip install --system --constraint constraints.lock -e ".[dev,harness,fhir,dicom,x12,xml,webauthn]" -e packaging/messagefoundry-webconsole - uv pip install --system --constraint constraints.lock pytest-cov diff-cover + # NEITHER package is in constraints.lock, so the --constraint above is a no-op for them and both + # previously floated to whatever PyPI served that day. diff-cover is now pinned exactly: the + # inline-annotation surface depends on `--format github-annotations:` and on adjacent-line + # coalescing in GitHubAnnotationsReportGenerator. Deliberately NOT in pyproject.toml -- these are + # CI-only tools, and adding them would trip the DEP-1 lock-sync gate and force four re-exports. + uv pip install --system --constraint constraints.lock pytest-cov "diff-cover==10.4.1" - name: Tests under coverage (advisory - never fails) + continue-on-error: true env: QT_QPA_PLATFORM: offscreen run: pytest -q --cov=messagefoundry --cov-report=xml --cov-report=term-missing:skip-covered || true - name: Diff-coverage vs the PR base (advisory - never fails) + continue-on-error: true env: BASE_REF: ${{ github.base_ref }} run: | echo "== coverage of the lines this PR changed (advisory; not a whole-repo % gate) ==" git fetch --no-tags --depth=1 origin "$BASE_REF" || true - diff-cover coverage.xml --compare-branch="origin/$BASE_REF" --fail-under=0 || true + # `github-annotations:notice` writes `::notice file=...,line=N,endLine=M` workflow commands to + # STDOUT, which the runner turns into INLINE annotations on the Files changed tab -- no token, + # no permission grant, identical behaviour on fork PRs. Adjacent uncovered lines are coalesced + # into ranges, so a long uncovered block costs one annotation rather than one per line. Every + # line flagged is a line this PR changed, so there is no pre-existing-debt noise by construction. + # The console report still prints, so the job log is unchanged. + diff-cover coverage.xml --compare-branch="origin/$BASE_REF" --fail-under=0 \ + --format "github-annotations:notice,markdown:diff-cover.md" || true + if [ -f diff-cover.md ]; then + { + echo "## Diff coverage" + echo "" + # The markdown report embeds a source snippet per uncovered line, so it can get large. + # $GITHUB_STEP_SUMMARY is capped at 1 MiB per step and an oversized write is DROPPED + # ENTIRELY -- truncate rather than silently lose the whole summary. + head -c 900000 diff-cover.md + } >> "$GITHUB_STEP_SUMMARY" + fi mutation: # Signal 7 - mutation testing (advisory; the HIGHEST-leverage gate -- it adversarially checks the @@ -150,6 +245,7 @@ jobs: uv pip install --system --constraint constraints.lock -e ".[dev,harness,fhir,dicom,x12,xml,webauthn]" -e packaging/messagefoundry-webconsole uv pip install --system --constraint constraints.lock "mutmut<3" - name: Mutation-test a bounded scope (advisory - never fails) + continue-on-error: true env: QT_QPA_PLATFORM: offscreen run: | @@ -159,4 +255,29 @@ jobs: printf '[mutmut]\npaths_to_mutate=messagefoundry/parsing/binary.py\nrunner=python -m pytest -x -q tests/test_binary_carriage.py\n' > setup.cfg mutmut run || true echo "== results ==" - mutmut results || true + mutmut results 2>&1 | tee mutmut-results.txt || true + { + echo "## Mutation testing (bounded scope: messagefoundry/parsing/binary.py)" + echo "" + echo '```' + head -c 900000 mutmut-results.txt + echo '```' + echo "" + echo "Advisory only. This job never runs on pull requests (cron on the public repo +" + echo "workflow_dispatch), so it has no PR surface by design — surviving mutants are reviewed" + echo "from this run page." + } >> "$GITHUB_STEP_SUMMARY" + - name: Upload the mutmut cache (advisory) + if: always() + continue-on-error: true + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: mutmut-cache + path: .mutmut-cache + # .mutmut-cache is a DOTFILE and upload-artifact skips hidden files by DEFAULT. Without this + # the step logs "No files were found", uploads nothing, and still reports success -- a silent + # no-op that looks green. + include-hidden-files: true + # Deliberately `warn`, not `ignore`: if mutmut ever renames its cache, that must be visible in + # the log rather than silently swallowed. + if-no-files-found: warn diff --git a/tests/test_quality_advisory_invariants.py b/tests/test_quality_advisory_invariants.py new file mode 100644 index 00000000..5a9a29bf --- /dev/null +++ b/tests/test_quality_advisory_invariants.py @@ -0,0 +1,247 @@ +"""Pin the advisory guarantees of .github/workflows/quality-advisory.yml. + +Every safety property of that workflow is a string in a YAML file: one edit can grant a write +scope, drop an `--exit-zero`, or add a SARIF upload, and nothing else in the repo would notice. +This repo already parses workflow YAML in tests for exactly that reason (see +test_dependabot_automerge_guardrails.py, test_release_pipeline.py, test_lint_scope_parity.py). + +The single strongest assertion here is that the workflow holds NO write permission on any job. +That -- not "it is absent from branch protection" -- is what makes it structurally incapable of +gating a merge, and it is only assertable because these jobs surface findings via workflow-command +annotations rather than by uploading SARIF to code scanning. +""" + +import re +from pathlib import Path + +import pytest +import yaml + +_WORKFLOW = Path(__file__).resolve().parents[1] / ".github" / "workflows" / "quality-advisory.yml" + +# Steps that actually run a quality tool. Setup steps (checkout, setup-python, apt-get, the tool +# installs) are deliberately NOT required to be continue-on-error: masking an infrastructure failure +# would produce confusing downstream errors, and the merge-blocking guarantee comes from the absent +# write scopes plus the workflow's absence from branch protection, not from those steps. +_ANALYSIS_MARKERS = ( + "ruff check", + "npx --yes jscpd", + "diff-cover coverage.xml", + "mutmut run", + "mutmut results", + "c901_delta.py", + "pytest -q --cov", +) + +# An analysis command must be incapable of failing its step. +_NON_FAILING_IDIOMS = ("--exit-zero", "--fail-under=0", "|| true") + +_SHA_PINNED = re.compile(r"^[^@]+@[0-9a-f]{40}$") + + +@pytest.fixture(scope="module") +def workflow() -> dict: + assert _WORKFLOW.is_file(), f"workflow not found at {_WORKFLOW}" + return yaml.safe_load(_WORKFLOW.read_text(encoding="utf-8")) + + +@pytest.fixture(scope="module") +def raw() -> str: + return _WORKFLOW.read_text(encoding="utf-8") + + +def _strip_comments(text: str) -> str: + """Drop YAML comments. + + The header deliberately EXPLAINS why there is no SARIF upload, so a naive substring search for + "sarif" matches the very comment that documents its absence. Assert against code instead. + """ + out = [] + for line in text.splitlines(): + stripped = line.lstrip() + if stripped.startswith("#"): + continue + out.append(line.split(" #", 1)[0] if " #" in line else line) + return "\n".join(out) + + +@pytest.fixture(scope="module") +def code(raw: str) -> str: + return _strip_comments(raw) + + +def _steps(workflow: dict) -> list[tuple[str, dict]]: + return [(name, step) for name, job in workflow["jobs"].items() for step in job["steps"]] + + +def _analysis_steps(workflow: dict) -> list[tuple[str, dict]]: + return [ + (job, step) + for job, step in _steps(workflow) + if any(marker in (step.get("run") or "") for marker in _ANALYSIS_MARKERS) + ] + + +# -------------------------------------------------------------------------------------------- +# The advisory guarantee. +# -------------------------------------------------------------------------------------------- + + +def test_workflow_grants_no_permissions_by_default(workflow: dict) -> None: + assert workflow["permissions"] == {}, "workflow-level permissions must stay deny-by-default" + + +def test_no_job_holds_any_write_scope(workflow: dict) -> None: + """The load-bearing assertion. A write scope anywhere here is a merge-gating capability in + disguise, and both the clone and complexity jobs execute third-party code fetched at run time + (`npx --yes jscpd`, `pipx install ruff`) with no integrity pin -- handing those a write-scoped + repository token would be a least-privilege regression.""" + for name, job in workflow["jobs"].items(): + permissions = job.get("permissions") + assert permissions is not None, f"job {name!r} must declare explicit permissions" + for scope, level in permissions.items(): + assert level == "read", f"job {name!r} grants {scope}: {level} -- must be read" + + +def test_no_sarif_upload_and_no_security_events(code: str) -> None: + """Measured, not stylistic: all 122 C901 findings anchor on a single `def` line, jscpd emits one + scan-order location per clone pair, and a PR-only upload never builds a baseline -- so every PR + would report every finding as new, forever. See the workflow header for the full reasoning + (which is why this asserts against comment-stripped code, not the raw text).""" + assert "sarif" not in code.lower(), ( + "no SARIF surface in this workflow -- see the header comment" + ) + assert "security-events" not in code + + +def test_no_pull_request_target(raw: str) -> None: + """pull_request_target grants a read/write token even from a public fork.""" + assert "pull_request_target" not in raw + + +def test_every_analysis_step_cannot_fail_its_job(workflow: dict) -> None: + steps = _analysis_steps(workflow) + assert len(steps) >= 6, f"expected the quality tool steps to be found, got {len(steps)}" + for job, step in steps: + name = step.get("name", "") + assert step.get("continue-on-error") is True, ( + f"{job}/{name!r} runs a quality tool without continue-on-error: true" + ) + body = step["run"] + assert any(idiom in body for idiom in _NON_FAILING_IDIOMS), ( + f"{job}/{name!r} has no non-failing idiom ({_NON_FAILING_IDIOMS})" + ) + + +# -------------------------------------------------------------------------------------------- +# Supply chain. +# -------------------------------------------------------------------------------------------- + + +def test_every_action_is_sha_pinned_with_a_version_comment(workflow: dict, raw: str) -> None: + uses = [step["uses"] for _, step in _steps(workflow) if "uses" in step] + assert uses, "expected at least one action" + for ref in uses: + assert _SHA_PINNED.match(ref), f"{ref} is not pinned to a 40-hex SHA" + for line in raw.splitlines(): + if "uses:" in line: + assert re.search(r"#\s*v", line), f"missing version comment: {line.strip()}" + + +def test_jscpd_stays_on_4x(raw: str) -> None: + """npm `latest` is a 5.x Rust rewrite shipped as platform binaries with a different CLI.""" + assert re.search(r"jscpd@4\.\d+\.\d+", raw), "jscpd must stay pinned to a 4.x release" + + +def test_diff_cover_is_pinned_exactly(raw: str) -> None: + """The annotation surface depends on this version's `--format github-annotations:`.""" + assert re.search(r'"diff-cover==\d+\.\d+\.\d+"', raw), "diff-cover must be pinned with ==" + + +def test_ruff_pin_matches_the_lock(raw: str) -> None: + """The delta script parses ruff's human-readable C901 message, so a version skew can change the + wording under the parser. DEP-1 byte-diffs the exported locks and cannot see this one.""" + match = re.search(r"pipx install ruff==(\S+)", raw) + assert match, "the complexity job must pin ruff" + workflow_pin = match.group(1) + + lock = (_WORKFLOW.parents[2] / "constraints.lock").read_text(encoding="utf-8") + locked = re.search(r"^ruff==(\S+?)(?:\s|$)", lock, re.MULTILINE) + assert locked, "ruff not found in constraints.lock" + assert workflow_pin == locked.group(1), ( + f"workflow pins ruff=={workflow_pin} but constraints.lock has {locked.group(1)}" + ) + + +# -------------------------------------------------------------------------------------------- +# The surfacing mechanisms themselves. +# -------------------------------------------------------------------------------------------- + + +def test_diff_coverage_emits_inline_github_annotations(code: str) -> None: + assert "github-annotations:" in code, "diff-cover must emit inline annotations" + + +def test_the_complexity_delta_is_wired_and_pr_gated(workflow: dict) -> None: + steps = [s for job, s in _steps(workflow) if "c901_delta.py" in (s.get("run") or "")] + assert len(steps) == 1, "expected exactly one complexity delta step" + assert steps[0].get("if") == "github.event_name == 'pull_request'", ( + "the delta needs a base ref; it must not run on the cron or workflow_dispatch" + ) + assert "--summary-file" in steps[0]["run"] + + +def test_the_delta_script_exists(workflow: dict) -> None: + script = _WORKFLOW.parents[2] / "scripts" / "quality" / "c901_delta.py" + assert script.is_file(), "the complexity job references a script that is not in the repo" + + +def test_the_complexity_job_fetches_full_history(workflow: dict) -> None: + """`git merge-base` cannot work against a shallow clone.""" + checkout = next( + step + for job, step in _steps(workflow) + if job == "complexity" and "checkout" in (step.get("uses") or "") + ) + assert checkout.get("with", {}).get("fetch-depth") == 0, ( + "the complexity job needs full history -- git merge-base cannot work on a shallow clone" + ) + + +def test_checkouts_do_not_persist_credentials(workflow: dict) -> None: + """quality-advisory.yml is not in .github/zizmor.yml's artipacked ignore list.""" + for job, step in _steps(workflow): + if "checkout" in (step.get("uses") or ""): + assert step.get("with", {}).get("persist-credentials") is False, ( + f"{job} checkout must set persist-credentials: false" + ) + + +def test_step_summary_writes_are_size_guarded(raw: str) -> None: + """An oversized $GITHUB_STEP_SUMMARY write is dropped ENTIRELY, losing the whole surface.""" + appends = raw.count('>> "$GITHUB_STEP_SUMMARY"') + assert appends >= 3, f"expected the summary blocks to be present, found {appends}" + assert raw.count("head -c 900000") >= 3, "each summary block must be truncated" + + +def test_mutmut_artifact_includes_hidden_files(workflow: dict) -> None: + """.mutmut-cache is a dotfile and upload-artifact skips hidden files by default -- without this + the step logs 'No files were found', uploads nothing, and still reports success.""" + upload = next( + step for _, step in _steps(workflow) if "upload-artifact" in (step.get("uses") or "") + ) + with_ = upload.get("with", {}) + assert with_.get("include-hidden-files") is True, ( + "the mutmut cache is a dotfile; without include-hidden-files this step uploads nothing " + "and still reports success" + ) + assert with_.get("if-no-files-found") == "warn" + + +def test_no_expression_interpolation_inside_run_bodies(workflow: dict) -> None: + """Template injection: untrusted `${{ }}` expanded into a shell body. Values must be routed + through `env:` instead (zizmor enforces this in CI; assert it here too).""" + for job, step in _steps(workflow): + body = step.get("run") + if body: + assert "${{" not in body, f"{job}/{step.get('name')!r} interpolates into a run body" From 55f93cf74e66f409510204cf2abc90b7b8c58c46 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 14:17:06 -0500 Subject: [PATCH 4/8] docs(ci): document the advisory quality workflow and how each signal reaches a reviewer docs/CI.md's workflow table omitted quality-advisory.yml entirely. Add it, plus a table of where each of the four signals shows up, and record the never-require rule with the real guarantee: the workflow holds no write permission on any job, so it cannot gate a merge even if branch protection or a ruleset changes. That is stronger than "branch protection is untouched" and is test-pinned. Also update the mutation/coverage handoff doc: the jobs now emit annotations and summaries, and mutation deliberately has no PR surface. Its section 5 ("flip the rubric") is marked UNACTIONABLE -- docs/Code_Quality_Standards.md is cited by the workflow header and pyproject.toml but has never existed in this repo's history, so there is no rubric file to flip. Flagged rather than papered over, and no new reference to it is added. Left open pending a measurement: whether mutation should run on PRs. Its wall-clock is unknown, so the honest next step is one workflow_dispatch run and a look at the duration, not a guess. --- docs/CI.md | 21 +++++++++++++++ .../HANDOFF-mutation-coverage.md | 27 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/docs/CI.md b/docs/CI.md index 29cffe2e..cb159987 100644 --- a/docs/CI.md +++ b/docs/CI.md @@ -15,6 +15,7 @@ which checks are *required* — this page describes the intended layout. | `scorecard.yml` | OpenSSF Scorecard analysis. | | `cla.yml` | CLA Assistant — records the Contributor License Agreement signature on each PR. | | `zizmor.yml` | Lints the workflow files themselves for insecure patterns (template injection, over-broad tokens). **Blocking.** | +| `quality-advisory.yml` | Advisory quality measurement — complexity (ruff `C901`), duplication (`jscpd`), diff-coverage (`diff-cover`) and mutation testing (`mutmut`). **Every job is advisory and none is in branch protection.** See below for how each signal reaches a reviewer. | Several heavier legs (server-DB store tests, load/throughput, service-smoke, DICOM/FHIR breadth) run **nightly on a schedule** and/or only when a PR touches their paths, so an ordinary PR does not pay for @@ -38,6 +39,26 @@ CodeQL and Scorecard run on PRs but are **advisory** (not in the required set) `security-events: write`, which fork-PR tokens do not have, so requiring them would block PRs from forks. Nightly / path-gated legs (service-smoke, load, SQL/Postgres store) are deliberately **not** required. +The `quality-advisory.yml` jobs create **no code-scanning category and no new check context**, and they +**must never be added to the required list**. The guarantee is stronger than "branch protection is +untouched": that workflow holds **no write permission on any job** (`contents: read` and nothing else), +so it cannot gate a merge even if branch protection or a ruleset changes. +`tests/test_quality_advisory_invariants.py` fails if a write scope, a SARIF upload, or a removed +`--exit-zero` ever lands there. + +### How the advisory quality signals reach a reviewer + +These use GitHub **workflow-command annotations** (`::notice` / `::warning` on stdout) rather than code +scanning. That needs no token and no permission grant, and behaves identically on fork PRs. The +reasoning — including why SARIF was measured and rejected — is recorded in the workflow's header comment. + +| Signal | Where it shows up | +|---|---| +| Diff-coverage | **Inline on the Files changed tab**, one `::notice` per contiguous uncovered range of lines the PR changed, plus a step summary. | +| Complexity (`C901`) | A **merge-base-vs-HEAD delta** — only functions this PR introduced over the threshold or made more complex — as annotations plus a summary table. Pre-existing findings are deliberately never reported; the full list stays in the job log. | +| Duplication (`jscpd`) | Step summary only. jscpd emits one location per clone pair chosen by scan order, so annotating it would anchor on the untouched twin about half the time. | +| Mutation (`mutmut`) | Step summary and an artifact. This job never runs on pull requests, so it has **no PR surface** by design. | + ### The `CI gate` roll-up `CI gate` `needs:` the individual legs, runs with `if: always()`, and fails **only** on a `failure` or diff --git a/docs/quality-gates/HANDOFF-mutation-coverage.md b/docs/quality-gates/HANDOFF-mutation-coverage.md index 7e31f46d..2a205a60 100644 --- a/docs/quality-gates/HANDOFF-mutation-coverage.md +++ b/docs/quality-gates/HANDOFF-mutation-coverage.md @@ -234,10 +234,37 @@ Add the cron to the workflow's `on:` block, then the job: ## 5. After they're green — flip the rubric +> **UNACTIONABLE as written (2026-07-27).** `docs/Code_Quality_Standards.md` has never existed in this +> repo's git history, despite being cited by `quality-advisory.yml`'s header and `pyproject.toml`. There is +> no rubric file to flip. Leave the existing dangling citations alone and do not add new ones; if the rubric +> is ever written, redo this step then. + Update [Code_Quality_Standards.md](../Code_Quality_Standards.md) exactly as #10/#8 were flipped in v0.3: signals 6 + 7 go from *deferred* → ✅ **Built (advisory)** in **§5** (status column), **Appendix A.2** (rows 6/7), **A.3**, and the **A.1** verdict. Bump the rubric **Version** + add a history row. +## 6. Update — how these signals now surface (2026-07-27) + +All four advisory signals were wired to reach a reviewer without buying GitHub's paid Code Quality SKU +(GA 2026-07-20, $10 per active committer/month, a standalone product **not** bundled with GHAS). They use +**workflow-command annotations**, not code scanning — no token, no permission grant, and identical +behaviour on fork PRs. The workflow still holds **no write scope on any job**. + +- **Diff-coverage** — `diff-cover --format "github-annotations:notice,markdown:diff-cover.md"` puts + `::notice` annotations **inline on the Files changed tab**. Adjacent uncovered lines are coalesced into + ranges, so a long uncovered block costs one annotation, not one per line. +- **Complexity** — a merge-base-vs-HEAD **delta** (`scripts/quality/c901_delta.py`). Raw `C901` is + unshippable as a diff signal: all 122 findings on this tree anchor on a single `def` line, so body edits + produce nothing and signature edits fire on pre-existing debt. The delta reports only what the PR caused. +- **Clones** — step summary only. jscpd emits one location per clone pair chosen by scan order. +- **Mutation** — step summary + a `.mutmut-cache` artifact (needs `include-hidden-files: true`; it is a + dotfile and `upload-artifact` skips hidden files by default, which would otherwise upload nothing and + still report success). This job never runs on `pull_request`, so it has **no PR surface** by design. + +**Open, pending a measurement:** whether the mutation job should run on PRs. Its scope is already one module +scored by one focused test file, so it may be a two-line `if:` + `paths:` change — but its wall-clock is +unmeasured. Trigger it once via `workflow_dispatch`, read the duration off the run page, then decide. + --- *Drafted 2026-07-13 by the session that shipped the complexity + clone gates (#1028) but could not run a local From cdb1240433dd4fb5061eb720c8f26d4c9c3ae2cb Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 14:18:35 -0500 Subject: [PATCH 5/8] fix(ci): say why the complexity delta was skipped instead of aborting silently Steps run under `bash -e`, so an unresolvable merge base aborted the step with no explanation -- and `continue-on-error: true` then swallowed it, leaving a green job with no delta and no reason. Nobody watches an advisory job for the ABSENCE of output, so that is a signal that could stop working indefinitely without anyone noticing. Resolve the merge base explicitly and emit a `::notice` naming the base ref when it cannot be found. Still never fails the job; it just stops being silent about why it did nothing. --- .github/workflows/quality-advisory.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/quality-advisory.yml b/.github/workflows/quality-advisory.yml index 90691135..6428dad3 100644 --- a/.github/workflows/quality-advisory.yml +++ b/.github/workflows/quality-advisory.yml @@ -86,7 +86,13 @@ jobs: run: | echo "== complexity this PR CHANGED (advisory; pre-existing findings deliberately omitted) ==" git fetch --no-tags origin "$BASE_REF" || true - MERGE_BASE="$(git merge-base HEAD "origin/$BASE_REF")" + # Steps run under `bash -e`, so an unresolvable merge base would abort this step with no + # explanation at all. Say why instead: an advisory signal that goes quiet should at least + # leave a reason in the log, since nobody watches a green advisory job for absence. + if ! MERGE_BASE="$(git merge-base HEAD "origin/$BASE_REF" 2>/dev/null)"; then + echo "::notice title=Complexity delta skipped::could not resolve a merge base against origin/$BASE_REF" + exit 0 + fi echo "merge base: $MERGE_BASE" ruff check --select C901 --output-format=json --exit-zero messagefoundry > c901-head.json # A second checkout of the merge base, so the "before" numbers come from real ruff output From e96dc4a16b1bc2d62ec3ecf1993d1c5e1b63db80 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 14:51:32 -0500 Subject: [PATCH 6/8] fix(ci): repair three defects an adversarial review found in the annotation wiring A multi-lens review of the previous four commits produced 21 candidate defects; 14 survived independent re-verification. These are the ones that mattered, each reproduced before fixing. 1. THE HEADLINE SURFACE WAS DEFEATED BY A SHALLOW FETCH. The coverage job did `git fetch --depth=1` against the COMPLETE clone that `fetch-depth: 0` had just produced. That writes .git/shallow and grafts away everything behind the base tip, and diff-cover diffs with the three-dot range `origin/...HEAD`, which needs a merge base. Once the base branch advances past the PR's merge ref -- another PR merging, or a re-run of a stale check -- the diff dies with "no merge base". Reproduced in a scratch repo modelling the PR ref. It then failed in the worst available way: diff-cover's markdown reporter runs BEFORE the annotations reporter and truncates its output file on open, so `|| true` swallowed the error, zero annotations were emitted, and the summary showed a bare heading that reads as "nothing uncovered". The `-f` guard passed on the 0-byte file. Now: no `--depth` (the fetch was redundant anyway), `-s` instead of `-f`, and a `::notice` when there is no report or no coverage.xml. 2. THE RUFF PIN TEST COUPLED A REQUIRED CHECK TO THIS WORKFLOW. Asserting the hardcoded pin equals constraints.lock meant a routine Dependabot ruff bump would red `test (ubuntu-latest, py3.14)` -- a BLOCKING context -- over a purely advisory concern. That is a worse failure than the drift it was guarding. The version is now read from the lock at run time, which removes the drift and the coupling together, and the test asserts the derivation instead of the value. 3. THE MUTATION SIGNAL IS DEAD AND WAS REPORTING SUCCESS. Verified against the real scheduled run 30248096425: mutmut 2.5.1 crashes in its pony-ORM cache (`cannot pickle 'itertools.count'`, cache.py:369) after the baseline and BEFORE generating a single mutant. The job went green in 37 seconds because of `|| true`. The previous commit made this worse by adding a summary block that would have presented a traceback as results. Now the exit status is captured, a `::warning` is emitted, and both the summary and the docs say plainly that the signal produces nothing on Python 3.14. Repairing mutmut is separate, unstarted work. Also, in the delta script: * a path outside the scan root discarded a successful repo-root strip and returned the raw absolute path, so the base tree and HEAD keyed differently and every such finding would flood as new; * a file move or function rename reported a whole file's worth of pre-existing complexity as PR-caused (13 annotations reproduced from one move). Vanished-and-reappeared findings with an identical (function, complexity, threshold) are now treated as moved. A move that also changes complexity is still reported; * a PR that TIGHTENS ruff's max-complexity would have flooded, since the threshold was carried but never compared. Mismatched thresholds now report "not comparable" instead of a fake delta; * `--max-annotations` accepted negatives, which sliced from the end of the list. And a claim of mine that was simply wrong, corrected in the workflow header, docs/CI.md and the test docstrings: "the workflow holds no write permission, so it cannot gate a merge" is a non-sequitur. Permissions and branch protection are unrelated mechanisms. What keeps these jobs advisory is that their contexts are not required and every analysis step is non-failing. The no-write-scope assertion stays on its own merits -- least privilege for jobs that run third-party code fetched at run time. docs/CI.md also claimed the jobs create "no new check context"; they do create contexts, just not required ones. And complexity annotations were described as landing inline when most of them do not: they anchor on a `def` line the PR often did not touch, so they appear in the Checks tab and the summary, which is now stated in both places. --- .github/workflows/quality-advisory.yml | 124 ++++++++++++++---- docs/CI.md | 20 ++- .../HANDOFF-mutation-coverage.md | 20 ++- scripts/quality/c901_delta.py | 81 +++++++++++- tests/test_c901_delta.py | 93 +++++++++++++ tests/test_quality_advisory_invariants.py | 71 +++++++--- 6 files changed, 345 insertions(+), 64 deletions(-) diff --git a/.github/workflows/quality-advisory.yml b/.github/workflows/quality-advisory.yml index 6428dad3..7aa3ff11 100644 --- a/.github/workflows/quality-advisory.yml +++ b/.github/workflows/quality-advisory.yml @@ -14,8 +14,11 @@ name: quality-advisory # * signal 9 - clone / duplication (jscpd) -> BUILT (clone job); step summary only, see below # * signal 8 - diff-coverage visibility -> BUILT (coverage job; PR-only); INLINE `::notice` # annotations on the Files changed tab + summary -# * signal 7 - mutation testing (highest leverage) -> BUILT (mutation job; mirror-nightly + workflow_dispatch); -# step summary + artifact, no PR surface (never runs on PRs) +# * signal 7 - mutation testing (highest leverage) -> BUILT but NOT CURRENTLY WORKING: mutmut 2.5.1 +# crashes on Python 3.14 before generating any +# mutant, so this signal reports NOTHING today. +# The job now says so loudly instead of going +# green in 37s. It never runs on PRs either way. # signal 10 (lint breadth) is a coordinated one-shot sweep, not an advisory job; DORA is a context caveat, not a gate. # # WHY THIS USES WORKFLOW-COMMAND ANNOTATIONS AND *NOT* CODE SCANNING / SARIF. This was measured, not @@ -29,9 +32,20 @@ name: quality-advisory # 3. Uploading on pull_request only leaves NO default-branch baseline, so every PR forever would report # all ~161 findings as new -- not a one-off first-PR blip. # Workflow commands need no token and no permission grant, and behave identically on fork PRs. Every job -# below therefore holds `contents: read` and nothing else; the workflow grants NO write scope anywhere, -# which is what actually guarantees it cannot gate a merge. tests/test_quality_advisory_invariants.py -# pins that, and pins the absence of any SARIF upload. +# below therefore holds `contents: read` and nothing else -- least privilege, worth pinning on its own +# merits given that two of these jobs execute third-party code fetched at run time. +# +# What actually keeps this advisory is TWO things, and permissions are neither of them: (a) these job +# contexts are not in the required-checks set on `main`, and (b) every analysis step is +# `continue-on-error: true` plus `--exit-zero` / `--fail-under=0` / `|| true`, so the job reports success +# regardless of findings. Branch-protection membership is the only thing that determines gating. +# tests/test_quality_advisory_invariants.py pins (b), the absent write scopes, and the absence of SARIF. +# +# HONEST LIMIT of the annotation mechanism: a workflow-command annotation renders INLINE on the Files +# changed tab only when its line is in the diff. That always holds for diff-coverage (every line it +# flags is a line the PR changed) but usually does NOT hold for the complexity delta, whose findings +# anchor on a `def` line the PR often did not touch -- those land in the Checks tab and the step summary +# instead. The summary table is the primary surface for complexity, not a nice-to-have. on: pull_request: @@ -61,13 +75,23 @@ jobs: persist-credentials: false # The delta step needs the base branch's history for `git merge-base`. fetch-depth: 0 - - name: Install ruff (pinned) - # Held to the SAME version as constraints.lock / requirements.lock / uv.lock (previously 0.15.19 - # against their 0.15.22 -- unguarded drift). This is now load-bearing: c901_delta.py parses ruff's - # human-readable C901 message because ruff exposes the complexity number nowhere else, so a skew - # can change the wording under the parser. Nothing automates this -- DEP-1 byte-diffs the exported - # locks and cannot see a version inside a `run:` body. - run: pipx install ruff==0.15.22 + - name: Install ruff (version derived from the lock) + # DERIVED, not duplicated. This job used to hardcode 0.15.19 while all four locks pinned 0.15.22, + # and that drift is load-bearing: c901_delta.py parses ruff's human-readable C901 message because + # ruff exposes the complexity number nowhere else, so a skew can change the wording under the + # parser. The obvious fix -- hardcode the right version and assert it matches the lock in a test -- + # would leave a REQUIRED check (the pytest suite) hostage to a routine Dependabot ruff bump: + # the lock moves, the workflow string does not, and a blocking context goes red for a purely + # advisory concern. Reading the lock at run time removes the drift AND the coupling. + run: | + RUFF_PIN="$(sed -n 's/^ruff==\([^ ;]*\).*/\1/p' constraints.lock | head -1)" + if [ -z "$RUFF_PIN" ]; then + echo "::notice title=Ruff pin not found::falling back to the unpinned ruff from PyPI" + pipx install ruff + else + echo "ruff pin from constraints.lock: $RUFF_PIN" + pipx install "ruff==$RUFF_PIN" + fi - name: Ruff C901 complexity triage (advisory - never gates) continue-on-error: true run: | @@ -198,7 +222,19 @@ jobs: BASE_REF: ${{ github.base_ref }} run: | echo "== coverage of the lines this PR changed (advisory; not a whole-repo % gate) ==" - git fetch --no-tags --depth=1 origin "$BASE_REF" || true + # NO --depth HERE. Running a shallow fetch against the COMPLETE clone that `fetch-depth: 0` + # just produced writes .git/shallow and grafts away everything behind origin/'s tip. + # diff-cover diffs with the three-dot range `origin/...HEAD`, which needs a merge base -- + # so once the base branch advances past the PR's merge ref (another PR merging, or a re-run of + # a stale check) the diff dies with "no merge base". Reproduced. It then fails in the worst + # possible way: the markdown reporter runs BEFORE the annotations reporter and truncates + # diff-cover.md on open, so `|| true` swallows the error, zero annotations are emitted, and the + # summary shows a bare heading that reads as "nothing uncovered". + git fetch --no-tags origin "$BASE_REF" || true + if [ ! -s coverage.xml ]; then + echo "::notice title=Diff coverage skipped::coverage.xml was not produced (the test step did not complete)" + exit 0 + fi # `github-annotations:notice` writes `::notice file=...,line=N,endLine=M` workflow commands to # STDOUT, which the runner turns into INLINE annotations on the Files changed tab -- no token, # no permission grant, identical behaviour on fork PRs. Adjacent uncovered lines are coalesced @@ -207,7 +243,10 @@ jobs: # The console report still prints, so the job log is unchanged. diff-cover coverage.xml --compare-branch="origin/$BASE_REF" --fail-under=0 \ --format "github-annotations:notice,markdown:diff-cover.md" || true - if [ -f diff-cover.md ]; then + # `-s` not `-f`: diff-cover creates and truncates this file BEFORE it can fail, so a crashed + # run leaves a 0-byte report. Testing for existence alone would append an empty section that + # looks like a clean result -- the exact silent-success shape this workflow keeps producing. + if [ -s diff-cover.md ]; then { echo "## Diff coverage" echo "" @@ -216,6 +255,8 @@ jobs: # ENTIRELY -- truncate rather than silently lose the whole summary. head -c 900000 diff-cover.md } >> "$GITHUB_STEP_SUMMARY" + else + echo "::notice title=Diff coverage unavailable::diff-cover produced no report (see the job log)" fi mutation: @@ -259,20 +300,47 @@ jobs: # Ephemeral mutmut config (the repo ships none): mutate one pure module, scored by its own # focused test file so each mutant re-runs seconds of tests, not the whole suite. printf '[mutmut]\npaths_to_mutate=messagefoundry/parsing/binary.py\nrunner=python -m pytest -x -q tests/test_binary_carriage.py\n' > setup.cfg - mutmut run || true - echo "== results ==" - mutmut results 2>&1 | tee mutmut-results.txt || true - { - echo "## Mutation testing (bounded scope: messagefoundry/parsing/binary.py)" - echo "" - echo '```' - head -c 900000 mutmut-results.txt - echo '```' - echo "" - echo "Advisory only. This job never runs on pull requests (cron on the public repo +" - echo "workflow_dispatch), so it has no PR surface by design — surviving mutants are reviewed" - echo "from this run page." - } >> "$GITHUB_STEP_SUMMARY" + # KNOWN BROKEN ON PYTHON 3.14 (verified from run 30248096425, 2026-07-27): mutmut 2.5.1 crashes + # in its pony-ORM cache -- `TypeError: cannot pickle 'itertools.count' object` at + # cache.py:369 update_line_numbers -- AFTER printing its banner and running the baseline, and + # BEFORE generating a single mutant. `|| true` then made the job green in 37s, so this signal + # has been reporting success while producing nothing. Do not present it as results; make the + # crash visible and let the exit status be seen. mutmut 3.x has a different CLI and mutating + # this to work is a separate piece of work, not part of surfacing existing signals. + if mutmut run > mutmut-run.txt 2>&1; then + MUTMUT_OK=1 + else + MUTMUT_OK=0 + fi + cat mutmut-run.txt + if [ "$MUTMUT_OK" = "1" ]; then + echo "== results ==" + mutmut results > mutmut-results.txt 2>&1 || true + cat mutmut-results.txt + { + echo "## Mutation testing (bounded scope: messagefoundry/parsing/binary.py)" + echo "" + echo '```' + head -c 900000 mutmut-results.txt + echo '```' + echo "" + echo "Advisory only. This job never runs on pull requests, so it has no PR surface by" + echo "design — surviving mutants are reviewed from this run page." + } >> "$GITHUB_STEP_SUMMARY" + else + echo "::warning title=Mutation run failed::mutmut produced no mutants (known: mutmut 2.5.1 is broken on Python 3.14)" + { + echo "## Mutation testing — NOT RUNNING" + echo "" + echo "\`mutmut run\` exited non-zero and generated **no mutants**. mutmut 2.5.1 crashes on" + echo "Python 3.14 (\`cannot pickle 'itertools.count'\`) before mutation begins, so this" + echo "signal currently reports nothing. Tail of the run:" + echo "" + echo '```' + tail -c 4000 mutmut-run.txt + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + fi - name: Upload the mutmut cache (advisory) if: always() continue-on-error: true diff --git a/docs/CI.md b/docs/CI.md index cb159987..4e5bda13 100644 --- a/docs/CI.md +++ b/docs/CI.md @@ -39,10 +39,13 @@ CodeQL and Scorecard run on PRs but are **advisory** (not in the required set) `security-events: write`, which fork-PR tokens do not have, so requiring them would block PRs from forks. Nightly / path-gated legs (service-smoke, load, SQL/Postgres store) are deliberately **not** required. -The `quality-advisory.yml` jobs create **no code-scanning category and no new check context**, and they -**must never be added to the required list**. The guarantee is stronger than "branch protection is -untouched": that workflow holds **no write permission on any job** (`contents: read` and nothing else), -so it cannot gate a merge even if branch protection or a ruleset changes. +The `quality-advisory.yml` jobs create **no code-scanning category** and **no _required_ check context** — +they do report as ordinary advisory checks, and they **must never be added to the required list**. Two +things keep them advisory: they are absent from branch protection, and every analysis step is +`continue-on-error: true` plus `--exit-zero` / `--fail-under=0` / `|| true`, so the job reports success +whatever it finds. (The workflow also holds **no write permission on any job** — that is least privilege, +worth having because two of these jobs run third-party code fetched at run time, but it is *not* what +determines merge gating; required-checks membership is.) `tests/test_quality_advisory_invariants.py` fails if a write scope, a SARIF upload, or a removed `--exit-zero` ever lands there. @@ -52,12 +55,15 @@ These use GitHub **workflow-command annotations** (`::notice` / `::warning` on s scanning. That needs no token and no permission grant, and behaves identically on fork PRs. The reasoning — including why SARIF was measured and rejected — is recorded in the workflow's header comment. +An annotation renders **inline on Files changed only when its line is in the diff**. That is always true +for diff-coverage and usually *not* true for complexity, so the two land in different places: + | Signal | Where it shows up | |---|---| -| Diff-coverage | **Inline on the Files changed tab**, one `::notice` per contiguous uncovered range of lines the PR changed, plus a step summary. | -| Complexity (`C901`) | A **merge-base-vs-HEAD delta** — only functions this PR introduced over the threshold or made more complex — as annotations plus a summary table. Pre-existing findings are deliberately never reported; the full list stays in the job log. | +| Diff-coverage | **Inline on the Files changed tab**, one `::notice` per contiguous uncovered range of lines the PR changed, plus a step summary. Every line it flags is a line the PR touched, so this is the one signal that is reliably inline. | +| Complexity (`C901`) | A **merge-base-vs-HEAD delta** — only functions this PR introduced over the threshold or made more complex. Findings anchor on the `def` line, which a body-only edit does not touch, so **most complexity annotations appear in the Checks tab and the step summary rather than inline**. The summary table is this signal's primary surface. Pre-existing findings are never reported; the full list stays in the job log. | | Duplication (`jscpd`) | Step summary only. jscpd emits one location per clone pair chosen by scan order, so annotating it would anchor on the untouched twin about half the time. | -| Mutation (`mutmut`) | Step summary and an artifact. This job never runs on pull requests, so it has **no PR surface** by design. | +| Mutation (`mutmut`) | **Currently produces nothing.** mutmut 2.5.1 crashes on Python 3.14 (`cannot pickle 'itertools.count'`) before generating a single mutant; the job used to go green in 37s regardless. It now emits a `::warning` and says so in the summary instead of reporting success. It also never runs on pull requests, so it has no PR surface either way. Fixing it (mutmut 3.x has a different CLI, or a different tool) is separate, unstarted work. | ### The `CI gate` roll-up diff --git a/docs/quality-gates/HANDOFF-mutation-coverage.md b/docs/quality-gates/HANDOFF-mutation-coverage.md index 2a205a60..524ba4b2 100644 --- a/docs/quality-gates/HANDOFF-mutation-coverage.md +++ b/docs/quality-gates/HANDOFF-mutation-coverage.md @@ -257,13 +257,19 @@ behaviour on fork PRs. The workflow still holds **no write scope on any job**. unshippable as a diff signal: all 122 findings on this tree anchor on a single `def` line, so body edits produce nothing and signature edits fire on pre-existing debt. The delta reports only what the PR caused. - **Clones** — step summary only. jscpd emits one location per clone pair chosen by scan order. -- **Mutation** — step summary + a `.mutmut-cache` artifact (needs `include-hidden-files: true`; it is a - dotfile and `upload-artifact` skips hidden files by default, which would otherwise upload nothing and - still report success). This job never runs on `pull_request`, so it has **no PR surface** by design. - -**Open, pending a measurement:** whether the mutation job should run on PRs. Its scope is already one module -scored by one focused test file, so it may be a two-line `if:` + `paths:` change — but its wall-clock is -unmeasured. Trigger it once via `workflow_dispatch`, read the duration off the run page, then decide. +- **Mutation** — **the signal is dead and was reporting success anyway.** Measured from the scheduled run + `30248096425` (2026-07-27): the job takes **37 seconds** and goes green because `mutmut run || true` + swallows a crash. mutmut 2.5.1 prints its banner, runs the baseline, then dies in its pony-ORM cache with + `TypeError: cannot pickle 'itertools.count' object` (`cache.py:369 update_line_numbers`) — **before + generating a single mutant**. The job now captures the exit status, emits a `::warning`, and says so in + the summary rather than presenting a results block for a tool that produced none. The `.mutmut-cache` + artifact still uploads (`include-hidden-files: true` — it is a dotfile and `upload-artifact` skips hidden + files by default), but expect it to be a stale/empty cache until the tool works. + +**The real blocker (supersedes the old "measure the wall-clock" step):** the PR-vs-nightly question cannot +be answered while the tool produces nothing — 37s is the cost of crashing, not of mutating. Fix mutmut +first (mutmut 3.x has a different CLI; cosmic-ray is the other candidate), *then* measure a real run, then +decide about PRs. Do not read the current duration as evidence that a PR run would be cheap. --- diff --git a/scripts/quality/c901_delta.py b/scripts/quality/c901_delta.py index ebd750dc..fb6654b3 100644 --- a/scripts/quality/c901_delta.py +++ b/scripts/quality/c901_delta.py @@ -81,6 +81,7 @@ def _normalise_path(filename: str, repo_root: str, scan_root: str) -> str: anchor = f"{scan_root}/" # Windows paths are case-insensitive; comparing casefolded avoids a C:/ vs c:/ miss. + stripped: str | None = None if root and normalised.casefold().startswith(f"{root.casefold()}/"): stripped = normalised[len(root) + 1 :] if stripped.startswith(anchor): @@ -92,6 +93,12 @@ def _normalise_path(filename: str, repo_root: str, scan_root: str) -> str: if index != -1: return normalised[index + 1 :] + # No scan-root anchor -- e.g. a widened scope that reports a file outside the package. Prefer the + # successful root strip over the raw absolute path: returning the latter would give the base tree + # and HEAD different keys for the same file, and every such finding would flood as new. + if stripped is not None: + return stripped.lstrip("./") + # Already relative, or a shape we cannot anchor. Return it normalised rather than guessing. return normalised.lstrip("./") @@ -141,6 +148,34 @@ class Change(NamedTuple): line: int +def _moved_unchanged(base: dict[Key, Finding], head: dict[Key, Finding]) -> set[Key]: + """Head keys that look like a rename/move of an identical base finding. + + A file move or a function rename makes every complex function in it read as NEW while its old key + silently disappears -- pre-existing debt annotated as PR-caused, a whole file's worth at a time. + Proper detection needs git rename data; this is the cheap floor that needs none: if a vanished base + finding has exactly the same (function, complexity, threshold) as an appearing head one, treat it as + moved rather than introduced. Conservative in the right direction -- an ACTUAL new function that + coincidentally matches a deleted one's name and exact complexity is merely under-reported, whereas + the flood it prevents is the failure that makes the whole signal untrustworthy. + """ + vanished: dict[tuple[str, int, int], int] = {} + for key, finding in base.items(): + if key not in head: + signature = (key.function, finding.complexity, finding.threshold) + vanished[signature] = vanished.get(signature, 0) + 1 + + moved: set[Key] = set() + for key, finding in sorted(head.items()): + if key in base: + continue + signature = (key.function, finding.complexity, finding.threshold) + if vanished.get(signature, 0) > 0: + vanished[signature] -= 1 + moved.add(key) + return moved + + def classify( base: dict[Key, Finding], head: dict[Key, Finding] ) -> tuple[list[Change], list[Change], list[Change]]: @@ -148,10 +183,13 @@ def classify( new: list[Change] = [] increased: list[Change] = [] decreased: list[Change] = [] + moved = _moved_unchanged(base, head) for key, finding in sorted(head.items()): previous = base.get(key) if previous is None: + if key in moved: + continue new.append(Change(key, None, finding.complexity, finding.threshold, finding.line)) elif finding.complexity > previous.complexity: increased.append( @@ -228,13 +266,30 @@ def _markdown(new: list[Change], increased: list[Change], decreased: list[Change return "\n".join(lines) +def _non_negative_int(value: str) -> int: + parsed = int(value) + if parsed < 0: + raise argparse.ArgumentTypeError(f"must be >= 0, got {parsed}") + return parsed + + +def _threshold_of(findings: dict[Key, Finding]) -> int | None: + """The mccabe threshold these findings were measured against, if they agree on one.""" + thresholds = {f.threshold for f in findings.values()} + return thresholds.pop() if len(thresholds) == 1 else None + + def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--base", required=True, help="ruff C901 JSON from the merge base") parser.add_argument("--head", required=True, help="ruff C901 JSON from HEAD") parser.add_argument("--repo-root", default=".", help="checkout root, for relative paths") parser.add_argument("--scan-root", default="messagefoundry", help="scanned package name") - parser.add_argument("--max-annotations", type=int, default=_DEFAULT_MAX_ANNOTATIONS) + # Non-negative: a negative cap would slice from the END of the list (reporting the wrong findings) + # and print a nonsense "further change(s)" count. + parser.add_argument( + "--max-annotations", type=_non_negative_int, default=_DEFAULT_MAX_ANNOTATIONS + ) parser.add_argument( "--summary-file", default=None, help="append markdown here (GITHUB_STEP_SUMMARY)" ) @@ -243,6 +298,30 @@ def main(argv: list[str] | None = None) -> int: repo_root = os.path.abspath(args.repo_root) base = _parse_findings(args.base, repo_root, args.scan_root) head = _parse_findings(args.head, repo_root, args.scan_root) + + # A PR that TIGHTENS ruff's max-complexity makes every function between the old and new thresholds + # appear as new -- pre-existing debt, annotated en masse, on the one PR whose author already knows. + # The two runs are not comparable, so say that instead of reporting a fake delta. + base_threshold, head_threshold = _threshold_of(base), _threshold_of(head) + if ( + base_threshold is not None + and head_threshold is not None + and base_threshold != head_threshold + ): + print( + f"::notice title=Complexity delta not comparable::the mccabe threshold moved " + f"{base_threshold} -> {head_threshold}; a delta against the old threshold would be noise" + ) + summary = ( + "## Cyclomatic complexity (C901) — not comparable\n\n" + f"This PR changes the mccabe threshold ({base_threshold} → {head_threshold}), so a " + "before/after delta would report pre-existing functions as new. Skipped deliberately.\n" + ) + if args.summary_file: + with open(args.summary_file, "a", encoding="utf-8") as handle: + handle.write(summary) + return 0 + new, increased, decreased = classify(base, head) annotated = new + increased diff --git a/tests/test_c901_delta.py b/tests/test_c901_delta.py index 313a4615..3635ecd6 100644 --- a/tests/test_c901_delta.py +++ b/tests/test_c901_delta.py @@ -312,6 +312,99 @@ def test_main_appends_rather_than_truncating_the_summary(tmp_path: Path) -> None assert "## existing content" in summary.read_text(encoding="utf-8") +def test_a_file_move_does_not_report_pre_existing_complexity_as_new(tmp_path: Path) -> None: + """Renaming a file changes every key in it. Without rename tolerance the whole file's worth of + pre-existing debt annotates as PR-caused -- the flood this script exists to prevent, arriving by + a different door.""" + old = [ + _finding(f"/repo/messagefoundry/old/m{i}.py", f"fn{i}", 12 + i, i + 1) for i in range(13) + ] + moved = [ + _finding(f"/repo/messagefoundry/new/m{i}.py", f"fn{i}", 12 + i, i + 1) for i in range(13) + ] + base = _parse(tmp_path, "base.json", old) + head = _parse(tmp_path, "head.json", moved) + + new, increased, decreased = delta.classify(base, head) + + assert new == [], f"a pure move must annotate nothing, got {[c.key for c in new]}" + assert increased == [] and decreased == [] + + +def test_a_move_that_also_increases_complexity_is_still_reported(tmp_path: Path) -> None: + """Rename tolerance must not swallow a real regression that happens to accompany a move.""" + base = _parse(tmp_path, "base.json", [_finding("/repo/messagefoundry/a.py", "fn", 12, 1)]) + head = _parse(tmp_path, "head.json", [_finding("/repo/messagefoundry/b.py", "fn", 40, 1)]) + + new, increased, decreased = delta.classify(base, head) + + assert len(new) == 1, "a moved function whose complexity changed is not an unchanged move" + assert new[0].after == 40 + + +def test_a_threshold_change_reports_not_comparable_instead_of_flooding( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A PR tightening max-complexity would otherwise report every function between the old and new + thresholds as new.""" + base = [ + _finding(f"/repo/messagefoundry/f{i}.py", f"fn{i}", 12, 1, threshold=10) for i in range(30) + ] + head = [ + _finding(f"/repo/messagefoundry/f{i}.py", f"fn{i}", 12, 1, threshold=5) for i in range(30) + ] + summary = tmp_path / "summary.md" + + rc = delta.main( + [ + "--base", + _write(tmp_path, "base.json", base), + "--head", + _write(tmp_path, "head.json", head), + "--repo-root", + "/repo", + "--summary-file", + str(summary), + ] + ) + out = capsys.readouterr().out + + assert rc == 0 + assert "::warning" not in out + assert "not comparable" in out + assert "not comparable" in summary.read_text(encoding="utf-8") + + +def test_a_negative_annotation_cap_is_rejected(tmp_path: Path) -> None: + """A negative cap would slice from the END of the list, reporting the wrong findings.""" + with pytest.raises(SystemExit): + delta.main( + [ + "--base", + _write(tmp_path, "base.json", []), + "--head", + _write(tmp_path, "head.json", []), + "--max-annotations", + "-5", + ] + ) + + +def test_a_path_outside_the_scan_root_still_keys_consistently(tmp_path: Path) -> None: + """A widened scope reporting a file outside the package must not key the base tree copy + differently from HEAD's -- that would flood every such finding as new.""" + root = "/home/runner/work/r/r" + head = _parse(tmp_path, "head.json", [_finding(f"{root}/tee/x.py", "f", 30, 10)], root) + base = _parse( + tmp_path, "base.json", [_finding(f"{root}/base-tree/tee/x.py", "f", 30, 10)], root + ) + + assert set(head) == {delta.Key("tee/x.py", "f")} + # The base tree copy cannot be root-stripped to the same string, so rename tolerance is what + # keeps it from flooding; either way it must not be reported as new. + assert delta.classify(base, head)[0] == [] + + def test_workflow_command_metacharacters_are_escaped() -> None: change = delta.Change( key=delta.Key(path="messagefoundry/a,b.py", function="weird%name"), diff --git a/tests/test_quality_advisory_invariants.py b/tests/test_quality_advisory_invariants.py index 5a9a29bf..0224ed9b 100644 --- a/tests/test_quality_advisory_invariants.py +++ b/tests/test_quality_advisory_invariants.py @@ -5,10 +5,15 @@ This repo already parses workflow YAML in tests for exactly that reason (see test_dependabot_automerge_guardrails.py, test_release_pipeline.py, test_lint_scope_parity.py). -The single strongest assertion here is that the workflow holds NO write permission on any job. -That -- not "it is absent from branch protection" -- is what makes it structurally incapable of -gating a merge, and it is only assertable because these jobs surface findings via workflow-command -annotations rather than by uploading SARIF to code scanning. +What actually keeps these jobs advisory is that their contexts are not in the required-checks set on +`main`, and that every analysis step is non-failing (`continue-on-error` + `--exit-zero` / +`--fail-under=0` / `|| true`). Only the second half is assertable from inside the repo, so that is +what test_every_analysis_step_cannot_fail_its_job pins. + +The no-write-scope assertion is a separate, narrower claim: least privilege. It does NOT by itself +prevent merge gating -- permissions and branch protection are unrelated mechanisms -- but it is +worth pinning because two of these jobs execute third-party code fetched at run time, and it is +only holdable because the jobs surface findings via workflow commands rather than SARIF upload. """ import re @@ -21,8 +26,8 @@ # Steps that actually run a quality tool. Setup steps (checkout, setup-python, apt-get, the tool # installs) are deliberately NOT required to be continue-on-error: masking an infrastructure failure -# would produce confusing downstream errors, and the merge-blocking guarantee comes from the absent -# write scopes plus the workflow's absence from branch protection, not from those steps. +# would produce confusing downstream errors, and a red ADVISORY job blocks nothing anyway -- these +# contexts are not in the required-checks set. _ANALYSIS_MARKERS = ( "ruff check", "npx --yes jscpd", @@ -92,10 +97,10 @@ def test_workflow_grants_no_permissions_by_default(workflow: dict) -> None: def test_no_job_holds_any_write_scope(workflow: dict) -> None: - """The load-bearing assertion. A write scope anywhere here is a merge-gating capability in - disguise, and both the clone and complexity jobs execute third-party code fetched at run time - (`npx --yes jscpd`, `pipx install ruff`) with no integrity pin -- handing those a write-scoped - repository token would be a least-privilege regression.""" + """Least privilege. Both the clone and complexity jobs execute third-party code fetched at run + time (`npx --yes jscpd`, `pipx install ruff`) with no integrity pin, so handing them a + write-scoped repository token would be a real regression. This does NOT by itself stop the jobs + gating a merge -- see the module docstring -- it just keeps their blast radius at zero.""" for name, job in workflow["jobs"].items(): permissions = job.get("permissions") assert permissions is not None, f"job {name!r} must declare explicit permissions" @@ -158,18 +163,26 @@ def test_diff_cover_is_pinned_exactly(raw: str) -> None: assert re.search(r'"diff-cover==\d+\.\d+\.\d+"', raw), "diff-cover must be pinned with ==" -def test_ruff_pin_matches_the_lock(raw: str) -> None: +def test_the_ruff_version_is_derived_from_the_lock_not_hardcoded(workflow: dict, code: str) -> None: """The delta script parses ruff's human-readable C901 message, so a version skew can change the - wording under the parser. DEP-1 byte-diffs the exported locks and cannot see this one.""" - match = re.search(r"pipx install ruff==(\S+)", raw) - assert match, "the complexity job must pin ruff" - workflow_pin = match.group(1) - - lock = (_WORKFLOW.parents[2] / "constraints.lock").read_text(encoding="utf-8") - locked = re.search(r"^ruff==(\S+?)(?:\s|$)", lock, re.MULTILINE) - assert locked, "ruff not found in constraints.lock" - assert workflow_pin == locked.group(1), ( - f"workflow pins ruff=={workflow_pin} but constraints.lock has {locked.group(1)}" + wording under the parser -- but the fix must NOT be a hardcoded pin asserted against the lock. + + This test file runs in the ordinary pytest suite, which IS a required check. Asserting equality + between a workflow string and constraints.lock would mean a routine Dependabot ruff bump reds a + BLOCKING context over a purely advisory concern. Deriving the version at run time removes the + drift and the coupling at once, so assert the derivation instead of the value. + """ + step = next( + s + for job, s in _steps(workflow) + if job == "complexity" and "pipx install" in (s.get("run") or "") + ) + body = step["run"] + assert "constraints.lock" in body, ( + "the complexity job must read ruff's version from constraints.lock at run time" + ) + assert not re.search(r"pipx install ruff==\d", code), ( + "do not hardcode the ruff version here -- it couples a required check to this workflow" ) @@ -217,6 +230,22 @@ def test_checkouts_do_not_persist_credentials(workflow: dict) -> None: ) +def test_the_coverage_job_does_not_reshallow_its_own_full_clone(code: str) -> None: + """`fetch-depth: 0` then `git fetch --depth=1` writes .git/shallow and grafts away the history + behind the base tip, so diff-cover's three-dot range loses its merge base the moment the base + branch advances. It then fails in the worst way: the markdown reporter truncates diff-cover.md + on open before raising, `|| true` swallows the error, and the summary shows a clean-looking + empty section while zero annotations are emitted.""" + assert "--depth" not in code, "a shallow fetch here defeats diff-cover's merge base" + + +def test_report_guards_test_for_content_not_mere_existence(code: str) -> None: + """diff-cover creates and truncates its report before it can fail, so `-f` passes on a 0-byte + file and would append an empty section that reads as 'nothing uncovered'.""" + assert "[ -s diff-cover.md ]" in code + assert "[ -f diff-cover.md ]" not in code + + def test_step_summary_writes_are_size_guarded(raw: str) -> None: """An oversized $GITHUB_STEP_SUMMARY write is dropped ENTIRELY, losing the whole surface.""" appends = raw.count('>> "$GITHUB_STEP_SUMMARY"') From 2aa5924edd01b0a1f95c3df016716d759bbd77f8 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 16:42:32 -0500 Subject: [PATCH 7/8] =?UTF-8?q?fix(ci):=20make=20mutation=20testing=20actu?= =?UTF-8?q?ally=20run=20=E2=80=94=20mutmut=203,=20and=20it=20is=20cheap,?= =?UTF-8?q?=20so=20it=20runs=20on=20PRs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signal 7 had been scored Built since 2026-07-14 while producing nothing. `mutmut<3` resolved to 2.5.1, which crashes on Python 3.14 in its pony-ORM cache (`cannot pickle 'itertools.count'`, cache.py:369) after printing its banner and before generating a single mutant; `|| true` turned that into a green 37-second job. The highest-leverage gate in the rubric was measuring nothing, and nothing noticed because an advisory job that reports success looks exactly like one that passed. Repaired on mutmut==3.6.0, verified on Linux/Python 3.14 in a container rather than reasoned about: 461 mutants, 87 killed, 19 survived, 355 not covered by the scoped test file. Three things in the mutmut 3 config are load-bearing, and each was found by a run that silently produced nothing rather than by reading the docs: * `source_paths` must be the PACKAGE, not the single file being mutated. mutmut 3 copies source_paths into `mutants/` and runs pytest there; with one file copied, tests/conftest.py died on `ModuleNotFoundError: messagefoundry.config` and all 461 mutants came back "not checked" -- a green run, a full results file, and zero information. `only_mutate` supplies the bounded scope instead, so the whole package is copied but one module is mutated. * `pytest-timeout` is REQUIRED. mutmut 3 always passes `--timeout`/`--timeout-method` to pytest; without the plugin every invocation dies with an unrecognised argument, surfacing only as an opaque BadTestExecutionCommandsException. * `paths_to_mutate` and `runner=` are mutmut 2 keys -- deprecated and ignored respectively. MUTATION NOW RUNS ON PULL REQUESTS. The "expensive, never per-PR" note this job carried was a property of mutmut 2 running the suite once per mutant, not of this scope: mutmut 3 runs only the tests that cover each mutant, so the mutating costs ~3 seconds and the job's real cost is its install step, same as its siblings. Survivors are most useful in review, where someone is already looking at the test that failed to kill them. The step summary carries a killed/survived/not-covered table plus the survivor list; a non-zero `mutmut run` emits a ::warning rather than passing quietly. The artifact now uploads the results and run log alongside the cache, since the cache alone is useless to a human. --- .github/workflows/quality-advisory.yml | 100 +++++++++++++--------- .gitignore | 3 + docs/CI.md | 2 +- tests/test_quality_advisory_invariants.py | 39 ++++++++- 4 files changed, 98 insertions(+), 46 deletions(-) diff --git a/.github/workflows/quality-advisory.yml b/.github/workflows/quality-advisory.yml index 7aa3ff11..c982483b 100644 --- a/.github/workflows/quality-advisory.yml +++ b/.github/workflows/quality-advisory.yml @@ -14,11 +14,11 @@ name: quality-advisory # * signal 9 - clone / duplication (jscpd) -> BUILT (clone job); step summary only, see below # * signal 8 - diff-coverage visibility -> BUILT (coverage job; PR-only); INLINE `::notice` # annotations on the Files changed tab + summary -# * signal 7 - mutation testing (highest leverage) -> BUILT but NOT CURRENTLY WORKING: mutmut 2.5.1 -# crashes on Python 3.14 before generating any -# mutant, so this signal reports NOTHING today. -# The job now says so loudly instead of going -# green in 37s. It never runs on PRs either way. +# * signal 7 - mutation testing (highest leverage) -> BUILT and WORKING on mutmut 3 (2.5.1 crashed on +# Python 3.14 before producing a mutant, and `|| true` +# made that look green for months). Measured: 461 +# mutants in 3s -- 87 killed, 19 survived. Runs on +# PRs too; survivor table in the step summary. # signal 10 (lint breadth) is a coordinated one-shot sweep, not an advisory job; DORA is a context caveat, not a gate. # # WHY THIS USES WORKFLOW-COMMAND ANNOTATIONS AND *NOT* CODE SCANNING / SARIF. This was measured, not @@ -51,9 +51,9 @@ on: pull_request: workflow_dispatch: schedule: - # Nightly mutation run. The mutation job's `if:` gates it to the MIRROR (free minutes); on the - # private repo mutation runs only via workflow_dispatch. The cheap jobs (complexity/clone) also - # fire on this cron but cost seconds, and coverage is PR-only so it no-ops here. + # Nightly sweep. Every job here is cheap (mutation is ~3s of actual mutating; complexity and clone + # are seconds), so the cron is a safety net that catches drift on `main` rather than a cost dodge. + # Coverage is PR-only -- it needs a base ref to diff against -- so it no-ops here. - cron: "23 4 * * *" # Deny-by-default at the workflow level; each job grants only what it needs. @@ -262,12 +262,13 @@ jobs: mutation: # Signal 7 - mutation testing (advisory; the HIGHEST-leverage gate -- it adversarially checks the # tests actually assert something, catching the "tests that never fail" AI-slop mode, rubric section 3). - # EXPENSIVE: mutmut runs the scoped tests once per mutant, so this is NOT per-PR. It runs on the - # mirror's nightly cron (free minutes) and on manual workflow_dispatch (so it can run on the private - # repo on demand -- e.g. to verify this job). Scope is bounded to one well-tested pure module to keep - # the run tractable; widen it later. Non-required; `|| true` so a surviving mutant never fails a build. + # + # NOT expensive, contrary to the note that used to sit here. MEASURED on Python 3.14 (linux, the + # bounded scope below): 461 mutants in 3 SECONDS -- 87 killed, 19 survived, 355 not covered by the + # scoped test file. mutmut 3 only runs the tests that actually cover each mutant, so the old + # "runs the suite once per mutant" cost model does not apply. That is why this now runs on PRs too: + # a real signal for seconds of runtime is worth having in review, which is where survivors get fixed. name: mutation (advisory) - if: github.event_name == 'workflow_dispatch' || (github.event_name == 'schedule' && github.repository == 'MEFORORG/MessageFoundry') runs-on: ubuntu-latest timeout-minutes: 30 permissions: @@ -290,64 +291,81 @@ jobs: - name: Install project + mutmut run: | uv pip install --system --constraint constraints.lock -e ".[dev,harness,fhir,dicom,x12,xml,webauthn]" -e packaging/messagefoundry-webconsole - uv pip install --system --constraint constraints.lock "mutmut<3" + # mutmut 3, pinned. `mutmut<3` resolved to 2.5.1, which CRASHES on Python 3.14 in its pony-ORM + # cache (`cannot pickle 'itertools.count'`) before generating a single mutant -- verified from + # run 30248096425. pytest-timeout is REQUIRED: mutmut 3 always passes `--timeout`/ + # `--timeout-method` to pytest, and without the plugin every test invocation dies with an + # unrecognised-argument error that surfaces only as BadTestExecutionCommandsException. + # Neither package is in constraints.lock, so the --constraint above does not pin them. + uv pip install --system "mutmut==3.6.0" pytest-timeout - name: Mutation-test a bounded scope (advisory - never fails) continue-on-error: true env: QT_QPA_PLATFORM: offscreen run: | echo "== mutation testing messagefoundry/parsing/binary.py (advisory; bounded scope) ==" - # Ephemeral mutmut config (the repo ships none): mutate one pure module, scored by its own - # focused test file so each mutant re-runs seconds of tests, not the whole suite. - printf '[mutmut]\npaths_to_mutate=messagefoundry/parsing/binary.py\nrunner=python -m pytest -x -q tests/test_binary_carriage.py\n' > setup.cfg - # KNOWN BROKEN ON PYTHON 3.14 (verified from run 30248096425, 2026-07-27): mutmut 2.5.1 crashes - # in its pony-ORM cache -- `TypeError: cannot pickle 'itertools.count' object` at - # cache.py:369 update_line_numbers -- AFTER printing its banner and running the baseline, and - # BEFORE generating a single mutant. `|| true` then made the job green in 37s, so this signal - # has been reporting success while producing nothing. Do not present it as results; make the - # crash visible and let the exit status be seen. mutmut 3.x has a different CLI and mutating - # this to work is a separate piece of work, not part of surfacing existing signals. + # Ephemeral mutmut config (the repo ships none). THREE things here are load-bearing, each + # found by a run that silently produced nothing: + # * source_paths is the PACKAGE, not the one file. mutmut 3 copies source_paths into + # `mutants/` and runs pytest THERE; with a single file copied, tests/conftest.py died on + # `ModuleNotFoundError: messagefoundry.config` and every mutant came back "not checked". + # * only_mutate keeps the bounded scope -- the whole package is copied, one module mutated. + # * pytest_add_cli_args_test_selection replaces mutmut 2's `runner=`; the old `runner` and + # `paths_to_mutate` keys are gone/deprecated in 3.x. + printf '[mutmut]\nsource_paths=messagefoundry\nonly_mutate=messagefoundry/parsing/binary.py\npytest_add_cli_args_test_selection=tests/test_binary_carriage.py\n' > setup.cfg if mutmut run > mutmut-run.txt 2>&1; then MUTMUT_OK=1 else MUTMUT_OK=0 fi - cat mutmut-run.txt + tail -c 20000 mutmut-run.txt if [ "$MUTMUT_OK" = "1" ]; then - echo "== results ==" mutmut results > mutmut-results.txt 2>&1 || true - cat mutmut-results.txt + KILLED="$(grep -c ': killed' mutmut-results.txt || true)" + SURVIVED="$(grep -c ': survived' mutmut-results.txt || true)" + NOTESTS="$(grep -c ': no tests' mutmut-results.txt || true)" + echo "== killed=$KILLED survived=$SURVIVED no-tests=$NOTESTS ==" + grep ': survived' mutmut-results.txt || true { - echo "## Mutation testing (bounded scope: messagefoundry/parsing/binary.py)" + echo "## Mutation testing — \`messagefoundry/parsing/binary.py\`" echo "" - echo '```' - head -c 900000 mutmut-results.txt - echo '```' + echo "| Killed | Survived | Not covered |" + echo "| --- | --- | --- |" + echo "| $KILLED | **$SURVIVED** | $NOTESTS |" + echo "" + if [ "$SURVIVED" != "0" ]; then + echo "Survivors — injected bugs the tests did **not** catch:" + echo "" + echo '```' + grep ': survived' mutmut-results.txt | head -c 100000 + echo '```' + fi echo "" - echo "Advisory only. This job never runs on pull requests, so it has no PR surface by" - echo "design — surviving mutants are reviewed from this run page." + echo "Advisory only. A survivor is a hint to strengthen an assertion, not a defect —" + echo "and mutation score is a poor single number (rubric §2), so this never gates." } >> "$GITHUB_STEP_SUMMARY" else - echo "::warning title=Mutation run failed::mutmut produced no mutants (known: mutmut 2.5.1 is broken on Python 3.14)" + echo "::warning title=Mutation run failed::mutmut exited non-zero — see the job log" { - echo "## Mutation testing — NOT RUNNING" + echo "## Mutation testing — FAILED TO RUN" echo "" - echo "\`mutmut run\` exited non-zero and generated **no mutants**. mutmut 2.5.1 crashes on" - echo "Python 3.14 (\`cannot pickle 'itertools.count'\`) before mutation begins, so this" - echo "signal currently reports nothing. Tail of the run:" + echo "\`mutmut run\` exited non-zero, so this signal produced nothing. Tail of the run:" echo "" echo '```' tail -c 4000 mutmut-run.txt echo '```' } >> "$GITHUB_STEP_SUMMARY" fi - - name: Upload the mutmut cache (advisory) + - name: Upload the mutmut results (advisory) if: always() continue-on-error: true uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: - name: mutmut-cache - path: .mutmut-cache + name: mutmut-results + path: | + mutmut-results.txt + mutmut-run.txt + .mutmut-cache # .mutmut-cache is a DOTFILE and upload-artifact skips hidden files by DEFAULT. Without this # the step logs "No files were found", uploads nothing, and still reports success -- a silent # no-op that looks green. diff --git a/.gitignore b/.gitignore index cd2a0aa4..25907273 100644 --- a/.gitignore +++ b/.gitignore @@ -68,6 +68,9 @@ coverage.xml diff-cover.md jscpd-report/ .mutmut-cache +mutants/ +mutmut-run.txt +mutmut-results.txt c901-head.json c901-base.json base-tree/ diff --git a/docs/CI.md b/docs/CI.md index 4e5bda13..fe5b1375 100644 --- a/docs/CI.md +++ b/docs/CI.md @@ -63,7 +63,7 @@ for diff-coverage and usually *not* true for complexity, so the two land in diff | Diff-coverage | **Inline on the Files changed tab**, one `::notice` per contiguous uncovered range of lines the PR changed, plus a step summary. Every line it flags is a line the PR touched, so this is the one signal that is reliably inline. | | Complexity (`C901`) | A **merge-base-vs-HEAD delta** — only functions this PR introduced over the threshold or made more complex. Findings anchor on the `def` line, which a body-only edit does not touch, so **most complexity annotations appear in the Checks tab and the step summary rather than inline**. The summary table is this signal's primary surface. Pre-existing findings are never reported; the full list stays in the job log. | | Duplication (`jscpd`) | Step summary only. jscpd emits one location per clone pair chosen by scan order, so annotating it would anchor on the untouched twin about half the time. | -| Mutation (`mutmut`) | **Currently produces nothing.** mutmut 2.5.1 crashes on Python 3.14 (`cannot pickle 'itertools.count'`) before generating a single mutant; the job used to go green in 37s regardless. It now emits a `::warning` and says so in the summary instead of reporting success. It also never runs on pull requests, so it has no PR surface either way. Fixing it (mutmut 3.x has a different CLI, or a different tool) is separate, unstarted work. | +| Mutation (`mutmut`) | A **killed / survived / not-covered** table in the step summary, with the surviving mutants listed — those are injected bugs the tests did not catch. Runs on PRs too: measured at **461 mutants in 3 seconds** (87 killed, 19 survived) over the bounded scope, because mutmut 3 only runs the tests that cover each mutant. Repaired 2026-07-27 — `mutmut<3` resolved to 2.5.1, which crashes on Python 3.14 before generating a single mutant and, thanks to `\|\| true`, had been reporting success in 37s while measuring nothing. | ### The `CI gate` roll-up diff --git a/tests/test_quality_advisory_invariants.py b/tests/test_quality_advisory_invariants.py index 0224ed9b..2630e66d 100644 --- a/tests/test_quality_advisory_invariants.py +++ b/tests/test_quality_advisory_invariants.py @@ -246,11 +246,42 @@ def test_report_guards_test_for_content_not_mere_existence(code: str) -> None: assert "[ -f diff-cover.md ]" not in code -def test_step_summary_writes_are_size_guarded(raw: str) -> None: - """An oversized $GITHUB_STEP_SUMMARY write is dropped ENTIRELY, losing the whole surface.""" - appends = raw.count('>> "$GITHUB_STEP_SUMMARY"') +def test_step_summary_writes_are_size_guarded(code: str) -> None: + """An oversized $GITHUB_STEP_SUMMARY write is dropped ENTIRELY, losing the whole surface. + + Asserts on the COUNT of truncation idioms rather than one magic constant: the blocks legitimately + use different limits (a survivor list needs less room than a coverage report), and pinning the + exact byte count made this fail on a change that was still correctly guarded. + """ + appends = code.count('>> "$GITHUB_STEP_SUMMARY"') assert appends >= 3, f"expected the summary blocks to be present, found {appends}" - assert raw.count("head -c 900000") >= 3, "each summary block must be truncated" + truncations = code.count("head -c ") + code.count("tail -c ") + assert truncations >= appends, ( + f"{appends} summary blocks but only {truncations} truncation guards -- an oversized write " + "is dropped entirely, silently losing the whole surface" + ) + + +def test_mutmut_is_pinned_to_3x_with_pytest_timeout(code: str) -> None: + """mutmut 2.5.1 crashes on Python 3.14 before generating a mutant, and `|| true` made that look + green for months. pytest-timeout is not optional: mutmut 3 always passes `--timeout` to pytest, + and without the plugin every invocation dies inside BadTestExecutionCommandsException.""" + assert re.search(r'"mutmut==3\.\d+\.\d+"', code), "mutmut must be pinned to an exact 3.x" + assert "mutmut<3" not in code, "mutmut 2.x does not run on Python 3.14" + assert "pytest-timeout" in code, "mutmut 3 requires pytest-timeout" + + +def test_mutmut_copies_the_package_not_just_the_mutated_file(code: str) -> None: + """mutmut 3 copies `source_paths` into mutants/ and runs pytest there. With a single FILE as the + source path, conftest.py cannot import the rest of the package and every mutant comes back + 'not checked' -- a green job measuring nothing. Copy the package, mutate one module.""" + # The config is emitted by `printf`, so the separators are literal backslash-n in the YAML. + assert r"source_paths=messagefoundry\n" in code, ( + "source_paths must be the package, not one file" + ) + assert "only_mutate=" in code, "the bounded scope must come from only_mutate" + assert "paths_to_mutate" not in code, "deprecated in mutmut 3" + assert "runner=" not in code, "mutmut 3 uses pytest_add_cli_args_test_selection" def test_mutmut_artifact_includes_hidden_files(workflow: dict) -> None: From 681f75c6491bc6a850cfa594567d31f8617ffabd Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 16:42:54 -0500 Subject: [PATCH 8/8] docs(quality): restore Code_Quality_Standards.md, and correct three claims measurement refuted This rubric is cited by quality-advisory.yml's header and by pyproject.toml:246, but it has never existed anywhere in this repository's git history -- both citations have always dangled. Restored from the maintained copy so they resolve. Restored faithfully at v0.9 content, then bumped to v0.10 with the corrections this cycle produced. The document's own convention is to restatus rows and record why in the version history, so that is what v0.10 does rather than silently rewriting the owner's text: * SIGNAL 7 WAS SCORED "BUILT" IN v0.8 AND v0.9 WHILE PRODUCING NOTHING. The mutation gate ran `mutmut<3` -> 2.5.1, which crashes on Python 3.14 before generating a mutant; `|| true` made it report success in 37 seconds. Now genuinely measured on mutmut 3: 461 mutants, 87 killed, 19 survived, 3 seconds. That also refutes the "Expensive / never per-PR" cost model in section 5. * SIGNAL 11's "85 functions exceed C901>10" is now 122 across 43 files, and the raw list turned out to be unusable as a diff signal -- every finding anchors on a single `def` line -- so a merge-base delta was added that reports only what a PR caused. * SIGNAL 8 now emits inline PR annotations rather than console-only output. The A- verdict stands. Worth stating plainly though: the signal-7 failure is precisely the mode this rubric exists to catch -- an advisory gate reporting success while measuring nothing -- and it was caught by re-verification, not by the gate. Section 4.1's anti-metric rule protects against trusting a number too much; nothing in the rubric protected against trusting a green check that never ran. Note for the public repo: this document links to Secure_Development_Standards.md, which is deliberately gitignored (owner decision -- security-posture docs stay private), so those particular links dangle. Its sibling Secure_AI_Development_Standards.md is tracked and resolves. The leak gate passes on the restored file. --- docs/Code_Quality_Standards.md | 378 ++++++++++++++++++ .../HANDOFF-mutation-coverage.md | 42 +- 2 files changed, 403 insertions(+), 17 deletions(-) create mode 100644 docs/Code_Quality_Standards.md diff --git a/docs/Code_Quality_Standards.md b/docs/Code_Quality_Standards.md new file mode 100644 index 00000000..d7df11b5 --- /dev/null +++ b/docs/Code_Quality_Standards.md @@ -0,0 +1,378 @@ +# Code Quality & Anti-Slop Standards — Executive Summary + +**Is this code good, or is it slop? · MEFOR verdict: A− · July 14, 2026** + +How to judge whether code — human- or AI-authored — is actually good, using signals the evidence supports. + +| At a glance | | +|----|----| +| **MEFOR verdict** | **A− / low slop-risk** — all 11 signals Built | +| **Core thesis** | No single number certifies quality — judge enforced structure and verified behavior | +| **Hard rule** | Never gate on coverage %, LOC, complexity, or SonarQube counts alone — all weak or gameable | +| **Proven value** | Caught private security docs shipping in the public PyPI package (fixed, verified at v0.3.0) | +| **Evidence base** | Peer-reviewed and adversarially verified — 20 claims confirmed, 5 popular claims refuted | + +### AI slop is real — and measurable + +- AI-assisted developers wrote *less* secure code while *more* confident it was secure (Stanford). +- ~14% of distinct Python modules in real ChatGPT output didn't exist ("slopsquatting"). +- 2024: copied lines exceeded refactored lines for the first time in industry telemetry. + +Each failure mode maps to a specific machine-enforced control — not a vibe check. + +### What actually predicts quality + +Machine-enforced structure (layer boundaries, strict typing), tests whose assertions are validated by mutation testing, and dependency + published-artifact integrity. The popular scoreboards — coverage %, complexity scores, SonarQube severities — correlate weakly, sometimes *invertedly*, with real defects. + +### The MEFOR result + +All 11 signals Built and running in CI: enforced boundaries, strict typing, 5,400+ behavior-verifying tests, locked dependencies, 11 security scanners. The one durable gap ever found — the PyPI leak — was caught by this rubric and closed. + +# The 11 Signals at a Glance + +Simplified from the full rubric (§4) and MEFOR scorecard (Appendix A), which follow. A codebase is judged by the **composite** — never by any single row. + +**Tier 1 — durable, high-signal controls (these carry the verdict)** + +| \# | Signal | What it asks | MEFOR | +|----|----|----|----| +| 1 | Architecture boundaries | Are module/layer rules machine-checked in CI, not just documented? | ✅ Strong | +| 2 | Strict typing | Full strict type-checking, no blanket suppressions? | ✅ Strong | +| 3 | Tests verify behavior | Do tests assert real values and failure paths — not mock choreography? | ✅ Strong | +| 4 | Dependency integrity | Is every dependency verified to exist and hash-locked? | ✅ Strong | +| 5 | Security scanning + threat model | Blocking scanners plus a written threat model and review? | ✅ Strong | +| 6 | Published-artifact integrity | Do released packages ship *only* intended content? | ✅ Built — caught and fixed a real leak | + +**Tier 2 — measurement layer (guidance and triage — never a gate on its own)** + +| \# | Signal | What it asks | MEFOR | +|----|----|----|----| +| 7 | Mutation testing | Do the tests actually catch injected bugs? | ✅ Built (advisory) | +| 8 | Coverage visibility | Is coverage reported on changed lines, as guidance? | ✅ Built (advisory) | +| 9 | Duplication / reuse | Is new copy-paste flagged for review? | ✅ Built (advisory) | +| 10 | Lint breadth | Is a broad static-analysis ruleset enforced? | ✅ Built — enforced in the required CI leg | +| 11 | Complexity triage | Are the genuinely large units surfaced for review? | ✅ Built (advisory) | + +> **The anti-metric rule:** none of the Tier 2 numbers may ever be *the* quality gate. They surface problems for a human to judge; the Tier 1 structure carries the verdict. + +The full standard follows: the evidence review, the AI failure-mode map, the complete rubric, gate placement, the MEFOR scorecard, and the cited methodology. + +# Code Quality & Anti-Slop Standards — Evidence-Based Rubric for Judging Code (Human- or AI-Authored) + +*A companion standard to [Secure Development Standards](Secure_Development_Standards.md) (SDS) and [Secure AI-Assisted Development Standards](Secure_AI_Development_Standards.md) (the AI-build companion). The SDS says **what a secure build must satisfy**; the AI-build companion says **how to build it with an AI assistant** (process, tiers, provenance). This document is the third leg: **how to judge whether the resulting code is actually good — not "AI slop" — using signals the evidence supports, not scoreboards it refutes.** It governs the **outcome**, where the SDS governs the process and the AI companion governs the tooling.* + +> **Scope boundary — read this first.** This is a **code-quality measurement rubric**, not a security standard (that is the [SDS](Secure_Development_Standards.md)) and not the AI-build process standard (that is the [Secure AI-Assisted Development Standards](Secure_AI_Development_Standards.md)). Where a signal here is *already owned* by a companion — dependency-existence verification, human review, SAST — this doc **points to it and does not restate it.** Its own additions are the *quality-measurement* gates neither companion carries: test-signal proof (mutation), duplication/reuse detection, coverage *visibility*, complexity *triage*, lint breadth, and **published-artifact / supply-chain-*out* integrity** (signal 6). + +| | | +|----|----| +| **Document** | Code Quality & Anti-Slop Standards | +| **Applies to** | Any project developed under the [SDS](Secure_Development_Standards.md). **MessageFoundry (MEFOR)** is the reference implementation (Appendix A); future projects add Appendix B, C, … | +| **Maintained by** | Project maintainers (open-source). Each deploying/adopting organization assigns its own local owner. | +| **Status** | Draft for review | +| **Version** | 0.10 | +| **Date** | July 27, 2026 | +| **License** | Publishable under the project's open-source license; intended to be shared with adopters and reused across projects. | +| **Review cadence** | At least annually, and on any material change to the metric evidence base or the AI toolchain. | +| **Aligns to** | **ISO/IEC 25010:2023** (product-quality model — Maintainability = modularity / reusability / analyzability / modifiability / testability) · companion to [SDS](Secure_Development_Standards.md) **PW.7 / PW.8** and the [Secure AI-Assisted Development Standards](Secure_AI_Development_Standards.md) §3 failure-modes / §9 deferred-gates. Evidence base is **peer-reviewed metric-validity studies + DORA 2024 + GitClear + the METR RCT + Stanford CCS'23**, each carried with its honesty caveat (§7). **Confers no certification.** | + +------------------------------------------------------------------------ + +## Executive summary + +**The core thesis, and it is counterintuitive:** *there is no single number that certifies code quality, and every metric people reach for first is a weak or gameable predictor.* Peer-reviewed evidence shows line-coverage %, raw cyclomatic complexity, SonarSource "Cognitive Complexity", and SonarQube issue severities all correlate **weakly — sometimes invertedly** — with real defects and change-proneness (§2). What survives is **structure and behavior, verified by enforced controls**: ISO/IEC 25010 maintainability (low coupling / information hiding), strict typing, tests whose *assertions* are validated (mutation testing as *guidance*, not a gate), dependency integrity, and static analysis in the loop. + +**"AI slop" is real but specific.** The evidence names measurable failure modes — a controlled Stanford study found AI-assisted developers wrote *less* secure code yet were *more* confident it was secure; GitClear telemetry shows 2024 was the first year copy/pasted lines exceeded refactored ("moved") lines (the signature of *copy-instead-of-abstract*); ~14.4% of *distinct* Python modules in real ChatGPT output were hallucinated (slopsquatting); DORA 2024 associated AI adoption with a delivery-**stability** drop. The answer to each is a **specific control**, not a vibe (§3). + +**This document is a measurement rubric, not another process gate.** It gives (1) the signals that genuinely separate good code from slop, with the honest note that **no validated single-metric cutoff exists** — thresholds are set empirically per project; (2) an anti-metric list of what **not** to gate on; and (3) a scored MEFOR scorecard (Appendix A). **MEFOR's verdict: strong exactly where the evidence says it counts (machine-enforced structure, strict typing, behavior-verifying tests, dependency integrity, security scanning), and — as of this cycle — the *measurement* layer has closed too: mutation, coverage visibility, clone detection, complexity triage, and the broadened lint ruleset all ship as CI gates (#1028/#1040/#1047), so every one of the 11 signals is now Built. The one *durable*-control gap this cycle surfaced — a private-doc leak in the published PyPI sdist — was caught via signal 6 and fixed (#1020), verified clean at v0.3.0.** + +------------------------------------------------------------------------ + +## 1. Purpose, scope, and the lens + +This rubric answers one question: **"Is this code good, or is it slop?"** — for code that may be human- or AI-authored, in a repository built largely with an AI assistant across many parallel sessions. + +It serves three audiences: **maintainers** (a standing scorecard to re-run each release), **adopters and auditors** (evidence the code is judged against the evidence, not a badge), and **future projects** (their own Appendix). + +**The lens — structure over scoreboards.** The evidence is unambiguous that *single-number gates fail*. Therefore this rubric is **composite and structural**: it weights machine-enforced architectural boundaries and validated test signal far above any coverage or complexity number, and it explicitly **forbids** certifying quality on a single metric (§4). This is the quality-measurement analogue of the SDS's "deterministic checks, never ask the model to be secure" principle — *measure structure and behavior, never trust a scoreboard.* + +------------------------------------------------------------------------ + +## 2. The evidence — validated vs. gameable (read before setting any gate) + +Peer-reviewed, [adversarially-verified](#b.2-how-the-matrix-was-derived) findings on what actually predicts quality (full citations → [Appendix B.4](#b.4-references)): + +| Popular metric | Verdict | Evidence | +|----|----|----| +| **Cyclomatic complexity** (raw) | **Weak predictor; largely a proxy for LOC.** Use as a local *triage smell*, never a gate. | Correlation with real bugs ≈ 0.06 (Kendall); "adds little if any" beyond executable-line counts. [\[R2\]](#r2) | +| **SonarSource "Cognitive Complexity"** | **No incremental predictive value** over traditional measures. | Peer-reviewed JSS evaluation: "does not appear to fulfill the promise." [\[R3\]](#r3) | +| **SonarQube quality-gate severities** | **Weak, inconsistent, sometimes *inverted*.** Flagged "dirty" classes are no more fault-prone than clean ones. Useful as a cheap filter, **not** a quality score. | 33 Apache projects, ~27K faults. [\[R4\]](#r4) | +| **Mutation score** (as a single number) | **Poor *linear* proxy** (mostly a test-suite-size artifact) — **but** mutation testing is high-value as *guidance*: top-decile suites catch 8–46% more real faults. | ICSE 2018 (Papadakis et al.). [\[R5\]](#r5) | +| **Line-coverage % / LOC** | **Gameable.** High coverage with weak assertions is the canonical AI-slop hiding place; LOC measures size, not quality. | Corollary of the mutation and complexity findings above. | + +**What survives as durable signal:** **ISO/IEC 25010:2023** [\[R1\]](#r1) maintainability — decomposed into *modularity, reusability, analyzability, modifiability, testability*, i.e. **low coupling + information hiding** — enforced as **architectural fitness functions** (import/layer boundaries), plus **strict typing**, **behavior-verifying tests validated by mutation testing**, **dependency integrity**, and **static analysis in the loop**. + +> **Honesty note on the hype.** Several widely-quoted "AI slop" statistics **failed adversarial verification** during this document's research: the "~40% of Copilot code is vulnerable" figure, a "10× duplicate-block surge", and two GitClear copy/move percentages were all refuted — full list in [Appendix B.5](#b.5-what-was-refuted-the-verification-worked). **Judge on the structural signals below, not on alarm-bell numbers.** (Full caveats: §7.) + +------------------------------------------------------------------------ + +## 3. AI-specific slop failure modes → the control that neutralizes each + +The [Secure AI-Assisted Development Standards §3](Secure_AI_Development_Standards.md#3-the-problem-this-standard-attacks) names *five process failure modes* (intent drift, context rot, error accumulation, the speed–quality paradox, misplaced trust). This rubric maps the **code-outcome** failure modes those produce to a measurable control: + +| AI slop failure mode | Evidence | The control (owner) | +|----|----|----| +| **Insecure code + overconfidence** | Stanford CCS'23: AI-assisted users wrote less-secure code, *more* confident it was secure. [\[R6\]](#r6) | Mandatory human review + blocking SAST that cannot be waived — **owned by [SDS PW.7](Secure_Development_Standards.md) + [AI companion §6.5/§6.6](Secure_AI_Development_Standards.md)**. This rubric only *checks it is present*. | +| **Hallucinated / typosquatted dependencies** ("slopsquatting") | ~14.4% of *distinct* Python modules in real ChatGPT output did not exist. [\[R7\]](#r7) | Verify-before-add + hash-locked lockfile + new-import audit — **owned by [AI companion §6.4/§9](Secure_AI_Development_Standards.md)**. This rubric *checks it is present*. | +| **Silent duplication over reuse** (copy-instead-of-abstract) | GitClear: 2024 was the first year copy/pasted lines (12.3%) exceeded "moved"/refactored lines (9.5%). [\[R9\]](#r9) *(Correlational — §7.)* | **Clone-detection on the diff** + a "moved vs copied" review lens. **New gate — this document (§5).** | +| **Shallow tests that assert little** | Coverage % hides assertion-free tests; mutation testing exposes them. [\[R5\]](#r5) | **Mutation testing on changed code, as guidance.** **New gate — this document (§5).** | +| **Unbounded complexity / over-abstraction** | Large, tangled units are a maintainability smell (though a weak *defect* predictor — §2). | **Advisory complexity triage** (surface, don't gate). **New gate — this document (§5).** | +| **Inconsistent conventions across a codebase** | Multi-session AI authorship drifts style/structure. | Broad lint ruleset + `CLAUDE.md` as the standing convention anchor. **Partly new (lint breadth) — §5.** | +| **Velocity ≠ delivered quality** | DORA 2024: AI adoption associated with ~7.2% lower delivery *stability* per 25% adoption. [\[R8\]](#r8) · [\[R10\]](#r10) *(Correlational, partly revised in 2025 — §7.)* | Small batch sizes + DORA change-fail/MTTR awareness. **Owned by [AI companion §3](Secure_AI_Development_Standards.md).** | +| **Control-parity gap** (a guard on one path, missed on its sibling) | AI companion §6.6: an AI implements a control *where prompted* and misses its siblings — every confirmed medium-or-higher finding in the 2026-06 MEFOR audit had this shape. Real instance: the fail-closed leak gate guarded the *git-mirror* publish path but not its sibling, the *PyPI* publish path, so the sdist shipped private docs on every release. | **Enumerate sibling paths for every control; encode as one deterministic check where feasible.** Here → the published-artifact-integrity gate (**signal 6**). | + +------------------------------------------------------------------------ + +## 4. The rubric — 11 signals that separate good code from slop + +Each signal is a **risk → control → measure**, tagged by **gate type** (deterministic = machine-checked; advisory = human arbitrates) and by which document **owns** it. The signals fall into **two tiers** — weight them accordingly. **A codebase is certified "not slop" by the *composite*, never by any single row** (§2). Signals are numbered **contiguously by tier: Tier 1 = 1–6, Tier 2 = 7–11.** + +**Tier 1 — Durable, high-signal controls (weight these most).** Structural, machine-enforceable properties where quality is *hard to fake* — the ones the evidence says actually predict maintainability and catch slop. These carry the verdict. + +| \# | Signal | What "good" looks like | Gate type | Owner | +|----|----|----|----|----| +| 1 | **Enforced architecture boundaries** (ISO 25010 modularity / low coupling) | Import/layer rules are *machine-checked in CI*, not just documented | Deterministic | SDS PW.1–2; **checked here** | +| 2 | **Strict typing** | `mypy --strict`; suppressions carry error codes (no blanket ignores) | Deterministic | AI companion §6.5; **checked here** | +| 3 | **Tests verify behavior, not mocks** | Value/negative-path assertions; real integrations over mock choreography | Deterministic | SDS PW.8; **checked here** | +| 4 | **Dependency integrity** (anti-slopsquatting) | Existence-verify + hash-locked lockfile + new-import audit | Deterministic | **AI companion §6.4/§9** (pointer only) | +| 5 | **Security scanning + threat model** | Blocking SAST/SCA/secret-scan + written threat model + human review | Deterministic + advisory | **SDS PW.7 / AI companion §6.5–6.6** (pointer only) | +| 6 | **Published-artifact integrity** (supply-chain-*out*) | Published sdist/wheel ship **only intended content** — the package manifest is an *allowlist* (not a whole-repo sweep) + a fail-closed publish gate blocks any private/unintended file before the irreversible upload. Distinct from row 4's *incoming* dependency integrity | Deterministic | **This document — new** (+ SDS supply-chain) | + +**Tier 2 — Measurement / lower-signal layer (guidance & triage — never a gate on their own).** Useful for *surfacing* problems, but each is a weak or gameable predictor in isolation (§2, §4.1), so they inform review — they do not certify quality. + +| \# | Signal | What "good" looks like | Gate type | Owner | +|----|----|----|----|----| +| 7 | **Test-signal proof** (mutation) | Mutation testing on changed code exposes shallow tests | Advisory (guidance) | **This document — new** | +| 8 | **Coverage visibility** | Coverage *measured on the diff* as guidance — never a whole-repo % gate | Advisory | **This document — new** | +| 9 | **Duplication / reuse discipline** | Clone-detection flags *new* duplication in diffs; justified parity whitelisted | Advisory | **This document — new** | +| 10 | **Lint breadth** (static analysis in the loop) | Broad ruleset (bugbear, comprehensions, simplify, pyupgrade, isort) | Deterministic | **This document — expand existing** | +| 11 | **Complexity as triage** (never a gate) | Advisory complexity signal surfaces the genuinely-large units | Advisory | **This document — new** | + +> **Context caveat, not a signal — delivery stability (DORA).** "Velocity ≠ delivered quality" is a real AI-slop concern (DORA 2024 associated AI adoption with lower delivery *stability*), but it measures **delivery outcomes**, not whether a given diff is slop — a different altitude from the code-artifact signals above, on weaker (correlational, partly-revised — §7) evidence, and **owned by the [AI companion §3](Secure_AI_Development_Standards.md)**. MEFOR's small-batch discipline (one coherent layer per commit) already covers the actionable part. It stays a §3 failure-mode entry, **not** a peer signal here. +> +> **Evidence & citations for the matrix.** Every signal and claim above maps to its supporting study in [**Appendix B.3**](#b.3-evidence-behind-each-rubric-element) (per-element evidence table), with full bibliographic citations in [**Appendix B.4**](#b.4-references), the derivation method in [**Appendix B.2**](#b.2-how-the-matrix-was-derived), and the claims that *failed* verification in [**Appendix B.5**](#b.5-what-was-refuted-the-verification-worked). + +### 4.1 The anti-metric rule (hard) + +**Do NOT certify quality — or fail a build — on any single one of:** line-coverage %, LOC, raw or cognitive cyclomatic complexity, or SonarQube severity counts. Each is a weak or gameable predictor (§2). They may be *surfaced as advisory triage signals*; they must never be *the* quality gate. This mirrors the AI companion's "gates are deterministic checks, never ask the model to be secure" — here: *a scoreboard is never the verdict.* + +### 4.2 No validated single threshold (honest) + +The literature validates that single metrics fail, but supplies **no validated numeric cutoff** for a combined scorecard. Thresholds (e.g. a mutation-score floor on changed code, a max new-clone count) are therefore **set empirically per project and recorded in the Appendix**, reviewed as data accumulates — not imported as universal constants. Where this document names a directional target, it is flagged as project-set, not evidence-certified. + +------------------------------------------------------------------------ + +## 5. The new measurement gates — placement (local vs CI) and status + +The five gates this document adds (rubric rows 7–11) are *quality-measurement* gates. Placement follows the project's **three enforcement points** (identical to the AI companion §6.5): **pre-commit hook** (local), `messagefoundry check` (local + CI + IDE), and **GitHub Actions CI** (the authoritative gate, incl. self-hosted Windows runners). + +| Gate | Cost | Recommended placement | Blocking? | Taxonomy | +|----|----|----|----|----| +| **Advisory complexity triage** (`C901` via ruff) | Cheap | CI advisory job | **Advisory only** (never a hard gate — §4.1) | ✅ **Built** — `quality-advisory.yml` (#1028) | +| **Clone-detection** (jscpd) | Cheap | CI advisory job; store-parity whitelisted | Advisory (finding, not fail) | ✅ **Built** — `quality-advisory.yml` (#1028) | +| **Diff-coverage visibility** (`diff-cover`) | Moderate | CI leg (reports on changed lines) | Advisory | ✅ **Built** — `quality-advisory.yml` (#1040; PR-only) | +| **Mutation testing** (`mutmut`, bounded scope) | **Cheap — measured** (461 mutants in 3s; mutmut 3 runs only the tests covering each mutant) | CI leg (advisory), **including PRs** + opt-in local command | Advisory (guidance) | ✅ **Built** — `quality-advisory.yml` (#1040; repaired 2026-07-27, see v0.10) | +| **Expand ruff ruleset** (`B, C4, SIM, UP, I`) | Cheap | The required CI `ruff check` leg | Blocking (from a clean baseline) | ✅ **Built** — `pyproject.toml` extend-select (#1047) | + +**Answering "are these local?"** — Cheap gates (ruff breadth, `C901`) run **both** locally (pre-commit) and in CI. The expensive gates (mutation, clone, diff-coverage) are **CI-first** — they are too slow for the inner loop — but each is **exposable as an opt-in local command** so a maintainer can run it on demand. The *authoritative* enforcement is CI in every case; local runs are for fast feedback. This split is deliberate: it keeps the local edit-loop fast while the diff-scoped heavy analysis runs where wall-clock doesn't block the human. + +> **All five measurement gates are now Built.** Complexity (11) and clone (9) shipped first (#1028), then diff-coverage (8) and mutation (7) (#1040) — non-required advisory jobs in [`quality-advisory.yml`](../.github/workflows/quality-advisory.yml) that can never block a merge — and finally the **ruff-breadth expansion (signal 10, \#1047)**, which unlike the others *is* enforced by the required `ruff check` leg (from a grandfathered clean baseline; new code must comply). Nothing remains designed-but-deferred. + +------------------------------------------------------------------------ + +## 6. How this maps to the companion standards + +| This rubric | Companion anchor | +|----|----| +| Rows 1–2 (structure, typing) | SDS **PW.1–PW.2** (secure design, modularity); AI companion **§6.5** (verification gates) | +| Row 3 (behavior-verifying tests) | SDS **PW.8** (test executable code) — *this rubric adds the quality bar on top of presence* | +| Row 4 (dependency integrity) | AI companion **§6.4 / §9** — *owned there; checked here* | +| Row 5 (security review + SAST) | SDS **PW.7**; AI companion **§6.5–6.6** — *owned there; checked here* | +| Row 6 (published-artifact integrity) | **New** — this document (+ SDS supply-chain) | +| Rows 7–11 (measurement gates) | **New** — 7/8/9/11 Built (advisory) in `quality-advisory.yml`; 10 Built + enforced by the required `ruff check` leg (#1047) | +| Delivery stability (DORA) — *context caveat, not a signal* (§4) | AI companion **§3** (the METR / DORA calibration) | +| The anti-metric rule (§4.1) | The quality-side analogue of AI companion **§5** "gates are deterministic checks, never ask the model to be secure" | + +------------------------------------------------------------------------ + +## 7. Evidence caveats (carry these into every use) + +1. **Metric-invalidation findings are robust** (peer-reviewed primary studies) but are *"this metric is weak alone"* results — they argue against **single-number gates**, not against measurement. +2. **The GitClear duplication/churn trend is descriptively solid but the AI *causation* is correlational**, from a commercial vendor using a proprietary "moved" reuse heuristic, and confounded (2022–24 layoffs, a startup-growth effect). Treat the duplication *trend* as real; treat the AI attribution as interpretation. +3. **Several AI-code findings are model-era-specific.** The Stanford security study used codex-davinci-002 (2022); the package-hallucination figures skew to GPT-3.5-era output. **Frontier models plausibly perform better — re-baseline periodically.** +4. **The 14.4% Python hallucination figure is share of *distinct* modules**, not per-import rate (which is far lower — common modules are never hallucinated). +5. **DORA 2024's negative delivery effect is associational** and was partly revised in DORA 2025. +6. **No source provides a validated single-metric certification threshold** — the evidence supports composite, structural, guidance-based assessment (§4.2). + +------------------------------------------------------------------------ + +## 8. References + +*Quick links below. **Full bibliographic citations, the per-element evidence map, and the derivation method are in [Appendix B](#appendix-b-the-rubric-matrix-methodology-cited-references)** (B.4 / B.3 / B.2).* + +- **ISO/IEC 25010:2023** — Systems and software Quality Requirements and Evaluation (product-quality model). +- **Cyclomatic complexity vs. defects** — [arXiv 1912.01142](https://arxiv.org/pdf/1912.01142). +- **Cognitive Complexity has no incremental predictive value** — [JSS 2022](https://www.sciencedirect.com/science/article/abs/pii/S0164121222002370). +- **SonarQube technical-debt items weakly/invertedly related to faults** — [arXiv 1908.11590](https://arxiv.org/pdf/1908.11590). +- **Mutation score is a size artifact but valuable as guidance** — [ICSE 2018, Papadakis et al.](https://dl.acm.org/doi/pdf/10.1145/3180155.3180183). +- **AI assistants → less-secure, more-confident code** — [Stanford, CCS 2023, arXiv 2211.03622](https://arxiv.org/abs/2211.03622). +- **Package hallucination / slopsquatting (~14.4% distinct Python modules)** — [WildCode, arXiv 2512.04259](https://arxiv.org/pdf/2512.04259). +- **DORA 2024** — AI adoption vs. delivery stability/throughput — [dora.dev/research/2024](https://dora.dev/research/2024/dora-report/). +- **GitClear** — copy/paste vs. moved lines, churn — [GitClear AI Copilot Code Quality 2025](https://gitclear-public.s3.us-west-2.amazonaws.com/GitClear-AI-Copilot-Code-Quality-2025.pdf). +- **METR RCT (experienced devs 19% slower)** — cited via the [AI companion §3/§11](Secure_AI_Development_Standards.md). +- **Cross-links:** [Secure Development Standards](Secure_Development_Standards.md) · [Secure AI-Assisted Development Standards](Secure_AI_Development_Standards.md) · [`../CLAUDE.md`](../CLAUDE.md). + +------------------------------------------------------------------------ + +## Appendix A — Scorecard: MessageFoundry (MEFOR) + +*Scored from a read-only repo audit (2026-07-13). Future projects add Appendix B, C, … with identical headings. Each signal is tagged **Built / designed-but-deferred / aspirational**, per the house honesty taxonomy.* + +### A.1 Verdict + +**A− / low slop-risk.** MEFOR implements **all six durable, high-signal controls** (rubric rows 1–6) as **Built**, and its *measurement* layer has now closed as well: **complexity (11) and clone (9)** shipped as advisory gates (#1028), then **mutation (7) and diff-coverage (8)** (#1040), and finally the **ruff-breadth expansion (#10)** (#1047, enforced by the required `ruff check` leg). So **all 11 signals are now Built**. It is strong where faking is hardest (machine-enforced structure) and thin only where the metrics are gameable anyway. + +**The rubric earned its keep this cycle.** Applying **signal 6** (published-artifact integrity) surfaced a real **control-parity** gap (§3): the PyPI **sdist** was shipping the private security-posture docs on *every* release, because the fail-closed leak gate covered the git-mirror publish path but not its sibling, the PyPI path. It was fixed (#1020: a `[tool.hatch.build.targets.sdist]` allowlist + a fail-closed "sdist is package-only" gate in `release.yml`) and **verified clean at v0.3.0**. That found-and-fixed leak is the one *durable*-control gap that has now closed; the rest of the gaps are all in the measurement layer. + +### A.2 Scored signals (from the audit) + +**Tier 1 — durable, high-signal controls (signals 1–6): all Built ✅.** + +| \# | Signal | Status | Evidence in repo | +|----|----|----|----| +| 1 | Enforced architecture boundaries | ✅ **Built — Strong** | `tests/test_dependency_boundaries.py` AST-scans engine packages, blocks `fastapi`/`pyside6`/`api`/`console` imports, in the required CI `test` leg | +| 2 | Strict typing | ✅ **Built — Strong** | `[tool.mypy] strict = true`, dual-platform CI; all 33 `# type: ignore` + 100 `# noqa` carry rule codes; no blanket ignores | +| 3 | Tests verify behavior, not mocks | ✅ **Built — Strong** | 5,402 test functions; ~7,600 value-`==` asserts; ~1,000 `pytest.raises`; **0** `assert_called*`; live SQL Server + Postgres integration legs | +| 4 | Dependency integrity | ✅ **Built — Strong** | Hash-locked `requirements.lock` (DEP-1 lock-sync + `--require-hashes` CI); pip-audit; `CLAUDE.md`/AI-companion verify-before-add rule | +| 5 | Security scanning + threat model | ✅ **Built — Strong** *(caveat A.4)* | 11 scanners (CodeQL, semgrep, bandit, gitleaks, pip-audit, crypto-inventory, forbidden-content, Trivy, Scorecard, zizmor, npm-audit); SECURITY.md (735 ln) + PHI.md (688 ln) | +| 6 | Published-artifact integrity (supply-chain-*out*) | ✅ **Built — found & fixed this cycle** | Was 🔴: the PyPI **sdist** swept the whole repo, shipping `docs/security/*`, `CLAUDE.md`, `scripts/publish/*` on releases 0.1.0..0.2.15 (the mirror leak-gate never covered the PyPI path — a control-parity miss, §3). **Fixed \#1020:** `[tool.hatch.build.targets.sdist] only-include` + a fail-closed "sdist is package-only" gate in `release.yml`; **v0.3.0 verified package-only against the live PyPI artifact** (sha256 download). Historical 0.1.0..0.2.15 sdists remain public (owner-only PyPI deletion). | + +**Tier 2 — measurement / lower-signal layer (signals 7–11): all 5 Built (#7, \#8, \#9, \#11 advisory; \#10 enforced).** + +| \# | Signal | Status | Evidence in repo | +|----|----|----|----| +| 7 | Test-signal proof (mutation) | ✅ **Built (advisory)** — \#1040, **repaired 2026-07-27 (v0.10)** | `quality-advisory.yml` runs **`mutmut==3.6.0`** over one bounded, well-tested pure module (`parsing/binary.py` ↔ `test_binary_carriage.py`), on PRs + nightly cron + `workflow_dispatch`. **Measured: 461 mutants in 3s — 87 killed, 19 survived, 355 not covered by the scoped test.** Survivor table in the step summary. *Was scored Built on `mutmut<3` from 0.8 to 0.9 while producing nothing — see v0.10.* | +| 8 | Coverage visibility | ✅ **Built (advisory)** — \#1040, **surfaced 2026-07-27 (v0.10)** | `quality-advisory.yml` runs `pytest-cov` + `diff-cover` on the PR's changed lines (`--fail-under=0`), PR-only — coverage *of the diff*, never a whole-repo % gate (§4.1). Now emits **inline `::notice` annotations on the Files changed tab** (`--format github-annotations:notice`), adjacent uncovered lines coalesced into ranges. Advisory. | +| 9 | Duplication / reuse detection | ✅ **Built (advisory)** — \#1028 | `quality-advisory.yml` runs `jscpd` on `messagefoundry/`, whitelisting the ~21k-LOC justified store-backend parity (`sqlserver.py` / `postgres.py`); surfaces *un*justified copy-paste for triage, non-blocking | +| 10 | Lint breadth | ✅ **Built** — \#1047 | `[tool.ruff.lint] extend-select = ["B","C4","SIM","UP","I"]`, enforced by the required `ruff check` leg. B008 (FastAPI DI, ~460 hits) handled via `extend-immutable-calls` + a route-layer per-file ignore (real `x=list()` bugs still caught); **515 violations auto-fixed** (import sort, pyupgrade, safe simplify); **235 non-auto-fixable grandfathered** with per-line `# noqa` → clean baseline, new code must comply | +| 11 | Complexity triage | ✅ **Built (advisory)** — \#1028, **sharpened 2026-07-27 (v0.10)** | `quality-advisory.yml` runs `ruff --select C901 --exit-zero` (advisory, never gates), **plus a merge-base-vs-HEAD delta** (`scripts/quality/c901_delta.py`) that reports only functions a PR *introduced* or *made worse*. **Re-measured 2026-07-27: 122 functions exceed** `C901`**\>10** across 43 files (was 85 on 2026-07-13), complexity 11 / 14 median / 320 max. The raw list is unusable as a diff signal — all 122 findings anchor on a single `def` line — which is what the delta exists to fix | + +### A.3 The gaps, ranked → buildable gates + +Ordered by anti-slop leverage, not effort (build placement per §5). **✅ = shipped** (advisory; \#1028 or \#1040): + +1. **Mutation testing** — highest leverage; directly counters shallow-test slop, extra weight under the solo-maintainer review deviation (A.4). ✅ **shipped** (#1040 — `mutmut` over a bounded, well-tested module, mirror-nightly + `workflow_dispatch`; widen the scope later). +2. **Clone-detection on diffs** — ✅ **shipped** (`jscpd`, store-parity whitelisted) — catches the copy-instead-of-abstract signature the parallel-worktree workflow is most exposed to. +3. **Diff-coverage visibility** — measured on changed lines, guidance only (never a whole-repo % gate — §4.1). ✅ **shipped** (#1040 — `pytest-cov` + `diff-cover`, PR-only). +4. **Advisory** `C901` **complexity** — ✅ **shipped** (advisory triage). +5. **Expand ruff** `select` (`B, C4, SIM, UP, I`) — ✅ **shipped** (#1047 — extend-select enforced by the required `ruff check` leg; 515 auto-fixed, 235 grandfathered from a clean baseline). + +*All gates are now shipped.* The ruff sweep (#10, \#1047) was run in a quiescent-worktree window (a 100+-file import sort would collide with in-flight parallel sessions) after pruning the stale worktrees to a minimal set. Mutation and diff-coverage were built *blind via CI* — verified by their own gate runs, since this repo's sessions can't stand up a local venv (see \#1040). + +**Rollout record (measured 2026-07-13 — how the \#1047 sweep was executed):** `B,C4,SIM,UP,I` = **853 violations** (238 `B008` FastAPI false positives to exclude; 111 `I001` repo-wide import reorder); `C901` = **85 hits**. Safe rollout: (a) exclude framework-idiom rules (`B008` on `api/`); (b) **grandfather** the existing backlog so the *required* gate stays green (per-file-ignores / ratchet — new code only); (c) run the repo-wide import sort as a **dedicated pass when parallel worktrees are quiescent** — a 100+-file sweep conflicts with in-flight sessions; (d) keep `C901` **advisory**. (The built coverage/mutation gates install their tools CI-side via an ephemeral `uv pip install`, so they needed **no** `requirements.lock` change — DEP-1 unaffected.) + +### A.4 Documented caveat — solo-maintainer review + +Row 5's "human review" is **self-review** (the [SDS §A.6](Secure_Development_Standards.md#a6-documented-deviations) / [AI companion Appendix A.6](Secure_AI_Development_Standards.md#a6-documented-deviations) single-maintainer deviation). The Stanford overconfidence finding (§3) bites hardest exactly when the author reviews their own AI-authored code — which is the strongest argument for the mutation gate (Built this cycle — \#1040), since it is the one control that *adversarially* checks whether the tests assert anything, independent of the author's confidence. + +------------------------------------------------------------------------ + +## Appendix B — The rubric matrix: methodology & cited references + +*This appendix explains how to read the §4 signal matrix, how it was derived, and the evidence behind each element — with full citations. The §2, §3, and §4 tables link here.* + +### B.1 What "the matrix" is and how to read it + +The rubric's core is the **§4 matrix**: 11 signals, each a **risk → control → measure**, tagged by **gate type** (deterministic = machine-checked; advisory = human arbitrates) and by which document **owns** it. Read it as a **composite**, not a checklist of independent boxes: + +- **Rows 1–6 are durable, high-signal controls** — enforced structure (ISO/IEC 25010 modularity), strict typing, behavior-verifying tests, dependency integrity, security scanning, and published-artifact integrity. These are where quality is *hard to fake*. +- **Rows 7–11 are the measurement / lower-signal layer** — mutation, coverage, clone-detection, lint breadth, complexity — useful as *guidance / triage*, never as a single gate. +- **The anti-metric rule (§4.1)** forbids certifying quality on any one number (coverage %, LOC, raw or cognitive complexity, SonarQube severity), because the evidence shows each is a weak or gameable predictor (B.3). +- **Delivery stability (DORA) is deliberately *not* a signal** — it measures delivery outcomes, not the code artifact, on weaker evidence; it is kept as a context caveat under the §4 table. + +A codebase is judged "not slop" by the **composite** of the durable controls plus the guidance signals — with thresholds set **empirically per project** (§4.2), because no source validates a universal single-metric cutoff. + +### B.2 How the matrix was derived + +The matrix is **evidence-informed and adversarially verified**, not authored from opinion: + +1. **Adversarially-verified deep-research pass.** The question was decomposed into **five search angles** — (a) academic metric-validity, (b) quality frameworks & delivery metrics, (c) AI-slop empirical trends, (d) security & correctness studies, (e) practitioner controls. Parallel searches fanned out → ~24 sources fetched → ~109 candidate claims → the load-bearing ones put through **3-vote adversarial verification** (each verifier tried to *refute* the claim; ≥2 refutations killed it). Result: **20 confirmed, 5 refuted** (B.5). Only survivors entered the rubric. +2. **Structural scaffold — ISO/IEC 25010:2023** [\[R1\]](#r1): the international product-quality model, whose *maintainability* characteristic (modularity, reusability, analyzability, modifiability, testability) supplies the "structure over scoreboards" backbone (signals 1–6). +3. **The MEFOR scorecard (Appendix A)** is a separate, read-only audit of the actual repository — evidence, not estimates. +4. **Honesty discipline (§7)** — every claim carries its limitation; correlational, vendor-sourced, and model-era-specific evidence is labelled as such. + +### B.3 Evidence behind each rubric element + +| Rubric element | What the evidence establishes | Reference | +|----|----|----| +| **Structure over scoreboards** (signals 1–6) | Product quality is dominated by *maintainability* = modularity / low coupling / information hiding — a structural property, not a metric score | [\[R1\]](#r1) ISO/IEC 25010:2023 | +| **Anti-metric rule (§4.1): raw complexity is weak** (bounds signal 11) | Cyclomatic / path / NPATH complexity correlate only weakly-to-moderately with real bugs; complexity is a *triage smell*, not a gate | [\[R2\]](#r2) Chen 2019 | +| **Anti-metric rule: Cognitive Complexity adds nothing** | SonarSource's Cognitive Complexity gives **no incremental** predictive value over traditional measures | [\[R3\]](#r3) Lavazza et al. 2023 | +| **Anti-metric rule: SonarQube severities are weak / inverted** | Over 33 Apache projects, "dirty" classes are **no more fault-prone** than clean ones; effects small, sometimes inverted | [\[R4\]](#r4) Lenarduzzi et al. 2020 | +| **Signal 7 — mutation as guidance** (why coverage % hides shallow tests) | Mutation *score* is a poor linear proxy (suite-size artifact), but top-decile suites catch **8–46% more real faults** — it exposes assertion-free tests that coverage % hides | [\[R5\]](#r5) Papadakis et al. 2018 | +| **Signal 5 / §3 — insecure code + overconfidence** (mandates human review) | AI-assistant users wrote **less-secure** code yet were **more confident** it was secure | [\[R6\]](#r6) Perry et al. 2023 | +| **Signal 4 — hallucinated dependencies** (slopsquatting) | LLM output frequently references **nonexistent packages** (~14.4% of distinct Python modules) → verify-before-add + hash-locked lockfile | [\[R7\]](#r7) Khanmohammadi et al. 2025 | +| **Signal 9 / §3 — silent duplication over reuse** | 2024 was the first year copy/pasted lines exceeded refactored ("moved") lines — the copy-instead-of-abstract signature *(vendor-sourced, correlational — §7)* | [\[R9\]](#r9) GitClear 2025 | +| **§3 — velocity ≠ delivered quality** (the DORA context caveat) | AI adoption associated with **lower delivery stability** (2024, correlational, partly revised); experienced devs measured **~19% slower** with early-2025 AI tooling | [\[R8\]](#r8) DORA 2024 · [\[R10\]](#r10) METR 2025 | + +### B.4 References + +**\[R1\]** ISO/IEC 25010:2023. *Systems and software engineering — Systems and software Quality Requirements and Evaluation (SQuaRE) — Product quality model.* International Organization for Standardization, 2023. + +**\[R2\]** Chen, C. (2019). *An Empirical Investigation of Correlation between Code Complexity and Bugs.* arXiv:1912.01142 \[cs.SE\]. + +**\[R3\]** Lavazza, L., Abualkishik, A. Z., Liu, G., & Morasca, S. (2023). *An Empirical Evaluation of the "Cognitive Complexity" Measure as a Predictor of Code Understandability.* Journal of Systems and Software, 197, 111561. + +**\[R4\]** Lenarduzzi, V., Saarimäki, N., & Taibi, D. (2020). *Some SonarQube issues have a significant but small effect on faults and changes: A large-scale empirical study.* Journal of Systems and Software, 170, 110750. + +**\[R5\]** Papadakis, M., Shin, D., Yoo, S., & Bae, D.-H. (2018). *Are Mutation Scores Correlated with Real Fault Detection? A Large Scale Empirical Study on the Relationship Between Mutants and Real Faults.* ICSE 2018. + +**\[R6\]** Perry, N., Srivastava, M., Kumar, D., & Boneh, D. (2023). *Do Users Write More Insecure Code with AI Assistants?* ACM CCS 2023. arXiv:2211.03622. + +**\[R7\]** Khanmohammadi, K., Roy, P., Khoury, R., Hamou-Lhadj, A., Konan, W. P., Da Re, A., & Rebelo Melo, N. (2025). *WildCode Revisited: A Comprehensive Empirical Study on the Security of LLM-Generated Code.* arXiv:2512.04259 \[cs.CR\]. — the ~14.4% figure is the share of *distinct* Python modules; per-reference rate is far lower (§7). + +**\[R8\]** Google Cloud / DORA (2024). *Accelerate State of DevOps Report 2024.* — AI adoption vs. delivery stability/throughput; associational, partly revised in 2025. + +**\[R9\]** GitClear (2025). *AI Copilot Code Quality 2025* ("AI-Generated Code Exerts Downward Pressures on Code Quality"). — commercial vendor; proprietary "moved" reuse heuristic; AI attribution correlational (§7). + +**\[R10\]** METR (2025). *Measuring the Impact of Early-2025 AI on Experienced Open-Source Developer Productivity.* arXiv:2507.09089. — 16 experienced devs ~19% slower while believing they were faster. + +### B.5 What was refuted (the verification worked) + +Five widely-circulated claims **failed** 3-vote adversarial verification and are **deliberately not** in the rubric — the filtering is part of the basis: + +- **"~40% of GitHub Copilot programs contain security vulnerabilities."** An over-simplified reading of the NYU "Asleep at the Keyboard" study (Pearce et al., IEEE S&P 2022, arXiv:2108.09293); the headline percentage did not survive scrutiny. *(The study is real; the "40%" framing is not a supported rubric claim.)* +- **A "10× surge in duplicate blocks" (2022→2024)** — GitClear figure, refuted 0–3. +- **"Copy/pasted lines rose 8.3%→12.3% (~48%)"** and **"moved operations fell 17% (2021→2023)"** — GitClear / secondary-blog percentages that did not reconcile, refuted 0–3. + +Because the refuted claims came mostly from the *AI-slop-trend* angle (vendor telemetry, secondary blogs), the rubric leans hardest on the **peer-reviewed metric-validity studies** (\[R2\]–\[R6\]) and treats the trend data (\[R8\], \[R9\], \[R10\]) as caveated context (§7). + +### B.6 Caveats + +The evidence caveats in **§7** are part of this appendix's basis: the metric-invalidation findings are robust peer-reviewed results, but they are "this-metric-is-weak-*alone*" results (they argue against single-number gates, not against measurement); the AI-slop trend data is correlational / vendor-sourced / model-era-specific; and no source supplies a validated single-metric certification threshold. + +------------------------------------------------------------------------ + +## Version history + +| Version | Date | Change | +|----|----|----| +| 0.10 | July 27, 2026 | **Restored to the repo, and corrected three claims that did not survive measurement.** This file had been absent from the repository's entire git history despite being cited by `quality-advisory.yml` and `pyproject.toml`; it is restored here from the maintained copy. Corrections, each measured rather than reasoned: **(a) Signal 7 was scored ✅ Built in 0.8 and 0.9 while producing nothing.** `mutmut<3` resolved to 2.5.1, which crashes on Python 3.14 in its pony-ORM cache (`cannot pickle 'itertools.count'`) *before generating a single mutant*; `\|\| true` made the job report success in 37s, so the gate looked green for two versions. Repaired on `mutmut==3.6.0` (+ `pytest-timeout`, and `source_paths` must be the package, not the one file, or the mutant copy cannot import `conftest`). Now genuinely measured: **461 mutants, 87 killed, 19 survived, 3 seconds** — so the "Expensive / never per-PR" cost model in §5 was also wrong, and mutation now runs on PRs. **(b) Signal 11's "85 functions over C901>10" is now 122 across 43 files**, and the raw list was found unusable as a diff signal (every finding anchors on one `def` line), so a merge-base delta was added that reports only PR-caused changes. **(c) Signal 8 now emits inline PR annotations** rather than console-only output. The A− verdict stands, but note that (a) is exactly the failure mode this rubric exists to catch — an advisory gate that reports success while measuring nothing — and it was caught by re-verification, not by the gate itself. | +| 0.9 | July 14, 2026 | **Restatused signal 10 (lint breadth) to ✅ Built — all 11 signals now Built.** The `extend-select = [B,C4,SIM,UP,I]` sweep shipped (#1047): B008 handled via `extend-immutable-calls` + a route-layer per-file ignore, 515 auto-fixed, 235 grandfathered with `# noqa`, enforced by the required `ruff check` leg. Flipped the exec verdict, §5 gate table + callout, §6 map, and Appendix A.1 / A.2 (row 10 + Tier-2 roll-up) / A.3 (gaps list + "remaining gate" prose → rollout *record*). No scoring change (A− stands). | +| 0.8 | July 14, 2026 | **Restatused signals 7 (mutation) + 8 (diff-coverage) to ✅ Built.** Both shipped as advisory jobs in `quality-advisory.yml` (#1040) — mutation over a bounded module (mirror-nightly + `workflow_dispatch`), diff-coverage on the diff's changed lines (PR-only). Flipped every place that called them deferred: the exec verdict, §5 gate table + callout, §6 map, and Appendix A.1 / A.2 (rows 7–8 + the Tier-2 roll-up) / A.3 (gaps list + DEP-1 note) / A.4. **Only signal 10 (lint breadth) remains designed-but-deferred → 10 of 11 signals now Built.** No scoring change (A− stands). | +| 0.7 | July 13, 2026 | **Tiered the Appendix A scorecard to match §4** — split A.2 into a Tier 1 table (durable, signals 1–6, all Built) and a Tier 2 table (measurement, signals 7–11), each with a per-tier status roll-up. Presentational only; no scoring change. | +| 0.6 | July 13, 2026 | **Renumbered signals contiguously by tier** (per owner): Tier 1 durable = **1–6** (published-artifact 11→6), Tier 2 measurement = **7–11** (mutation 6→7, coverage 7→8, clone 8→9, lint 9→10, complexity 10→11). Updated every cross-reference — §3, §4, §5, §6, exec summary, scope, and Appendix A.1 / A.2 (scorecard reordered) / A.3 / B.1 / B.2 / B.3. The `[R1]–[R10]` reference IDs are unchanged (they're citations, not signals). | +| 0.5 | July 13, 2026 | **Split the §4 matrix into two tiers** for clarity: **Tier 1 — durable, high-signal controls** (signals 1–5 + 11) and **Tier 2 — measurement / lower-signal layer** (signals 6–10). Signal numbers kept **stable** (they are IDs referenced across Appendix A + B), so grouped by tier rather than renumbered. No change to content, evidence, or the scorecard. | +| 0.4 | July 13, 2026 | **Added Appendix B — the rubric matrix's methodology & cited references** (B.1 how to read the matrix, B.2 derivation, B.3 per-element evidence map, B.4 full citations \[R1–R10\], B.5 the 5 refuted claims). **Annotated the rubric with links to it:** §2 + §3 evidence cells now carry `[Rn]` citation links, §4 has an evidence/citations pointer, and §8 points to the full bibliography. Fetched-verified author/title details for the academic references. | +| 0.3 | July 13, 2026 | **Built two gates + demoted DORA.** Complexity (signal 10) and clone (signal 8) shipped as advisory jobs in `quality-advisory.yml` (#1028) — scorecard restatused to ✅ Built (§5, A.2, A.3). **Demoted delivery-stability (DORA) from a peer signal to a context caveat** (it measures delivery outcomes, not the code artifact; weak/correlational evidence; owned by the AI companion) → back to **11 signals** (published-artifact renumbered 12→11). Mutation (#6) + diff-coverage (#7) deferred to a local-venv session; ruff-breadth (#9) to a quiescent-worktree sweep. | +| 0.2 | July 13, 2026 | Added **signal 12 — published-artifact integrity** (supply-chain-*out*) and the **control-parity** failure mode (§3), after the rubric's own application surfaced a private-doc leak in the published PyPI sdist (fixed \#1020, verified clean at v0.3.0 — Appendix A.1 + A.2 row 12). Scorecard refreshed (12 signals). | +| 0.1 | July 13, 2026 | Initial rubric. Evidence base from an adversarially-verified deep-research pass (20 confirmed / 5 refuted claims) + a read-only MEFOR code-quality audit. Establishes the anti-metric rule (§4.1), the 11-signal composite rubric (§4), the five new measurement gates with local-vs-CI placement (§5), and the MEFOR scorecard (Appendix A, verdict A−). Recorded as the third companion to the SDS + Secure AI-Assisted Development Standards. | diff --git a/docs/quality-gates/HANDOFF-mutation-coverage.md b/docs/quality-gates/HANDOFF-mutation-coverage.md index 524ba4b2..990d9850 100644 --- a/docs/quality-gates/HANDOFF-mutation-coverage.md +++ b/docs/quality-gates/HANDOFF-mutation-coverage.md @@ -234,10 +234,12 @@ Add the cron to the workflow's `on:` block, then the job: ## 5. After they're green — flip the rubric -> **UNACTIONABLE as written (2026-07-27).** `docs/Code_Quality_Standards.md` has never existed in this -> repo's git history, despite being cited by `quality-advisory.yml`'s header and `pyproject.toml`. There is -> no rubric file to flip. Leave the existing dangling citations alone and do not add new ones; if the rubric -> is ever written, redo this step then. +> **NOW ACTIONABLE — and done (2026-07-27).** `docs/Code_Quality_Standards.md` had never existed in this +> repo's git history despite being cited by `quality-advisory.yml`'s header and `pyproject.toml:246`. It was +> restored from the maintained copy at **v0.10**, which also carries the corrections this cycle produced: +> signal 7 had been scored ✅ Built since v0.8 while its tool crashed before generating a mutant, signal 11's +> "85 functions over C901>10" is now 122 across 43 files, and signal 8 now emits inline PR annotations. The +> citations resolve again. Update [Code_Quality_Standards.md](../Code_Quality_Standards.md) exactly as #10/#8 were flipped in v0.3: signals 6 + 7 go from *deferred* → ✅ **Built (advisory)** in **§5** (status column), **Appendix A.2** (rows @@ -257,19 +259,25 @@ behaviour on fork PRs. The workflow still holds **no write scope on any job**. unshippable as a diff signal: all 122 findings on this tree anchor on a single `def` line, so body edits produce nothing and signature edits fire on pre-existing debt. The delta reports only what the PR caused. - **Clones** — step summary only. jscpd emits one location per clone pair chosen by scan order. -- **Mutation** — **the signal is dead and was reporting success anyway.** Measured from the scheduled run - `30248096425` (2026-07-27): the job takes **37 seconds** and goes green because `mutmut run || true` - swallows a crash. mutmut 2.5.1 prints its banner, runs the baseline, then dies in its pony-ORM cache with - `TypeError: cannot pickle 'itertools.count' object` (`cache.py:369 update_line_numbers`) — **before - generating a single mutant**. The job now captures the exit status, emits a `::warning`, and says so in - the summary rather than presenting a results block for a tool that produced none. The `.mutmut-cache` - artifact still uploads (`include-hidden-files: true` — it is a dotfile and `upload-artifact` skips hidden - files by default), but expect it to be a stale/empty cache until the tool works. - -**The real blocker (supersedes the old "measure the wall-clock" step):** the PR-vs-nightly question cannot -be answered while the tool produces nothing — 37s is the cost of crashing, not of mutating. Fix mutmut -first (mutmut 3.x has a different CLI; cosmic-ray is the other candidate), *then* measure a real run, then -decide about PRs. Do not read the current duration as evidence that a PR run would be cheap. +- **Mutation** — **was dead, now repaired and running.** The signal had been reporting success while + measuring nothing: from scheduled run `30248096425` (2026-07-27) the job went green in 37 seconds because + `mutmut run || true` swallowed a crash — mutmut 2.5.1 dies in its pony-ORM cache with `TypeError: cannot + pickle 'itertools.count' object` (`cache.py:369`) **before generating a single mutant**. Repaired on + **`mutmut==3.6.0`**, verified on Linux/Python 3.14: **461 mutants, 87 killed, 19 survived, 355 not covered + by the scoped test — in 3 seconds.** The step summary now carries that table plus the survivor list, and + a non-zero `mutmut run` emits a `::warning` instead of passing silently. + + Three things are load-bearing in the mutmut 3 config, each found by a run that produced nothing: + `source_paths` must be the **package** (mutmut 3 copies it into `mutants/` and runs pytest there; with a + single file copied, `conftest.py` cannot import `messagefoundry.config` and every mutant returns "not + checked"); `only_mutate` supplies the bounded scope instead; and **`pytest-timeout` must be installed** + because mutmut 3 always passes `--timeout` to pytest. + +**Mutation-on-PR: DECIDED — yes, it now runs on pull requests.** The old "measure the wall-clock first" step +is answered: the mutating itself costs ~3 seconds, so the previous "expensive, never per-PR" cost model was +a property of mutmut 2's run-the-suite-per-mutant design, not of this scope. The job's real cost on a PR is +its install step, in line with the other jobs here. Survivors are most useful in review, which is where +someone is already looking at the test that failed to kill them. ---