From 3a2501f2bbb96cd3de944820612345ac2dc603d7 Mon Sep 17 00:00:00 2001 From: "Jeremy [KK7GWY]" Date: Tue, 13 Oct 2026 12:06:52 -0700 Subject: [PATCH 1/3] feat(governance): route transmit-path changes by what the diff does. Principle VI. CODEOWNERS routes review by path, but the keying code is spread through Tier-3 files too large to gate that way (RadioModel.cpp, MainWindow*.cpp, every backend), so a keying change arrived looking like any other source change. .github/tx-surface.txt lists the transmit surface: files that are wholly TX machinery, and tokens (wire keying commands, keying entry points, TX permission guards) that are TX machinery wherever they appear. tools/check_tx_surface.py runs in Static checks and fails a PR whose non-comment src/ lines reach that surface until it adds .github/tx-acks/.md stating what changes on the TX path and how it was verified. Any code owner can approve the acknowledgement (.github/tx-acks/ is @aethersdr/reviewers); the surface list is Tier 1, since shrinking it is the bypass. Replayed over the 382 merges on main since 2026-09-13: 13 trigger, all keying, keying-control registration, TX-permission or TX-arbitration changes. Comment-only mentions (6 commits) and a UI string naming the env var do not. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/CODEOWNERS | 10 ++ .github/tx-surface.txt | 40 +++++++ .github/workflows/static-checks.yml | 16 +++ AGENTS.md | 4 + CONTRIBUTING.md | 7 ++ GOVERNANCE.md | 21 ++++ tools/check_tx_surface.py | 168 ++++++++++++++++++++++++++++ tools/test_check_tx_surface.py | 121 ++++++++++++++++++++ 8 files changed, 387 insertions(+) create mode 100644 .github/tx-surface.txt create mode 100644 tools/check_tx_surface.py create mode 100644 tools/test_check_tx_surface.py diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 847ae42eaa..f8ad106385 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -151,6 +151,12 @@ docs/ @aethersdr/reviewers resources/ @aethersdr/reviewers tests/ @aethersdr/reviewers +# Transmit-path acknowledgements (Principle VI, tools/check_tx_surface.py). A PR +# whose diff reaches .github/tx-surface.txt adds .github/tx-acks/.md; any +# code owner's approval covers it, so the directory is Tier 3. Its files are +# markdown, so like docs/ this line must sit below the *.md glob. +.github/tx-acks/ @aethersdr/reviewers + # AI-instruction files. AGENTS.md / CLAUDE.md / GEMINI.md / # .github/copilot-instructions.md all land here via the *.md glob above — # they are listed in the Tier 2 header comment rather than repeated as @@ -252,6 +258,10 @@ SECURITY* @aethersdr/maintainers # diff rather than one more "rides along" step in ci.yml (which is Tier 2). # See tools/check_ci_test_gate.py. .github/ci-test-gate.txt @aethersdr/maintainers +# The transmit surface that tools/check_tx_surface.py gates on. Tier 1 for +# the same reason: shrinking it is how a keying change would slip past the +# TX acknowledgement, so the diff to it is the review signal. +.github/tx-surface.txt @aethersdr/maintainers .github/codeql/ @aethersdr/maintainers docs/RELEASE-SIGNING-KEY.pub.asc @aethersdr/maintainers diff --git a/.github/tx-surface.txt b/.github/tx-surface.txt new file mode 100644 index 0000000000..ef811293d6 --- /dev/null +++ b/.github/tx-surface.txt @@ -0,0 +1,40 @@ +# The transmit surface. Principle VI: AetherSDR never transmits without +# operator intent. A PR whose diff reaches anything listed here must say so: +# tools/check_tx_surface.py fails Static checks until the PR adds +# .github/tx-acks/.md stating what changes on the TX path and how it +# was verified. That directory is owned by @aethersdr/reviewers, so any code +# owner's approval covers it. See GOVERNANCE.md §Transmit-path changes. +# +# This list is Tier 1: shrinking it is how the gate would be bypassed, so the +# diff to this file is the review signal, the same as .github/ci-test-gate.txt. +# +# Two kinds of entry, one per line, `kindregex`, trailing `# reason` +# not parsed: +# path — Python regex matched against the repo-relative path of any file +# the PR changes in a line that is not comment-only. The whole +# file is TX machinery. +# token — Python regex searched in every added or removed line under src/ +# that is not comment-only. The token is TX machinery wherever it +# appears, including inside RadioModel.cpp and MainWindow*.cpp, +# which are too large to gate by path. +# +# Comment-only lines never trigger either kind: a reworded comment is not a +# keying change. A user-visible string that merely names a token is not one +# either, which is why the env-var entry matches the read, not the name. + +path ^src/models/TxController\.(h|cpp)$ # the TX permission object +path ^src/core/TxCoordinator\.(h|cpp)$ # arbitration between TX producers +path ^src/models/RadioModel_TxCoordinator\.cpp$ # RadioModel's half of the coordinator +path ^src/core/backends/flex/FlexPttWireSession\.(h|cpp)$ # writes `xmit 1` / `xmit 0` +path ^src/core/TxKeyingMarker\.h$ # which controls key, and the bridge's refusal + +token "xmit\b # Flex MOX on the wire +token "transmit tune\b # Flex TUNE carrier +token "atu start\b # ATU tune keys a carrier +token \bcwx (macro )?send\b # CWX keys CW +token \bsetKeying\b # IRadioBackend keying entry point +token \b(setMox|setPtt|requestMox)\b # backend and model keying +token \b(markTxKeying|registerTxKeyingAction|registerReceiveControlAction)\b # keying-control registration +token \b(transmitControlMatch|requiresTxPermission)\b # keyboard and bridge TX guards +token \b(qEnvironmentVariable\w*|qgetenv|getenv)\(\s*"AETHER_AUTOMATION_ALLOW_TX" # reading the bridge's TX opt-in +token \bm_txAllowed\b # the bridge's cached TX opt-in diff --git a/.github/workflows/static-checks.yml b/.github/workflows/static-checks.yml index 3c497b7c75..e49af58368 100644 --- a/.github/workflows/static-checks.yml +++ b/.github/workflows/static-checks.yml @@ -343,6 +343,22 @@ jobs: if: ${{ !cancelled() }} run: python tools/check_ci_test_gate.py --strict + # ── Transmit-path acknowledgement (Principle VI). Keying code is spread + # across Tier-3 files too large to gate by path (RadioModel.cpp, + # MainWindow*.cpp, every backend), so this gates on what the diff does: + # a PR whose non-comment lines reach .github/tx-surface.txt must add + # .github/tx-acks/.md saying what changes on the TX path and how it + # was verified. The checkout is the merge ref, so HEAD^1 is the base. + - name: Transmit-path acknowledgement + if: ${{ !cancelled() }} + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + run: python tools/check_tx_surface.py --base HEAD^1 --pr "$PR_NUMBER" + + - name: Transmit-path checker self-test + if: ${{ !cancelled() }} + run: python tools/test_check_tx_surface.py + # ── Network transfer-timeout ratchet (#4688 §6). A QNetworkAccessManager # with no transfer timeout turns a half-open connection into a reply that # never finishes and never errors — the surface that issued it just stops, diff --git a/AGENTS.md b/AGENTS.md index fb595d90bf..e48764c139 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -319,6 +319,10 @@ runs in Docker image `ghcr.io/aethersdr/aethersdr-ci:latest`; a new `find_package(...)` needs its `-dev` package in `.github/docker/Dockerfile`. Test registration, the frozen per-PR test gate, and gate integrity: [`docs/agents/tests-ci.md`](docs/agents/tests-ci.md). +A PR whose code reaches `.github/tx-surface.txt` (keying commands, keying entry +points, TX permission guards) must add `.github/tx-acks/.md` stating +what changes on the TX path and how it was verified — Static checks fails +without it (GOVERNANCE.md §Transmit-path changes, Principle VI). ## Architecture Overview diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a9858f0371..f65e101bad 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -154,6 +154,13 @@ tier simply means changes to the repo's own scaffolding (dependency bumps, issue templates, CI config, build config) need an infrastructure owner rather than a maintainer. +One gate is not by path. A PR whose code reaches the transmit surface listed +in `.github/tx-surface.txt` (wire keying commands, keying entry points, TX +permission guards) must add `.github/tx-acks/.md` saying what +changes on the TX path and how it was verified; Static checks fails until it +does, and any code owner can approve it. See +[`GOVERNANCE.md`](GOVERNANCE.md) §Transmit-path changes. + For the mechanics around this gate — draft-PR conventions, the stale-branch policy, and how to recover a red `main` — see [`docs/PR-WORKFLOW.md`](docs/PR-WORKFLOW.md). diff --git a/GOVERNANCE.md b/GOVERNANCE.md index e075a53427..f11c017f47 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -202,6 +202,27 @@ review from someone else. The Tier-1 paths are hard gates: no merge without maintainer sign-off (an admin override is the only bypass). Team rosters live in the GitHub org; `.github/CODEOWNERS` carries the exact patterns. +### Transmit-path changes + +CODEOWNERS routes review by path, but the code that keys the transmitter is +spread through Tier-3 files too large to gate that way (`RadioModel.cpp`, +`MainWindow*.cpp`, every backend). A keying change there would otherwise +arrive looking like any other source change. Principle VI makes this the one +place where review is routed by **what a diff does**, not where it lands: + +- `.github/tx-surface.txt` lists the transmit surface: files that are wholly + TX machinery, and tokens (wire commands, keying entry points, TX permission + guards) that are TX machinery wherever they appear. +- When a PR's added or removed lines under `src/` reach that surface, Static + checks fails until the PR adds `.github/tx-acks/.md` stating what + changes on the transmit path and how it was verified. Comment-only lines + never trigger it. +- Any code owner (`@aethersdr/reviewers`) can approve the acknowledgement. Its + value is that the approval covers an explicit TX disclosure, and stale-review + dismissal makes it cover the head that merges. +- The surface list itself is Tier 1. Growing it is routine; shrinking it is + how a keying change would slip through, so that diff needs the maintainer. + --- ## AI Contributors diff --git a/tools/check_tx_surface.py b/tools/check_tx_surface.py new file mode 100644 index 0000000000..b58456756a --- /dev/null +++ b/tools/check_tx_surface.py @@ -0,0 +1,168 @@ +#!/usr/bin/env python3 +""" +AetherSDR transmit-surface gate (Principle VI). + +CODEOWNERS routes review by path, but the code that keys the transmitter is not +in one place: it runs through RadioModel.cpp, the MainWindow_*.cpp siblings, +TciServer.cpp and every backend, all Tier 3. A change to keying there reaches +the same reviewers as a tooltip fix. This checker routes it by what the diff +does instead: when a PR's diff reaches the transmit surface listed in +.github/tx-surface.txt, the PR must also add .github/tx-acks/.md +stating what changes on the transmit path and how it was verified. A code +owner's approval (@aethersdr/reviewers owns that directory) then covers an +explicit TX disclosure rather than a keying change buried in a large diff, and +the existing stale-approval dismissal makes it cover the head that merges. + +What it reads: every added or removed line under src/ that is not +comment-only. A `path` entry fires when such a line lands in a listed file; a +`token` entry fires when such a line anywhere under src/ matches it. A reworded +comment, even in TxController.cpp, does not trigger the gate. + +What it cannot see: keying reached through a name the list does not carry. The +list is Tier 1 for that reason; growing it is the remedy, and shrinking it is a +maintainer-reviewed diff. + +Usage: + python tools/check_tx_surface.py # report against origin/main + python tools/check_tx_surface.py --base HEAD^1 --pr 6500 # CI: enforce +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent +SURFACE = ".github/tx-surface.txt" +ACK_DIR = ".github/tx-acks" + +# `*` only when it reads as a block-comment continuation (`* text`, `*/`), so +# a statement such as `*m_keyed = true;` is still code. No `#`: in C++ that is +# the preprocessor, and `#define KEY "xmit 1"` keys as surely as a call. +COMMENT_START = re.compile(r"^\s*(//|/\*|\*(\s|/|$))") + + +def comment_only(body: str) -> bool: + if not COMMENT_START.match(body): + return False + # `/* note */ sendCmd("xmit 1");` carries code after the comment closes. + head, sep, tail = body.partition("*/") + return not (sep and tail.strip() and not tail.strip().startswith("//")) + + +@dataclass +class Surface: + paths: list[re.Pattern[str]] + tokens: list[re.Pattern[str]] + + +def load_surface(text: str) -> Surface: + paths: list[re.Pattern[str]] = [] + tokens: list[re.Pattern[str]] = [] + for lineno, raw in enumerate(text.splitlines(), 1): + line = raw.strip() + if not line or line.startswith("#"): + continue + kind, _, rest = line.partition(" ") + pattern = re.sub(r"\s+#.*$", "", rest).strip() + if kind not in ("path", "token") or not pattern: + raise ValueError(f"{SURFACE}:{lineno}: expected `path ` or `token `") + (paths if kind == "path" else tokens).append(re.compile(pattern)) + if not paths or not tokens: + raise ValueError(f"{SURFACE}: needs at least one path and one token entry") + return Surface(paths, tokens) + + +def git(repo: Path, *args: str) -> str: + return subprocess.run(["git", "-C", str(repo), *args], check=True, + capture_output=True, text=True, errors="replace").stdout + + +def default_base(repo: Path) -> str: + return git(repo, "merge-base", "origin/main", "HEAD").strip() + + +def triggers(diff: str, surface: Surface) -> list[str]: + """Return a human-readable reason for every hit in a `git diff -U0` text.""" + found: list[str] = [] + current: str | None = None + for line in diff.splitlines(): + if line.startswith("diff --git "): + current = None + continue + if line.startswith(("--- ", "+++ ")): + name = line[4:] + if name != "/dev/null": + current = name[2:] + continue + if current is None or not current.startswith("src/"): + continue + if line[:1] not in "+-": + continue + body = line[1:] + if comment_only(body): + continue + for p in surface.paths: + if p.search(current): + reason = f"{current}: file is on the TX surface ({p.pattern})" + if reason not in found: + found.append(reason) + for t in surface.tokens: + m = t.search(body) + if m: + reason = f"{current}: {line[0]} line uses `{m.group(0)}`" + if reason not in found: + found.append(reason) + return found + + +def added_files(repo: Path, base: str) -> set[str]: + out = git(repo, "diff", "--name-only", "--diff-filter=A", f"{base}", "HEAD") + return set(out.split()) + + +def main(argv: list[str] | None = None) -> int: + ap = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) + ap.add_argument("--base", help="diff base (default: merge-base with origin/main)") + ap.add_argument("--pr", type=int, help="PR number; when given, a hit without its ack fails") + ap.add_argument("--repo", type=Path, default=REPO, help=argparse.SUPPRESS) + args = ap.parse_args(argv) + + repo: Path = args.repo + surface = load_surface((repo / SURFACE).read_text()) + base = args.base or default_base(repo) + diff = git(repo, "diff", "-U0", "--no-color", "--no-ext-diff", base, "HEAD") + hits = triggers(diff, surface) + + if not hits: + print("TX surface: not touched.") + return 0 + + print("TX surface touched (Principle VI):") + for h in hits: + print(f" {h}") + + if args.pr is None: + print(f"\nA PR with this diff must add {ACK_DIR}/.md.") + return 0 + + ack = f"{ACK_DIR}/{args.pr}.md" + if ack not in added_files(repo, base): + print(f"::error::This PR changes the transmit path. Add {ack} with one or two " + f"lines saying what changes on the TX path and how it was verified. " + f"Any code owner's approval covers it (GOVERNANCE.md " + f"§Transmit-path changes).") + return 1 + if not (repo / ack).read_text().strip(): + print(f"::error::{ack} is empty; say what changes on the TX path.") + return 1 + print(f"\nAcknowledged in {ack}.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/test_check_tx_surface.py b/tools/test_check_tx_surface.py new file mode 100644 index 0000000000..2e4ba4db1a --- /dev/null +++ b/tools/test_check_tx_surface.py @@ -0,0 +1,121 @@ +#!/usr/bin/env python3 +""" +Self-test for tools/check_tx_surface.py. + +Each case builds a throwaway git repo carrying the real .github/tx-surface.txt, +commits a base, applies one change on top, and runs the checker against it. +The gate is only worth having if it fails on a keying change without an ack, +passes with one, and stays quiet on a comment-only or unrelated change. + +Usage: + python tools/test_check_tx_surface.py +""" + +from __future__ import annotations + +import contextlib +import io +import subprocess +import sys +import tempfile +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import check_tx_surface # noqa: E402 + +SURFACE_TEXT = (check_tx_surface.REPO / check_tx_surface.SURFACE).read_text() +PR = 4242 +ACK = f"{check_tx_surface.ACK_DIR}/{PR}.md" + +BASE_FILES = { + "src/models/RadioModel.cpp": "void f() {\n int x = 0;\n}\n", + "src/models/TxController.cpp": "// controller\n", + "src/gui/Label.cpp": "void g() {}\n", +} + +# (name, files to write after the base commit, expected exit code, substring +# the output must contain) +CASES = [ + ("unrelated change passes", + {"src/gui/Label.cpp": "void g() { int y = 1; }\n"}, 0, "not touched"), + ("keying token without ack fails", + {"src/models/RadioModel.cpp": "void f() {\n sendCmd(\"xmit 1\");\n}\n"}, 1, ACK), + ("removed keying token without ack fails", + None, 1, "- line uses"), + ("keying token with ack passes", + {"src/models/RadioModel.cpp": "void f() {\n m_backend->setKeying(true);\n}\n", + ACK: "Keys MOX through setKeying; verified on the demo radio.\n"}, 0, "Acknowledged"), + ("empty ack fails", + {"src/models/RadioModel.cpp": "void f() {\n m_backend->setKeying(true);\n}\n", + ACK: "\n"}, 1, "is empty"), + ("comment-only mention passes", + {"src/models/RadioModel.cpp": "void f() {\n // was sendCmd(\"xmit 1\")\n int x = 0;\n}\n"}, + 0, "not touched"), + ("path entry without ack fails", + {"src/models/TxController.cpp": "// controller\nint grants = 0;\n"}, 1, "file is on the TX surface"), + ("comment-only edit in a path-listed file passes", + {"src/models/TxController.cpp": "// controller, reworded\n"}, 0, "not touched"), + ("preprocessor line is code, not a comment", + {"src/models/RadioModel.cpp": "#define KEY \"xmit 1\"\nvoid f() {\n int x = 0;\n}\n"}, + 1, ACK), + ("code after a closed block comment is code", + {"src/models/RadioModel.cpp": "void f() {\n /* key */ sendCmd(\"xmit 1\");\n}\n"}, 1, ACK), + ("UI string naming the env var passes", + {"src/gui/Label.cpp": "void g() { tip(\"set AETHER_AUTOMATION_ALLOW_TX=1\"); }\n"}, + 0, "not touched"), + ("token outside src/ passes", + {"docs/notes.md": "setKeying is the entry point\n"}, 0, "not touched"), +] + + +def git(repo: Path, *args: str) -> None: + subprocess.run(["git", "-C", str(repo), *args], check=True, capture_output=True) + + +def write(repo: Path, files: dict[str, str]) -> None: + for rel, text in files.items(): + p = repo / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text) + + +def run_case(name: str, change: dict[str, str] | None, want: int, needle: str) -> bool: + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) + git(repo, "init", "-q", "-b", "main") + git(repo, "config", "user.email", "t@example.invalid") + git(repo, "config", "user.name", "t") + git(repo, "config", "commit.gpgsign", "false") + base_files = dict(BASE_FILES) + if change is None: + # A removal: the base already keys, the change takes it out. + base_files["src/models/RadioModel.cpp"] = "void f() {\n sendCmd(\"xmit 0\");\n}\n" + change = {"src/models/RadioModel.cpp": BASE_FILES["src/models/RadioModel.cpp"]} + write(repo, {check_tx_surface.SURFACE: SURFACE_TEXT, **base_files}) + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "base") + write(repo, change) + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "change") + + out = io.StringIO() + with contextlib.redirect_stdout(out): + got = check_tx_surface.main(["--repo", str(repo), "--base", "HEAD^1", "--pr", str(PR)]) + text = out.getvalue() + ok = got == want and needle in text + print(f"{'PASS' if ok else 'FAIL'}: {name}") + if not ok: + print(f" exit {got}, wanted {want}; output must contain {needle!r}:") + print(" " + text.replace("\n", "\n ")) + return ok + + +def main() -> int: + check_tx_surface.load_surface(SURFACE_TEXT) # the real list must parse + results = [run_case(*c) for c in CASES] + print(f"\n{sum(results)}/{len(results)} cases passed") + return 0 if all(results) else 1 + + +if __name__ == "__main__": + sys.exit(main()) From 7905f4f6520e5aa608e11f8d2591e7847175f54b Mon Sep 17 00:00:00 2001 From: "Jeremy [KK7GWY]" Date: Tue, 13 Oct 2026 12:07:54 -0700 Subject: [PATCH 2/3] ci(full-suite): attribute a red main to its source PR and its parent's verdict. Principle VIII. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tracking issue named the commit and the failing tests, but deciding fix-forward vs revert still meant looking up which PR the commit came from and whether main was already red before it. The issue now carries both: the commit's source PR and author, and the parent commit's own full-suite verdict ("first red" vs "already red at parent — look earlier"). It also points at the #4703 flake umbrella and says the revert stays a human call, so a flake does not revert good work. Each lookup degrades to "unknown" rather than blocking the issue. Exercised against real main commits through gh api: d3ab349 (#6199) reports main already red at its parent; 37fbdbc (#6413) reports itself as the first red commit. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/full-suite.yml | 44 ++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/.github/workflows/full-suite.yml b/.github/workflows/full-suite.yml index 7ce9f7bf08..0a608a39d3 100644 --- a/.github/workflows/full-suite.yml +++ b/.github/workflows/full-suite.yml @@ -30,9 +30,13 @@ on: # `github.run_id` in the group, which gave every run a group of its own and # made the whole thing inert — the same behaviour, reached confusingly. +# actions: read and pull-requests: read are for the tracking issue's +# attribution lines (the parent commit's verdict, the commit's source PR). permissions: contents: read issues: write + actions: read + pull-requests: read jobs: full-suite: @@ -318,13 +322,53 @@ jobs: const createLabels = [...labels, 'bug', 'priority: high']; const sha = context.sha.substring(0, 7); const date = new Date().toISOString().split('T')[0]; + const repo = { owner: context.repo.owner, repo: context.repo.repo }; + + // Attribution, so a human can decide fix-forward vs revert from the + // issue alone (docs/PR-WORKFLOW.md §Recovering from a red main). + // Squash merges make the pushed commit the suspect, unless main was + // already red one commit earlier. Neither lookup may stop the issue + // from being filed, so each degrades to "unknown". + let sourcePr = 'unknown'; + try { + const prs = await github.rest.repos.listPullRequestsAssociatedWithCommit({ + ...repo, commit_sha: context.sha, + }); + const pr = prs.data.find(p => p.merged_at) || prs.data[0]; + sourcePr = pr ? `#${pr.number} by @${pr.user.login}` : 'none (direct push)'; + } catch (e) { + core.warning(`source PR lookup failed: ${e.message}`); + } + let parentVerdict = 'unknown'; + try { + const commit = await github.rest.repos.getCommit({ ...repo, ref: context.sha }); + const parent = commit.data.parents[0]?.sha; + if (parent) { + const runs = await github.rest.actions.listWorkflowRuns({ + ...repo, workflow_id: 'full-suite.yml', head_sha: parent, per_page: 10, + }); + const done = runs.data.workflow_runs.find(r => r.status === 'completed'); + const p7 = parent.substring(0, 7); + parentVerdict = + !done ? `no completed run for parent \`${p7}\`` + : done.conclusion === 'success' ? `green at parent \`${p7}\` — this commit is the first red one` + : `already ${done.conclusion} at parent \`${p7}\` (${done.html_url}) — look earlier`; + } + } catch (e) { + core.warning(`parent verdict lookup failed: ${e.message}`); + } const body = [ `**Run:** ${process.env.RUN_URL}`, `**Commit:** \`${sha}\``, + `**Source PR:** ${sourcePr}`, + `**Main before this commit:** ${parentVerdict}`, `**Date:** ${date}`, `**Failed:** ${process.env.FAILED}`, '', + 'Before reverting, check the failing tests against the known-flake umbrella #4703 ' + + 'and re-run once; a revert is a human call (docs/PR-WORKFLOW.md §Recovering from a red main).', + '', '
ctest report', '', '```', From bd540594261e8c12926fb953b2e3043136984b74 Mon Sep 17 00:00:00 2001 From: "Jeremy [KK7GWY]" Date: Tue, 13 Oct 2026 12:08:27 -0700 Subject: [PATCH 3/3] docs(review): judge a PR against its stated intent first, and stop asking for interior defense. Principle VII. The PR template gains an Intent section: what will be true after the merge and what is out of scope. /pr-review starts step 2 from it and checks it against the linked issue before reading hunks, so a correct diff for the wrong goal is caught first; the report carries an Intent line. The adversarial stance gains its second limit: requests for validation inside code that Principle VII says may "treat parsed values as sound", or for abstraction the stated intent does not need, are nits at most unless the reviewer can name the input that reaches the interior unvalidated. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/pr-review/SKILL.md | 17 +++++++++++++++++ .github/PULL_REQUEST_TEMPLATE.md | 7 +++++++ 2 files changed, 24 insertions(+) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 0448a76aa6..387755248d 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -85,6 +85,15 @@ clean report stated plainly and without hedging. Severity stays calibrated what you actually observed — file:line, command output, failing scenario. Break the code, not the author. +Nor does it mean **asking for defense the code does not need.** Principle VII +puts validation at the boundary "so the interior can treat parsed values as +sound": a request to re-check, inside our own code, a value that already +crossed a boundary or that our own code produced is over-engineering, not +rigor. So is an abstraction, option or fallback the PR's stated intent does +not call for. Raise either as a nit at most, never a blocker, unless you can +name the concrete input that reaches the interior unvalidated. That input is +the finding; the missing check is not. + ## 0. Test-boundary preflight — before parallel work Complete this preflight before building or running any test. It overrides the @@ -178,6 +187,13 @@ the shape the bullet above routes a simulator closed loop to — see ## 2. Linked issue → does the PR actually solve it? +- **Start with intent.** Read the PR body's `## Intent` section (the PR + template asks for it) and state in one line what the PR is for. Check that + against the linked issue before reading any hunk: a correct diff for the + wrong goal is the most expensive finding a review can miss, and the one + line-by-line reading misses most. If the section is absent, reconstruct the + intent from the body and issue, say you did, and note the missing section + as a nit. - Linked issues = `closingIssuesReferences` plus any `#NNNN` referenced in the PR title/body as "fixes/closes/resolves". Read each with `gh issue view --json title,body,comments` (skim comments for accepted @@ -711,6 +727,7 @@ round trip even for a nit. One body is read once and answered once. ## PR #NNNN — (@author) **Issue:** #MMMM — one-paragraph summary of the problem as reported. +**Intent:** the PR's stated goal in one line, and whether it matches the issue. **Proposed fix:** one-paragraph summary of the approach the diff takes. **Does it solve the issue?** Yes / Partially / No — with the requirement→hunk mapping and anything unaddressed. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 04a8d2b7a4..7b22d30a9e 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -18,6 +18,13 @@ Before opening the PR, please: <!-- One-paragraph description of what changes and why. If this fixes an issue, lead with "Fixes #N" or "Closes #N". --> +## Intent + +<!-- What will be true after this merges that is not true now, in one or two +sentences, and anything deliberately left out of scope. Review starts here: +the diff is judged against this before it is read line by line, so a correct +diff for the wrong goal is caught first. --> + ## Constitution principle honored <!-- Cite the principle by Roman numeral when relevant. E.g.