feat(quality): the username-as-access-key screen now runs, and can fail on a new site (BACKLOG #1226)

.github/workflows/ci.yml

@@ -371,6 +371,18 @@ jobs:
         if: runner.os == 'Linux'
         run: python scripts/quality/control_char_check.py
 
+      # BACKLOG #1226. The screen shipped and NOTHING RAN IT, so it read as coverage and produced
+      # none -- a username reaching a WHERE clause was found three times by accident and zero times
+      # on purpose. --baseline is what makes this step able to FAIL without turning the screen into
+      # a verdict-emitter: it is silent on the judged sites and red on a key nobody has read yet.
+      # Several judged entries are deliberately CORRECT code and one is a confirmed defect, so the
+      # baseline records that a human looked, never that a site is fine. stdlib only.
+      - name: Username-as-access-key screen (new sites only)
+        if: runner.os == 'Linux'
+        run: >-
+          python scripts/quality/username_access_key_screen.py
+          --baseline scripts/quality/username_access_key_baseline.txt
+
       # Two mypy passes on Linux so BOTH platforms' typed branches are checked without paying for a
       # Windows mypy: the default linux-platform run, plus an explicit --platform win32 run that types
       # the sys.platform=='win32' branches (ctypes.windll / DPAPI / service_control) the linux run

.pre-commit-config.yaml

@@ -229,6 +229,16 @@ repos:
         pass_filenames: true
         files: \.(py|ps1|sh|ts|js|go|md|toml|ya?ml|json|cfg|ini|txt)$
 
+      # BACKLOG #1226. always_run with pass_filenames FALSE: the screen walks its own DEFAULT_SCOPE,
+      # and handing it a changed-file list would silently narrow what it looked at while still
+      # printing a confident total -- the shape this screen exists to catch, in its own wiring.
+      - id: username-access-key
+        name: username-as-access-key screen (new sites only)
+        entry: python scripts/quality/username_access_key_screen.py --baseline scripts/quality/username_access_key_baseline.txt
+        language: system
+        pass_filenames: false
+        always_run: true
+
   # Secret scanning — blocks a commit that would introduce a credential/token/key.
   # (rev is verified by `pre-commit autoupdate` on first install; a wrong tag fails loudly there,
   # never silently in CI. Run it TARGETED -- `pre-commit autoupdate --repo

scripts/quality/username_access_key_baseline.txt

@@ -0,0 +1,63 @@
+# Judged username-as-access-key sites (BACKLOG #1226).
+#
+# A KEY IN THIS FILE RECORDS THAT A HUMAN HAS LOOKED AT THE SITE. It does NOT record that the site is
+# correct: one entry below is a CONFIRMED DEFECT that cannot be fixed inside this item. Treating the
+# baseline as an approval list would turn a screen the item explicitly designed to emit CANDIDATES
+# into the verdict-emitter it forbids.
+#
+# NO LINE NUMBERS, BY CONSTRUCTION. The key is `file::callee::slot`, because the same site is
+# `create_search_preset` or `upsert_search_preset` depending where you stand, at different lines
+# depending on your base. `Candidate.key` was written stable for this and went unused until now.
+#
+# A NEW KEY FAILS THE RUN. That is the whole point of wiring: read the site, decide, then add it here
+# with a reason. Adding a key without reading one is the only way to defeat this, and it is visible in
+# review because every line carries its reasoning.
+
+# ---- CORRECT BY DESIGN: the username is a display label, not a key -------------------------------
+
+# The item names this one itself: the username is deliberately the label while `uploader_id` carries
+# the access key. A screen that auto-classified would be wrong on its own author's code.
+app.py::save::uploader
+
+# Response-model fields. These SHOW a name to the caller; nothing is scoped by them.
+auth_routes.py::CurrentUser::username
+auth_routes.py::UserSummary::username
+auth_routes.py::UserPermissions::username
+
+# ---- CORRECT BY DESIGN: the username IS the subject of the operation ------------------------------
+
+# Looking a user up BY NAME is the function's entire purpose; there is no other key to use yet.
+auth_routes.py::get_user_by_username::arg0
+
+# Policy checks against the PROPOSED name, before any account exists to have an id.
+auth_routes.py::password_violations::username
+auth_routes.py::create_local_user::username
+
+# ---- CONFIRMED INSTANCE, ROUTED ONWARD, NOT FIXED HERE -------------------------------------------
+
+# `GET /me/security-events` -> AuthService.security_events_for(identity.username) ->
+# store.security_events_for_user(username), whose SQL is:
+#     SELECT ts, action, detail FROM audit_log WHERE actor = ? AND action LIKE 'auth.%'
+#
+# The username is not labelling a row here -- it IS the WHERE clause that decides which rows the
+# caller may see, which is this item's definition of the defect verbatim.
+#
+# The PRECEDING route handler in the same file, `my_sessions`, scopes on an immutable id --
+# `list_sessions(identity.user_id)`. That contrast is real but it is NOT evidence of an oversight:
+# the sessions table HAS a user id and `audit_log` does not. Measured on main: 9 `FROM audit_log`
+# sites in store.py, ZERO keyed on `user_id`, against a positive control of 1 keyed on `actor = ?`.
+# THE AUTHOR HAD NO IMMUTABLE KEY AVAILABLE AT THIS SITE.
+#
+# CONDITIONAL SEVERITY (CLAUDE.md section 0 -- zero deployments, nothing is exposed today): on a first
+# deployment, an operator who deleted a user and recreated the same username would hand the new
+# account holder the previous holder's sign-ins, lockouts and password changes.
+#
+# WHY IT IS NOT FIXED IN THIS ITEM, measured rather than assumed: `audit_log` has NO immutable key to
+# re-key onto. Its columns are id/ts/actor/action/channel_id/detail/client/row_hash -- `actor TEXT`
+# is the only identity, and a repo-wide grep for `user_id` against the audit store returns ZERO
+# against a positive control of 3 for `actor`. So the fix is a schema change to a table carrying a
+# `row_hash` TAMPER-EVIDENCE CHAIN, which is a different piece of work with different risks.
+#
+# Named as a subject, not a number, because no number is allocated for it: "the self-service
+# security-events view scopes on a reassignable username and audit_log has no immutable actor id".
+auth_routes.py::security_events_for::arg0

scripts/quality/username_access_key_screen.py

@@ -242,7 +242,34 @@ def screen_source(path: str, source: str) -> tuple[list[Candidate], list[Candida
     return candidates, labels
 
 
+#: Judged sites, one ``file::callee::slot`` per line. A key records that a HUMAN HAS LOOKED, never
+#: that the site is correct -- several judged entries are deliberately correct code, and one is a
+#: known defect awaiting its own item. Keys carry NO LINE NUMBER on purpose: the same site is
+#: `create_search_preset` or `upsert_search_preset` depending where you stand, at different lines
+#: depending on your base, which is why ``Candidate.key`` was built stable and unused until now.
+DEFAULT_BASELINE = _ROOT / "scripts" / "quality" / "username_access_key_baseline.txt"
+
+
+def load_baseline(path: Path) -> set[str]:
+    out: set[str] = set()
+    for raw in path.read_text(encoding="utf-8").splitlines():
+        line = raw.split("#", 1)[0].strip()
+        if line:
+            out.add(line)
+    return out
+
+
 def main(argv: list[str]) -> int:
+    # --baseline turns the screen from an advisory report into a step that can FAIL, WITHOUT making
+    # it a verdict-emitter. It fails only on a key nobody has judged yet. Wiring it without this
+    # would install a CI step that cannot fail for the reason it exists -- decoration that reads as
+    # coverage, which is the class this screen was written to catch.
+    baseline_path: Path | None = None
+    if "--baseline" in argv:
+        i = argv.index("--baseline")
+        baseline_path = Path(argv[i + 1]) if i + 1 < len(argv) else DEFAULT_BASELINE
+        argv = argv[:i] + argv[i + 2 :] if i + 1 < len(argv) else argv[:i]
+
     paths = argv or [str(_ROOT / p) for p in DEFAULT_SCOPE]
     all_candidates: list[Candidate] = []
     label_total = 0
@@ -272,7 +299,31 @@ def main(argv: list[str]) -> int:
         "  These are CANDIDATES, not defects. A username is correct as a label on an audit row "
         "and wrong as a key that scopes a resource; only a reader can tell which this is."
     )
-    return 0
+
+    if baseline_path is None:
+        return 0
+
+    judged = load_baseline(baseline_path)
+    seen = {f"{c.path}::{c.key}" for c in all_candidates}
+    unjudged = sorted(seen - judged)
+    resolved = sorted(judged - seen)
+
+    # STATED EVEN WHEN EMPTY. A baseline entry that no longer matches is either a fixed site or a
+    # screen that stopped seeing it, and those are opposite facts; printing the count is what lets a
+    # reader notice the second one.
+    print(f"  baseline: {len(judged)} judged, {len(seen)} seen, {len(resolved)} no longer reported")
+    for key in resolved:
+        print(f"    NO LONGER REPORTED (fixed, or the screen stopped seeing it): {key}")
+    if not unjudged:
+        return 0
+    print("")
+    print(
+        "username-access-key: NEW UNJUDGED SITE(S) -- read each, then add its key to the baseline:"
+    )
+    for key in unjudged:
+        print(f"    {key}")
+    print(f"  baseline file: {baseline_path}")
+    return 1
 
 
 if __name__ == "__main__":

tests/test_username_access_key_screen.py

@@ -23,6 +23,12 @@ class this screen exists to catch.
     ``save(uploader=identity.username)`` where the username is deliberately the display label and
     a sibling ``uploader_id`` carries the key. The screen therefore exits 0 whatever it finds, and
     a test asserting "zero hits on correct code" would be unsatisfiable by design.
+
+**AMENDED (#1226 wiring): that remains exactly true of the DEFAULT invocation, and ``--baseline``
+does not weaken it.** Baseline mode fails only on a key nobody has JUDGED yet -- never on a
+candidate being present -- so the screen still emits candidates rather than verdicts. The baseline
+records that a human read the site; one of its entries is a confirmed defect, which is precisely
+why it must not be read as an approval list.
 """
 
 from __future__ import annotations
@@ -146,3 +152,100 @@ def test_the_live_api_scope_still_surfaces_the_unjudged_candidate() -> None:
     assert proc.returncode == 0, proc.stderr
     assert "security_events_for(arg0=...)" in proc.stdout, proc.stdout
     assert "audit-label site(s) excluded as correct" in proc.stdout
+
+
+# ---------------------------------------------------------------------------------------------
+# BACKLOG #1226, the wiring limb. The screen shipped and NOTHING INVOKED IT, so it read as
+# coverage and produced none. Measured before the fix, with a control:
+#     git grep -l username_access_key origin/main -> BACKLOG.md, the screen, 2 test files
+#     git grep -l control_char_check  origin/main -> ci.yml AND .pre-commit-config.yaml
+# A wired screen shows up in those two files. This one did not.
+# ---------------------------------------------------------------------------------------------
+
+BASELINE = ROOT / "scripts" / "quality" / "username_access_key_baseline.txt"
+CI = ROOT / ".github" / "workflows" / "ci.yml"
+PRECOMMIT = ROOT / ".pre-commit-config.yaml"
+
+
+def _run(*args: str) -> subprocess.CompletedProcess[str]:
+    return subprocess.run(
+        [sys.executable, str(SCREEN), *args],
+        capture_output=True,
+        text=True,
+        cwd=str(ROOT),
+        check=False,
+    )
+
+
+def test_the_live_baseline_covers_every_site_the_screen_reports() -> None:
+    """MUST NOT FIRE, and it is the arm that keeps the wiring honest.
+
+    If this reds, a new username-as-access-key site has appeared and nobody has read it yet. That
+    is the entire purpose of the step; the fix is to read the site and record a judgement, never to
+    delete the arm."""
+    proc = _run("--baseline", str(BASELINE))
+    assert proc.returncode == 0, f"unjudged site(s) present:\n{proc.stdout}"
+
+
+def test_an_unjudged_site_makes_the_step_FAIL(tmp_path: Path) -> None:
+    """MUST FIRE. Without this the wiring would install a step that cannot fail for the reason it
+    exists -- decoration that reads as coverage, which is the class this screen was written to
+    catch, reproduced in its own gate."""
+    thinned = tmp_path / "baseline.txt"
+    kept = [
+        ln
+        for ln in BASELINE.read_text(encoding="utf-8").splitlines()
+        if not ln.strip().startswith("auth_routes.py::security_events_for")
+    ]
+    assert len(kept) < len(BASELINE.read_text(encoding="utf-8").splitlines()), (
+        "the line this arm removes is absent -- the mutation would be a no-op reporting success"
+    )
+    thinned.write_text("\n".join(kept), encoding="utf-8")
+
+    proc = _run("--baseline", str(thinned))
+    assert proc.returncode == 1, f"an unjudged site did not fail the step:\n{proc.stdout}"
+    assert "NEW UNJUDGED SITE" in proc.stdout
+    assert "security_events_for" in proc.stdout
+
+
+def test_without_a_baseline_it_stays_ADVISORY() -> None:
+    """MUST NOT FIRE -- the property the module docstring pins, preserved.
+
+    Eight candidates are reported on the default scope and the exit code is still 0. Baseline mode
+    is opt-in, so nothing that invoked this screen before behaves differently now."""
+    proc = _run()
+    assert proc.returncode == 0
+    assert "candidate(s) FOR JUDGEMENT" in proc.stdout
+
+
+def test_no_baseline_key_carries_a_line_number() -> None:
+    """The item states this as a hard rule: cite the greppable name, never a line.
+
+    The same site is ``create_search_preset`` or ``upsert_search_preset`` depending where you
+    stand, at different lines depending on your base. A baseline keyed on lines would go stale on
+    the next rebase and re-fire on sites already judged -- a false alarm that teaches the reader to
+    delete the step."""
+    for raw in BASELINE.read_text(encoding="utf-8").splitlines():
+        key = raw.split("#", 1)[0].strip()
+        if not key:
+            continue
+        assert not any(part.isdigit() for part in key.split("::")), f"line-keyed entry: {key}"
+
+
+def test_the_screen_is_wired_in_BOTH_places_like_its_sibling() -> None:
+    """THE ANTI-VACUITY ARM, and the one that decays. A screen wired in one place and not the other
+    is half a gate, and the half that is missing is invisible in a green run.
+
+    ``control_char_check`` is the POSITIVE CONTROL: it is wired in both, so if this test's own
+    reading were broken the control would fail first and say so."""
+    ci = CI.read_text(encoding="utf-8")
+    pre = PRECOMMIT.read_text(encoding="utf-8")
+
+    assert "control_char_check" in ci and "control_char_check" in pre, (
+        "the positive control is missing from one of the two files -- this test cannot be trusted"
+    )
+    assert "username_access_key_screen" in ci, "not wired into CI"
+    assert "username_access_key_screen" in pre, "not wired into pre-commit"
+    assert "--baseline" in ci and "--baseline" in pre, (
+        "wired WITHOUT --baseline, which installs a step that can never fail"
+    )