From 6a1895562e0b49b2559cd8a6074e97e77a18cb2f Mon Sep 17 00:00:00 2001 From: David Date: Mon, 24 Aug 2026 02:03:59 -0700 Subject: [PATCH] fix(drift-audit): two silent failures that hid real drift for two months The audit has run green every Monday since 2026-07-06 while filing exactly nothing and while one of its checks could never have passed. Both are fixed here; the drift it was hiding is fixed in the four companion PRs. 1. Issue emission failed silently. emit_issue() called _gh(), which discards stderr and returns None on failure, then printed "opened new issue on " WITHOUT checking. main() returned 0 unconditionally in emit mode ("the issues ARE the deliverable"). DRIFT_AUDIT_TOKEN has never been set, so the job ran on GITHUB_TOKEN, which cannot open an issue on a sibling repo -- and the displayxr-publish-bot App has no `issues` permission at all, so wiring it would not have helped either. Seven weekly runs, five claimed issues each, zero that exist: no drift issue has EVER been filed on any repo, open or closed. Now: writes go through _gh_full() and are checked; failure prints the real gh error; the run raises a ::warning::; and the findings still land as ONE aggregate issue on this repo, which a workflow token can always write to. The exit code now tracks whether the findings reached a human rather than whether drift exists. 2. The vendored-spec comparison could never clear. strip_license_header() treated lines starting with '#', '*' and '//' as comments. In a markdown doc those are content: it ate the canonical file's '# Title' and '**Status:**' paragraph while stopping dead at the vendored copy's '> Vendored copy' provenance banner. So the two hashed unequal no matter how faithfully the copy was synced -- confirmed by hashing a byte-perfect re-sync, which still compared unequal. A permanently un-actionable finding. Replaced with strip_doc_banner(), which drops leading blockquote/HTML- comment provenance and leaves markdown content alone. Verified: the current copy still differs (real drift), a faithful re-sync now matches. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Ua8CevgSHNqoyiy7ziZjHk --- .github/workflows/drift-audit.yml | 20 +++- scripts/drift_audit.py | 164 +++++++++++++++++++++++++----- 2 files changed, 153 insertions(+), 31 deletions(-) diff --git a/.github/workflows/drift-audit.yml b/.github/workflows/drift-audit.yml index e71b001c52..2601a14bc6 100644 --- a/.github/workflows/drift-audit.yml +++ b/.github/workflows/drift-audit.yml @@ -10,9 +10,23 @@ name: drift-audit # # TOKEN: reading private org repos (displayxr-shell-pvt) and opening issues on # *other* repos both need more than the default GITHUB_TOKEN. Set an org-scoped -# PAT as the `DRIFT_AUDIT_TOKEN` secret. Without it the run still works but -# silently skips repos the default token can't reach (the script degrades to -# "could not fetch … skipped" notes rather than failing). +# PAT as the `DRIFT_AUDIT_TOKEN` secret. Without it READS degrade to +# "could not fetch … skipped" notes rather than failing. +# +# WRITES used to degrade far worse than that. `DRIFT_AUDIT_TOKEN` has never +# been set, so this ran on GITHUB_TOKEN, which cannot open an issue on a +# sibling repo -- and emit_issue printed "opened new issue on …" without +# checking, while main() returned 0 unconditionally in emit mode. Result: +# every weekly run from 2026-07-06 to 2026-08-24 reported success having filed +# nothing, and real drift (a displayxr-common pin spread, a stale vendored +# spec) accumulated unseen behind a green check. +# +# Now: emission failure is printed with the real gh error, the run annotates a +# ::warning::, and the findings still land as ONE aggregate issue on this repo +# (a workflow token can always write to its own repo). The job fails only if +# the findings reached nobody at all. Setting DRIFT_AUDIT_TOKEN -- or granting +# the displayxr-publish-bot App the `issues` permission it currently lacks +# entirely -- restores per-repo issues. on: schedule: diff --git a/scripts/drift_audit.py b/scripts/drift_audit.py index 49f9e1d602..3398f0da36 100755 --- a/scripts/drift_audit.py +++ b/scripts/drift_audit.py @@ -177,7 +177,14 @@ _tag_sha_cache: dict[str, dict[str, str]] = {} # repo -> {commit sha: semver tag} -def _gh(args: list[str]) -> str | None: +def _gh_full(args: list[str]) -> tuple[int, str, str]: + """Run gh and return (returncode, stdout, stderr). + + Read paths deliberately swallow failure (a repo the token can't see is a + "skipped", not an error). WRITE paths must not: discarding stderr here is + what let issue emission fail silently every week for two months. Anything + that mutates state should use this and check. + """ try: out = subprocess.run( ["gh", *args], @@ -187,9 +194,12 @@ def _gh(args: list[str]) -> str | None: ) except FileNotFoundError: sys.exit("error: the 'gh' CLI is required and was not found on PATH") - if out.returncode != 0: - return None - return out.stdout + return out.returncode, out.stdout, out.stderr + + +def _gh(args: list[str]) -> str | None: + rc, out, _ = _gh_full(args) + return None if rc != 0 else out def gh_raw(repo: str, path: str, ref: str = "HEAD") -> str | None: @@ -317,20 +327,34 @@ def extract_fetchcontent_pins(cmake_text: str) -> dict[str, str]: return pins -def strip_license_header(text: str) -> str: - """Drop a leading comment/license block so bodies compare fairly.""" +def strip_doc_banner(text: str) -> str: + """Drop a leading provenance banner so two copies of a doc compare fairly. + + A vendored copy legitimately carries a banner the canonical file must not + have -- "> **Vendored copy.** … Last synced: " -- and the canonical + one does not. Everything above the first real paragraph is provenance. + + This replaces an earlier version that also stripped lines beginning `#`, + `*` and `//`. In a MARKDOWN doc those are content, not comments: it ate the + canonical file's `# Title` and its `**Status:**` paragraph while stopping + dead at the vendored copy's `>` banner. The two could therefore never hash + equal no matter how faithfully the copy was synced, so the finding it + raised was permanently un-actionable -- verified by re-hashing a + byte-perfect re-sync, which still compared unequal. + """ lines = text.splitlines() i = 0 - while i < len(lines) and ( - not lines[i].strip() - or lines[i].lstrip().startswith(("