Repository navigation
Conversation
…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/<PR>.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) <noreply@anthropic.com>
…s verdict. Principle VIII.
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) <noreply@anthropic.com>
…king 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) <noreply@anthropic.com>
ten9876
added
the
governance
K5PTB
left a comment
K5PTB
left a comment
There was a problem hiding this comment.
Review of #6436 at bd540594, against RFC #6435
Issue fit. The PR's Intent section matches the RFC. Each of the RFC's three items (A: transmit-path acknowledgement, B: red-main attribution, C: intent-first review) has its own commit, and the file lists match the RFC's "Implementation scope". C is complete and correct. A and B both have a defect that undercuts the outcome their Intent promises:
- A: a keying change can still merge without an ack. Several real keying entry points aren't on the surface, including the one the RFC triage asked to have added, and renaming a listed file escapes the path entries.
- B: on the PR's own example commit, the red-main attribution points away from the commit that caused the failures.
None of this needs a redesign. A tested fix is below.
Scope
| File / group | Claimed in the RFC/body? | Verdict |
|---|---|---|
.github/tx-surface.txt, tools/check_tx_surface.py, tools/test_check_tx_surface.py |
A | In scope |
.github/workflows/static-checks.yml (2 steps) |
A | In scope |
.github/CODEOWNERS (2 entries) |
A | In scope. The tx-acks/ line is needed: without it *.md (line 118) would route acks to infrastructure |
GOVERNANCE.md, CONTRIBUTING.md, AGENTS.md |
A | In scope |
.github/workflows/full-suite.yml |
B | In scope, including the two new read permissions, which the body discloses |
.github/PULL_REQUEST_TEMPLATE.md, .claude/skills/pr-review/SKILL.md |
C | In scope |
Everything in the diff is explained by the RFC. The PR has 455 additions and no deleted lines, so no guard is removed. There's no CHANGELOG.md entry, which is correct. The checklist's claims hold.
Blockers
1. The transmit surface misses keying entry points that Principle VI names, including the gap the RFC triage flagged. .github/tx-surface.txt:31-40
The triage comment on #6435 asked for setTune/setAtu before merge. The list doesn't have them yet, and they're not the only names missing. Each of these was run through the PR's own check_tx_surface.main() as a one-line added change under src/, and each one exits 0 with "TX surface: not touched":
| Keying path | Where | Gated? |
|---|---|---|
setTune (TUNE carrier; Principle VI names "a tune request") |
IRadioBackend.h:605, flex/hl2/icom implementations |
no |
setAtu |
IRadioBackend.h:683 |
no |
setCwKeying (\bsetKeying\b doesn't match it) |
IRadioBackend.h:575, Hl2Backend.cpp:4607 |
no |
Flex "cw key %1" / "cw ptt 1" wire commands |
RadioModel.cpp:5108-5109 |
no |
HL2 MOX bit and CW MOX: withMox, setCwMox, applyKeying |
MetisProtocol.h:382, MetisClient.cpp:1216, Hl2Backend.cpp:4327 |
no |
Icom CI-V PTT frame: cmdSetPtt |
CivCodec.cpp:1066, IcomCivBackend.cpp:942,5197 |
no |
The Flex wire writer is path-listed (FlexPttWireSession, line 28), but the HL2 and Icom equivalents aren't covered by either kind of entry. As written, the gate is mostly a Flex gate.
Adding these costs nothing in trigger rate. I replayed main's 382 first-parent commits since 2026-09-13, up to this PR's merge base 47aa4098. I got the PR's 13 with the list as it stands, and still 13 with every name above added. Suggested lines:
token "cw (key|ptt)\b # Flex CW key and CW PTT on the wire
token \b(setKeying|setCwKeying|setTune|setAtu)\b # IRadioBackend keying, CW, TUNE carrier and ATU entry points
token \b(applyKeying|setCwMox|withMox|cmdSetPtt)\b # HL2 and Icom keying down to the wire
The first goes after "atu start. The second replaces the \bsetKeying\b line.
2. Renaming a path-listed file escapes the path entries. tools/check_tx_surface.py:138
git diff detects renames by default, and triggers() files each hunk under the +++ name. If a PR moves src/models/TxController.cpp to src/models/TxPermission.cpp and changes a line of code in the same commit, it passes with no ack. The same edit without the rename is gated, as the control case showed. After the rename the path entry also silently matches nothing. GOVERNANCE.md:223 says shrinking the list "is how a keying change would slip through", and a rename shrinks it without touching the Tier-1 file. The fix is --no-renames, which makes the old path's removed lines count. Pinning the prefixes also fixes the second nit below:
# No rename detection: a renamed TX file must still be judged under its old
# path. Fixed prefixes: diff.noprefix / diff.mnemonicPrefix would otherwise
# change what name[2:] strips.
diff = git(repo, "diff", "-U0", "--no-color", "--no-ext-diff", "--no-renames",
"--src-prefix=a/", "--dst-prefix=b/", base, "HEAD")Do the same at line 124 ("--name-only", "--no-renames", "--diff-filter=A"). Otherwise an ack created from a file deleted in the same PR shows up as R, not A. Please add a rename case to the self-test as well. None of the 12 current cases covers it.
3. "Already red at parent — look earlier" is wrong when a commit adds a new failure to an already-red main. .github/workflows/full-suite.yml:352-355
The parent verdict compares workflow conclusions, not failure signatures. The PR's own example shows the problem. d3ab349 (#6199) is cited in the body as reporting "already red at its parent", and it does. Its parent 0cce730 was red only on radio_setup_headphone_output_gate_test, which went to #6418. d3ab349 itself broke fm_tone_presentation_contract, icom_family_test and hl2_tci_signaling_test, and the fingerprint rule opened a new issue for them, #6420. With this PR, #6420 would have said "look earlier", pointing away from #6199, the commit that introduced those three failures. That is the fix-forward-or-revert decision the Intent says this change supports, and it gets the wrong pointer. Main has been red in exactly this way before: five consecutive red runs on 2026-10-12, with two signatures.
Drop-in wording that stays true in both cases, at line 355:
: `${done.conclusion} at parent \`${p7}\` (${done.html_url}). If that run failed the same tests, look earlier; a test failing here but not there starts at this commit`;A sharper version would compute the verdict after the tagged lookup: a new fingerprint on a red parent means this commit is the first red for that signature. The wording above is enough to stop the misdirection.
Needs maintainer decision
- The RFC hasn't been approved on #6435. GOVERNANCE.md:130-131: "The maintainer will comment with approval, rejection, or requested changes. Do not open a PR until the RFC issue is approved." This PR opened at 19:16Z, four minutes after the RFC was labelled and before triage. #6435 has no approval comment, and it still carries
maintainer-review. You're the maintainer, so clearing this takes one comment. That comment should also give the ruling triage asked for: is the transmit surface keying-only (Principle VI), or thesafetylabel's wider "TX power, ATU, antenna switching"?transmit set vox_enable(FlexModelControls.cpp:154) is one example the ruling decides. It isn't gated, and it lets audio key the transmitter. - The RFC promised items you could approve or reject separately ("Each item can be approved or rejected on its own"). Splitting them into three commits keeps them separate only until a squash merge collapses them into one. If you want B or C revertable on their own, use three PRs or a rebase merge. If not, it's fine as it is.
Nits (non-blocking)
tools/check_tx_surface.py:97: any removed line whose content starts at column 0 with--, or added line starting++(for example++ n;), reads as a file header.currentthen changes, and the rest of that file's hunks are skipped. I reproduced it: a-- xremoval followed by anxmitaddition passes. It's rare in C++, but the fix is cheap: treat---/+++as headers only before a file's first@@. The patch below does that.tools/check_tx_surface.py:46:COMMENT_STARTtreats an indented* p = setMox(true);as a block-comment continuation, so the token is never checked. It's an unusual style, and making the check stateful probably isn't worth it. Noted so the limit is known.- In local mode with
diff.noprefix=true, a realxmitchange prints "not touched" (reproduced). CI is unaffected. The prefixes pinned in blocker 2 fix it. - The body says "Seven that matched only in comments or a help string". The replay with
comment_only()disabled finds eight comment-only matches: d3ab349, f9f8495, ca88369, 3b5d693, 04d269b, 8f4b4dc, 5607b56, 0a3494e. Triage reported the same count. I checked all eight and the filter is right on each. Only the prose is off. - Rollout, also unaddressed from triage: when this merges, open PRs #4921 (FT-991 backend), #4920 (ColibriNANO backend) and draft #4770 (TX Linearity Analyzer) all reach the surface and go red. #6002's diff is too large for
gh pr diff, so I couldn't scan it. Worth a note on those PRs when this lands. full-suite.yml:337: when no associated PR is merged (a direct push),|| prs.data[0]names an open PR as the source and @-mentions its author. Useprs.data.find(p => p.merged_at)alone, so a direct push falls through to "none (direct push)".- The self-test doesn't pin
--diff-filter=A. Removing it still gives 12/12, so an ack that was only modified would pass. It's harmless today because PR numbers don't repeat.
Suggested patch (blockers 1–2 and the header nit)
Applied to bd540594 in a scratch tree: the self-test gives 12/12, and all 11 probe cases above are gated. vox_enable stays ungated pending your ruling.
--- a/tools/check_tx_surface.py
+++ b/tools/check_tx_surface.py
@@ def triggers(diff: str, surface: Surface) -> list[str]:
found: list[str] = []
current: str | None = None
+ in_hunk = False
for line in diff.splitlines():
if line.startswith("diff --git "):
current = None
+ in_hunk = False
continue
- if line.startswith(("--- ", "+++ ")):
+ if line.startswith("@@"):
+ in_hunk = True
+ continue
+ # A removed `-- x` or added `++ n;` reads as `--- `/`+++ `; only the
+ # lines before a file's first hunk are headers.
+ if not in_hunk and line.startswith(("--- ", "+++ ")):
name = line[4:]
@@ def added_files(repo: Path, base: str) -> set[str]:
- out = git(repo, "diff", "--name-only", "--diff-filter=A", f"{base}", "HEAD")
+ out = git(repo, "diff", "--name-only", "--no-renames", "--diff-filter=A", f"{base}", "HEAD")
@@ def main(argv: list[str] | None = None) -> int:
- diff = git(repo, "diff", "-U0", "--no-color", "--no-ext-diff", base, "HEAD")
+ # No rename detection: a renamed TX file must still be judged under its old
+ # path. Fixed prefixes: diff.noprefix / diff.mnemonicPrefix would otherwise
+ # change what name[2:] strips.
+ diff = git(repo, "diff", "-U0", "--no-color", "--no-ext-diff", "--no-renames",
+ "--src-prefix=a/", "--dst-prefix=b/", base, "HEAD")What I verified, and what held up
- Ran:
tools/test_check_tx_surface.pypasses 12/12 at the head. I made seven deliberate breaks to the checker and the self-test caught six: comment filter off, ignoring code after*/, skipping removed lines, the empty-ack check, thesrc/restriction, and the path entries. It missed only--diff-filter=A(nit above). The checker reports "not touched" on this PR itself. Both workflows parse as YAML. - Reproduced: the 382 → 13 replay matches exactly. CI ran on the current main tip (
47aa4098, earliest run 19:16Z), so the green is not stale. - Held up:
- Static checks is
pull_request-only, so--pr "$PR_NUMBER"is never empty. fetch-depth: 0makesHEAD^1on the merge ref the base tip.- Add and delete both attribute the right path.
- The env-var token matches the read, not a UI string.
- B's lookups are each try-wrapped, so neither can stop the issue being filed, and the new lines reach both the sticky-comment and new-issue paths.
full-suite.ymlhas noconcurrency:block, so a parent run is never auto-cancelled.docs/PR-WORKFLOW.md §Recovering from a red mainresolves (line 47).- CODEOWNERS ordering is right: no later pattern overrides
.github/tx-acks/.
- Static checks is
- Not exercised: the full-suite issue step only runs on a red main, so blocker 3 is reasoned from the code and from the #6418/#6420 history, not from a live run. I didn't drive the app because there are no
src/changes. No socket tests: the new Python test uses only temporary git repos. - The automated
code-reviewpass at medium effort found the rename, header,*and noprefix bypasses, thesetCwKeying/applyKeyinggap, the fingerprint issue and theprs.data[0]fallback. I verified each one before including it. I dropped two of its claims:- That the
tx-acks/CODEOWNERS line is a no-op. Without it,*.mdroutes acks to infrastructure, and any-code-owner approval is the RFC's stated choice. - That the self-test should also be registered in
tests.cmake. Sibling checkers are split on that, so there's no rule to cite.
- That the
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #6436 — route review by what a diff does (@ten9876)
Issue: RFC #6435 — CODEOWNERS routes review by path, but keying code is spread through Tier-3 files too large to gate that way, so a keying change reaches the same reviewers as a tooltip fix. The RFC also wants a red main to name its source PR, and reviews to start from stated intent.
Intent: stated, and the PR eats its own dog food — a ## Intent section is present. The three bullets ("a change to TX keying can't merge without the author saying so"; "a red main names its likely PR"; "reviews start from what a PR is for") match the RFC's A/B/C. Finding 2 below falsifies the first bullet empirically.
Proposed fix: three independent commits — a token/path surface list plus a Static-checks gate requiring .github/tx-acks/<PR>.md; two attribution lookups in full-suite.yml; an Intent section in the PR template and two paragraphs in /pr-review.
Does it solve the issue? Partially. B and C I found nothing to fix in. A is wired correctly — but its recall is low enough that the largest open TX-feature PR in the repo passes it silently, and one diff-shape accident disables it for a whole file.
Scope
| File(s) | What it changes | Claimed in body? | Verdict |
|---|---|---|---|
.github/tx-surface.txt, tools/check_tx_surface.py, tools/test_check_tx_surface.py |
The gate (A) | Yes | In scope |
.github/workflows/static-checks.yml |
Two steps for A | Yes | In scope |
.github/CODEOWNERS |
tx-acks/ Tier 3, tx-surface.txt Tier 1 |
Yes | In scope. Ordering verified: tx-acks/ (158) sits after *.md (118) and nothing later re-matches it, so it resolves to @aethersdr/reviewers as claimed; tx-surface.txt (264) resolves to @aethersdr/maintainers. |
GOVERNANCE.md, CONTRIBUTING.md, AGENTS.md |
Document A | Yes | In scope (Tier 1 / Tier 1 / Tier 2) |
.github/workflows/full-suite.yml |
Red-main attribution (B) | Yes | In scope |
.github/PULL_REQUEST_TEMPLATE.md, .claude/skills/pr-review/SKILL.md |
Intent-first review (C) | Yes | In scope |
Everything in the diff is explained by the RFC. No src/ changes, no CHANGELOG entry, no new public surface. The one scope issue is not a stray file — it is the bundling, below.
Blockers
1. tools/check_tx_surface.py:97-101 — a content line beginning ++ / -- is parsed as a file header, and silently drops the rest of that file's diff
if line.startswith(("--- ", "+++ ")):
name = line[4:]
if name != "/dev/null":
current = name[2:]
continueHeader detection is not hunk-aware. In git diff -U0, an added line whose content is ++ x; (legal C++ — ++ counter;, or a continuation line) renders as +++ x;, which this matches. current becomes name[2:] = garbage, fails startswith("src/") at line 102, and every remaining changed line in that file is skipped — including path entries.
Reproduced against the checker, with a control:
# one '++ ' line early in RadioModel.cpp + a real setMox flip later in the SAME file
$ python3 tools/check_tx_surface.py --repo /tmp/txprobe --base HEAD^1 --pr 9999
TX surface: not touched. # exit 0
# the identical setMox flip alone
TX surface touched (Principle VI):
src/models/RadioModel.cpp: - line uses `setMox`
src/models/RadioModel.cpp: + line uses `setMox`
::error::This PR changes the transmit path... # exit 1
The diff confirms the keying change was present and skipped, not absent:
8:+++ probeCounter;
11:- m_transmitModel.setMox(false);
12:+ m_transmitModel.setMox(true); // probe: keying flipped
This is a defect in the gate's core, not the "a name the list does not carry" boundary the docstring admits to. Independently confirmed by the harness security audit (CodeGuard), which raised the same mechanism plus a second instance: git quotes non-ASCII paths as +++ "b/src/...", so name[2:] yields /src/... and the file drops out of scope.
Drop-in fix:
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
in_hunk = False
for line in diff.splitlines():
if line.startswith("diff --git "):
current = None
in_hunk = False
continue
if line.startswith("@@"):
in_hunk = True
continue
if not in_hunk and line.startswith(("--- ", "+++ ")):
name = line[4:]
if name != "/dev/null":
current = name[2:]
continueWorth a 13th self-test case — {"src/models/RadioModel.cpp": "void f() {\n++ x;\n sendCmd(\"xmit 1\");\n}\n"} must exit 1 with the ack needle — since this is precisely the class the existing 12 cases do not reach.
2. .github/tx-surface.txt:33-40 — the token list covers the backend plumbing but not the model- or producer-level key-on API, so real keying changes pass
The strongest evidence is an open PR. #6292 (client-side voice keyer — a feature whose entire purpose is to key the transmitter) produces zero hits, across 43 changed src/ files:
open PR #6292 vs merge-base 34363a3: 0 hit(s)
Its keying lines read:
+ if (!m_radio.requestProducerPttOn(m_request, TransmitModel::PttSource::Dax))
+ m_radio.requestProducerPttOff(m_request, TransmitModel::PttSource::Dax);
+ if (!m_context.permitsDispatch(TxCoordinator::monotonicMs())) {
requestProducerPttOn/Off is the TxCoordinator producer key-on API and is not a token; TxCoordinator is a path entry only, and #6292 does not modify that file.
Three more probes, each run through the checker, each reporting TX surface: not touched.:
- Removing a TX permission check.
src/models/TransmitModel.cpp:460,if ((route.admit || m_keyingAdmission) && (!permit || !permit())) {→if (false) {. That is the TUNE keying-admission gate — Principle VI itself — and the gate is silent. - The TCI tune keying call.
src/core/TciServer.cpp:3418,tx.startTune(TransmitModel::PttSource::Dax);edited.TciServer.cppis named in the checker's own docstring as a motivating file. - A new, entirely unguarded bridge keying verb added to
AutomationServer.cppcallingcontroller->capture(TxController::Activity::Mox).start()with nom_txAllowedcheck. Note the perverse incentive: adding the guard is what makes the gate fire (m_txAllowedis a token); omitting it does not.
Coverage survey — keying API names in src/ versus the list:
requestProducerPttOn 7 files not listed setTune 32 files not listed
requestProducerPttOff 7 files not listed setAtu 18 files not listed
requestPttOn 9 files not listed Activity::Mox 21 files not listed
requestPttOff 6 files not listed Activity::Tune 8 files not listed
startTune 10 files not listed beginTxActivity 3 files not listed
requestTune 7 files not listed KeyingIntent 3 files not listed
Suggested additions, on top of the setTune|setAtu entry the triage bot already asked for on #6435:
token \b(requestProducerPtt(On|Off)|requestPtt(On|Off))\b # TxCoordinator producer key-on/off
token \b(startTune|requestTune|stopTune|beginTxActivity)\b # TransmitModel key-on entry points
token \bTxController::Activity::(Mox|Cwx|Tune)\b # capture-based key-on, every producer
token \b(setTune|setAtu)\b # TUNE carrier and ATU cycle, every backend
The 382-commit replay should be re-run with these, since the measured 3.4% was computed against the narrower list and will rise. Note this also resolves the rollout worry the other way round: nothing breaks today because recall is too low to break anything.
3. .github/CODEOWNERS — the checker is not Tier 1, so one reviewer-level PR can neuter it
GOVERNANCE's new section says "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." That reasoning applies with equal force to the script that reads the list — but CODEOWNERS has no tools/ pattern at all, so tools/check_tx_surface.py matches only the catch-all * @aethersdr/reviewers (line 111) while .github/tx-surface.txt is @aethersdr/maintainers (line 264).
Failure scenario: one PR adds an early return 0 to main() (or guts the self-test's assertions, which ship in the same merge tree) alongside a keying change, titled "simplify the TX checker", and is approved at reviewer level. Raised by the harness security audit (CodeGuard); I confirmed the CODEOWNERS gap. Fix is two lines beside the existing .github/tx-surface.txt entry:
tools/check_tx_surface.py @aethersdr/maintainers
tools/test_check_tx_surface.py @aethersdr/maintainers
4. RFC #6435 is still open and labelled maintainer-review
GOVERNANCE.md:131: "Do not open a PR until the RFC issue is approved." The RFC was opened 19:12 today, this PR at 19:16, and the triage comment at 19:19 still reads "no PRs are open" — it postdates the PR and leaves two items for the maintainer: the setTune/setAtu gap, and whether the surface is keying-only or matches the repo's wider safety scope (TX power, antenna switching).
The RFC also designed for independent approval — "Three independent branches… Each item can be approved or rejected on its own" — and this PR bundles all three into one diff, which removes exactly that property. B and C are clean and could merge today; A has the three findings above. Per GOVERNANCE this is a maintainer call, not mine: either rule on the RFC and keep the bundle, or split A off so B and C aren't held behind it.
Nits (non-blocking)
tools/check_tx_surface.py:84-86—git diffis called without--src-prefix/--dst-prefix, soname[2:]assumesa//b/. A contributor withdiff.noprefix=truegets "not touched" from the default--base origin/mainlocal run. CI's git config is default, so this is local-only.tools/check_tx_surface.py:46—COMMENT_STARTtreats*+ whitespace as a block-comment continuation, so a statement formatted as a continuation (* 0 + (sendCmd("xmit 1"), 0);) classifies as comment-only. Contrived; noting for completeness (security audit).tools/check_tx_surface.py:54— a//comment that contains*/followed by text (// was sendCmd("xmit 1") */ see #123) is classified as code, so a reworded comment can trigger the gate. False positive, so it costs an ack, not a miss.tools/check_tx_surface.py:104—line[:1] not in "+-"isFalsefor an empty string (Python substring semantics), andcomment_only("")isFalse, so a whitespace-only+/-line in a path-listed file reports "file is on the TX surface". Harmless today (a path-listed file fires anyway), but it is load-bearing if the guard is ever reused.- The gate reads
.github/tx-surface.txtfrom the merged head only, so a PR that shrinks the list is caught by CODEOWNERS Tier 1 rather than by the checker. Unioning the tokens fromHEAD^1:.github/tx-surface.txtwould make it self-defending. Optional — CODEOWNERS does cover it. - PR body, test plan: "Seven that matched only in comments or a help string don't trigger." The triage bot's replay measured eight comment-only plus the help string. Immaterial to the design; worth correcting since the number is cited as evidence.
GOVERNANCE.md§Transmit-path changes: "stale-review dismissal makes it cover the head that merges" depends on a branch-protection setting I cannot read from here. Flagging it as an untested claim, not a defect.
What I tried to break
- Ran the gate, not just read it. 12/12 self-test cases pass locally, and
Static checksis green onbd54059— so both new steps ran against the merge ref, which is the case memory says a new gate usually escapes. The checker on this PR's own diff reports "not touched", correctly. - The CI wiring, where it would most plausibly be wrong.
static-checks.ymlispull_request-only, sogithub.event.pull_request.numberis never empty and--pr ""cannot throw;PR_NUMBERis passed as stepenv:and dereferenced quoted;fetch-depth: 0(line 173) is on the samestatic-checksjob as the new step at 356, so--base HEAD^1resolves against the merge ref. All three held. - Diff-shape attacks.
/dev/nullon either side attributes the right path (add and delete both correct). The---/+++content-line confusion did not hold — Finding 1, with a control proving the same change fires alone. - Live-code keying probes rather than synthetic fixtures: TCI
startTune, a new unguarded bridge verb, removal of the TUNE admission check, and open PR #6292's real diff. Four for four silent — Finding 2. - Part B's leak point. The same
bodyarray reaches bothissues.createComment()on a fingerprint-matched sticky andissues.create()on a new issue (lines 424/432), so attribution appears in the "already red at parent" case that needs it most. Both lookups are individuallytry-wrapped and degrade to"unknown", so neither can stop the issue being filed.permissionsadditions match the two calls exactly;on:is unchanged (push: [main]+workflow_dispatch), so there is no fork-PR path.pr.user.loginanddone.html_urlgo into a REST JSON body with no shell, and GitHub's login charset carries no markdown metacharacters — no injection. - Every CodeGuard lead refutes. All 21
CS-VLN-CMDI-BACKTICKhits are Markdown-quoted terms inside YAML#comments or Python docstrings (lexical rule). The threeSUBSHELLhits and theENVVARhit are pre-existing lines outside this diff — the flagged numbers are the base file shifted by this PR's +4 / +15 lines.CS-MAL-SCRIPTis the repo-standard#!/usr/bin/env python3shebang;CS-PII-EMAIList@example.invalid, an RFC 2606 reserved address used forgit configin a throwaway repo. Two findings from that pass are credited above as security-audit (CodeGuard). - Could not check: the 382-commit replay (the triage bot reproduced it exactly; I did not re-run it — and note it must be re-run if Finding 2's tokens land); the
full-suite.ymlissue step, which is unexercisable until the next redmain, as the body says; thegh apiattribution claim on d3ab349/37fbdbc; and branch-protection settings. No socket tests added or removed.
All findings are reasoned from code and from running tools/check_tx_surface.py against constructed diffs — nothing was compiled, and no probe was a build or a test run.
Recommendation
Request changes, then needs maintainer decision on sequencing.
Part B and Part C are clean — I tried to break both and found nothing. Part A is the right design and the wiring is correct, but two of its findings are substantive: a one-line diff accident disables the gate for a whole file (Finding 1), and the token list misses the key-on API that the repo's largest open TX feature actually uses, so #6292 passes with zero hits (Finding 2). Both are small, mechanical fixes to files already in this PR, plus the two CODEOWNERS lines in Finding 3 and a replay re-run.
Concrete next step: apply Finding 1's parser fix with its 13th test case, add Finding 2's tokens and re-run the 382-commit replay, add Finding 3's CODEOWNERS lines — then it is the maintainer's call whether to rule on RFC #6435 and keep the bundle, or land B and C now and let A follow with the RFC's two open questions (setTune/setAtu, and keying-only vs the safety label's wider scope) settled.
Thanks for this — the gate's idea is sound and the measurement discipline behind it is the reason these findings were cheap to locate. The surface list being Tier 1 is exactly the right escape hatch; it just needs to carry the names the code actually keys through.
🤖 aethersdr-agent · cost: $15.9208 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (28 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $16.5958 · model: claude-opus-5 |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
Issue fit: the Intent section matches RFC #6435's three outcomes: explicit TX acknowledgement, red-main attribution, and intent-first review. The template/review guidance is implemented coherently. The TX gate and attribution only partially deliver their stated outcomes; four independently reproduced defects are detailed inline below.
Scope
| File / group | Claimed purpose | Verdict |
|---|---|---|
.github/tx-surface.txt, checker and self-test |
TX acknowledgement | In scope |
| Static Checks and CODEOWNERS | Enforce and route acknowledgement | In scope |
| GOVERNANCE, CONTRIBUTING, AGENTS | Document acknowledgement | In scope |
| Full Suite workflow | Red-main attribution | In scope |
| PR template and review skill | Intent-first review | In scope |
Everything in the diff is explained by the RFC. No unrelated files, runtime UI changes, removed guards, or CHANGELOG entry.
Blockers
- Missing keying API tokens allow changes to existing keying entry points through without acknowledgement; see the inline comment on
.github/tx-surface.txt:34. - A header-like C++ increment line makes the diff parser skip subsequent keying changes; see
tools/check_tx_surface.py:97. - Renaming a path-listed TX file bypasses its protection; see
tools/check_tx_surface.py:138. - A red parent does not establish that newly failing tests originated earlier; see
full-suite.yml:355.
Non-blocking notes / maintainer decisions
- The checker and self-test remain Tier 3 although the surface list is Tier 1. Jeremy should decide whether disabling the gate should also require maintainer review.
- At
full-suite.yml:337,|| prs.data[0]can name an unmerged associated PR as the source of a direct push. The extracted workflow script reproduced that fallback with an unmerged associated PR. - RFC #6435 has no recorded approval comment and still carries
maintainer-review. GOVERNANCE says “Do not open a PR until the RFC issue is approved.” Since the PR author is the maintainer, record the ruling on keying-only scope and combined delivery rather than leaving it implicit. - The historical replay produced 381 commits / 13 triggers locally, rather than the stated 382 / 13; the trigger count agrees.
Verification and attacks that held up
Reviewed head bd540594261e8c12926fb953b2e3043136984b74 and merge be278fb3f03dc99139217dc71b2b568088cdb840 against current main 47aa4098d2b3182c8608d9bd7f4b6d842758d84a. Refreshed the head, base, checks, decision and live review threads immediately before posting; the head is unchanged and all reported checks are successful.
- Ran
python3 tools/test_check_tx_surface.py: 12/12 pass. Disabling token matching made its existing keying test fail, demonstrating that assertion detects the deliberate break. - Ran the checker against this PR's merge diff: correctly reports “TX surface: not touched.”
- The keying-token control triggered; the increment-line case did not. The actual CLI returned exit 0 for a renamed-and-edited TX-only file; parsing the same change with rename detection disabled found the old TX path.
- Executed the actual extracted workflow JavaScript with stubbed APIs and captured issue bodies. Attribution reaches both new issues and sticky comments. Exceptions in both lookups degrade to “unknown” and still produce an issue body.
- Independently verified the historical parent run and failure signatures through GitHub reads. No live issue-writing workflow was run.
- The new Python tests are socket-free. No application build or GUI session was needed because there are no runtime source changes; no hardware or TX was exercised. Branch-protection settings were inaccessible. No model-invocable automated code-review skill was available; existing reviewer/bot leads were checked independently.
Recommendation: request changes. Fix the four defects and add focused parser, rename and keying-API cases before approval.
| 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 |
There was a problem hiding this comment.
[P2] Include existing keying entry points in the surface
Using this PR's load_surface() and triggers(), added calls under src/models/RadioModel.cpp to setTune, setAtu, setCwKeying, requestProducerPttOn, and startTune each produced an empty hit list. The control sendCmd("xmit 1"); did trigger. These are existing keying paths: IRadioBackend.h:575,605,683 declares the backend verbs, while TciServer.cpp:2576,3418 calls producer PTT and TUNE. The current setKeying regex does not match them, and these call sites are not path-listed. Consequently an author can change those keying calls without the disclosure this PR promises. Add the missing entry points, survey the sibling key-on/wire APIs, and pin them with focused cases. The RFC triage already identifies the setTune/setAtu gap.
| if line.startswith("diff --git "): | ||
| current = None | ||
| continue | ||
| if line.startswith(("--- ", "+++ ")): |
There was a problem hiding this comment.
[P2] Distinguish hunk content from file headers
An added valid C++ line ++ counter; is rendered as +++ counter; by git. This branch treats it as a file header and replaces current with a non-src string, so later keying changes in the same file are skipped. I reproduced zero hits for a diff adding ++ counter; followed by sendCmd("xmit 1");; the identical keying line alone produces src/models/RadioModel.cpp: + line uses "xmit``. Removed -- counter; has the analogous ambiguity. Track whether the parser is inside a hunk and recognize ---/+++ headers only before the first @@; add a regression case with the increment and keying change in the same file.
| 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") |
There was a problem hiding this comment.
[P2] Preserve the old TX path when files are renamed
In a temporary repository, I renamed src/models/TxController.cpp to src/models/TxPermission.cpp and changed one code line (96% similarity). The actual checker returned exit 0 with TX surface: not touched.. Git reports one rename, and the parser checks only the destination path; the original protected path disappears. Running the same diff with --no-renames produced a hit on TxController.cpp. Disable rename detection so deleted old-path code is evaluated, and add a rename-plus-edit test. Pinning prefixes also avoids local diff.noprefix settings defeating the assumed a/b prefixes.
| diff = git(repo, "diff", "-U0", "--no-color", "--no-ext-diff", base, "HEAD") | |
| diff = git(repo, "diff", "-U0", "--no-color", "--no-ext-diff", "--no-renames", | |
| "--src-prefix=a/", "--dst-prefix=b/", base, "HEAD") |
| 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`; |
There was a problem hiding this comment.
[P2] Do not infer a new failure's origin from the parent's overall result
A parent workflow conclusion of failure does not mean the tests failing at this commit already failed there. I independently checked the PR's example: d3ab349's parent is 0cce730; its completed run https://github.com/aethersdr/AetherSDR/actions/runs/38301867624 failed only radio_setup_headphone_output_gate_test (recorded on #6418). d3ab349's run https://github.com/aethersdr/AetherSDR/actions/runs/38302350478 adds fm_tone_presentation_contract, hl2_tci_signaling_test and icom_family_test (#6420). The extracted script still writes “already failure at parent ... — look earlier” for the new failures, directing investigation away from this commit. Compare signatures, or qualify the instruction without asserting attribution:
| : `already ${done.conclusion} at parent \`${p7}\` (${done.html_url}) — look earlier`; | |
| : `${done.conclusion} at parent \`${p7}\` (${done.html_url}). If that run failed the same tests, look earlier; failures absent there may originate at this commit`; |
NF0T
left a comment
NF0T
left a comment
There was a problem hiding this comment.
Review of #6436 at bd540594
I reproduced the blockers already raised by K5PTB, rfoust and aethersdr-agent (header-line confusion, rename bypass, missing keying entry points, the "look earlier" wording in B) against the PR head, and agree with them. I'm not repeating them. Three things are different from what's on the thread:
1. The suggested parser patch doesn't close the non-ASCII path case. The in_hunk change fixes ++ a;, -- a; and the rename, and I checked that in a scratch copy. A path with a non-ASCII character is still silent, because git quotes the header (+++ "b/src/...") and name[2:] becomes /src/.... Adding -c core.quotePath=false to the git diff call closes it. All probes were then gated, and the self-test is still 12/12. It needs its own test case.
2. Which token additions cost anything. I replayed 381 first-parent commits (the PR's window) with each proposed set separately:
| Token set | Triggers |
|---|---|
| list as it stands | 13 |
+ K5PTB's cw key/ptt, setCwKeying/setTune/setAtu, applyKeying/setCwMox/withMox/cmdSetPtt |
13 |
+ the bot's requestProducerPtt*, startTune/requestTune/stopTune/beginTxActivity, Activity::Mox/Cwx/Tune |
17 |
K5PTB's set is free. The bot's set adds 03dcff47c and 2859fdef4 (TCI keying), 74a5ac084 (a TxController::Activity::Mox capture inside a Window-menu commit, not judged) and 85f2a1249. The last matches only the KPA1500 amplifier's own startTune(), so a bare startTune is too loose. I'd take both sets, scope that token, and update the replay numbers in the body.
3. Process note. Four of the changed files are Tier 1, and @aethersdr/maintainers has one member (checked via the org API), who is the author. No second reviewer can clear that gate. How to handle that is the maintainer's call.
Smaller:
full-suite.yml:runs.find(r => r.status === 'completed')prints "already cancelled at parent" if the parent run was cancelled.- The window is 381 commits, not 382; including the 09-13 boundary commit
b8138b830gives 14.
Not tested: the full-suite.yml issue step (only runs on a red main) and branch-protection settings. No src/ change, so I didn't drive the app.
73,
Ryan NF0T
👨🏼💻 Co-authored by Claude Sonnet 5.5
| 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") |
There was a problem hiding this comment.
Adds to the --no-renames suggestion above: a non-ASCII path is still silent after the in_hunk fix, because git quotes the header (+++ "b/src/...") and name[2:] becomes /src/.... core.quotePath=false closes it (checked: gated, self-test still 12/12). It needs its own self-test case.
| diff = git(repo, "diff", "-U0", "--no-color", "--no-ext-diff", base, "HEAD") | |
| diff = git(repo, "-c", "core.quotePath=false", "diff", "-U0", "--no-color", "--no-ext-diff", | |
| "--no-renames", "--src-prefix=a/", "--dst-prefix=b/", base, "HEAD") |
| 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 |
There was a problem hiding this comment.
Replay over the PR's 381-commit window, per token set: list as it stands 13; plus K5PTB's cw/setCwKeying/setTune/setAtu/applyKeying/setCwMox/withMox/cmdSetPtt lines 13; plus the bot's requestProducerPtt*/startTune|requestTune|stopTune|beginTxActivity/Activity::Mox|Cwx|Tune lines 17. One of the four new hits (85f2a1249, KPA1500) matches only the amplifier's own startTune(), so scope that token rather than matching it bare.
| 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`; |
There was a problem hiding this comment.
Agree with rfoust's comment on this line (same evidence: 0cce730 failed only radio_setup_headphone_output_gate_test, d3ab349 added three more, #6420). One more case for the same wording: runs.find(r => r.status === 'completed') also matches a cancelled run, which would print "already cancelled at parent". The softened wording covers that too.
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Review of #6436 at bd540594, against RFC #6435 and main as of 6bac6a089
Issue fit. The Intent section matches the RFC's three outcomes (A: TX-path acknowledgement, B: red-main attribution, C: intent-first review). C is complete. A and B don't yet deliver what the Intent promises, for the reasons K5PTB, rfoust, NF0T and aethersdr-agent gave on Oct 13–14. The head hasn't moved since then, so all of those findings still stand.
This review adds what has changed since: main has moved 67 first-parent commits past this PR's merge base 47aa4098. That gives an out-of-sample test of the gate, which the original 382-commit replay could not provide.
This is posted as a comment, not "request changes", because the account is the PR's author. Read the blockers below as blocking.
Scope
| File / group | Claimed in the RFC/body? | Verdict |
|---|---|---|
.github/tx-surface.txt, tools/check_tx_surface.py, tools/test_check_tx_surface.py |
A | In scope |
.github/workflows/static-checks.yml (2 steps), .github/CODEOWNERS (2 entries) |
A | In scope |
GOVERNANCE.md, CONTRIBUTING.md, AGENTS.md |
A | In scope |
.github/workflows/full-suite.yml (+ actions: read, pull-requests: read) |
B | In scope, permissions disclosed |
.github/PULL_REQUEST_TEMPLATE.md, .claude/skills/pr-review/SKILL.md |
C | In scope |
Everything in the diff is explained by the RFC. There are 455 additions and no deletions, no src/ changes and no CHANGELOG.md entry. git merge-tree against current main is clean, although main has since changed static-checks.yml and AGENTS.md.
Blockers
1. The out-of-sample replay: three keying changes merged since Oct 13 that this gate would have passed. .github/tx-surface.txt:31-40
I replayed triggers() from this PR, with this PR's surface list, over every first-parent commit in 47aa4098..origin/main (67 commits). I used --no-renames and fixed prefixes, so only the token list is under test. Two commits trigger: #6526 and #6548, both through the RadioModel_TxCoordinator.cpp path entry. These three don't:
| Commit | What it does to the TX path | Hit with the proposed tokens |
|---|---|---|
a655e4281 #6292 (client-side voice keyer, titled "Principle VI") |
A new producer that keys the transmitter | src/core/VoiceKeyerTxRoute.cpp: + requestProducerPttOn / requestProducerPttOff |
450f7b396 KAT500 tuner applet |
The ATU pre-tune sweep now keys a TUNE carrier for an external tuner (captureProgram(m_externalTuner ? TxController::Activity::Tune : …Atu)) |
src/gui/AtuPreTuneDialog.cpp: + TxController::Activity::Tune |
839434225 #6593 (one owner for the transmit dax flag) |
Changes which TX audio source the radio keys onto, not whether it keys | none, with either set |
The aethersdr-agent review predicted #6292 would score zero, and it merged with zero. The KAT500 commit is new evidence: a carrier-keying change in a Tier-3 GUI file, exactly the case A exists for, matched by no entry.
With the tokens proposed on the thread, both of the first two are gated, and no other commit in the window triggers:
token "cw (key|ptt)\b # Flex CW key and CW PTT on the wire
token \b(setKeying|setCwKeying|setTune|setAtu)\b # IRadioBackend keying, CW, TUNE carrier and ATU
token \b(applyKeying|setCwMox|withMox|cmdSetPtt)\b # HL2 and Icom keying down to the wire
token \b(requestProducerPtt(On|Off)|requestPtt(On|Off))\b # TxCoordinator producer key-on/off
token \b(requestTune|stopTune|beginTxActivity)\b # TransmitModel key-on entry points
token \bTxController::Activity::(Mox|Cwx|Tune)\b # capture-based key-on, every producer
The second line replaces \bsetKeying\b. This leaves out a bare startTune, per NF0T's KPA1500 replay. #6593 is the scope ruling's question (item 4), not a list defect.
2. The diff parser treats content lines as file headers. tools/check_tx_surface.py:97-101
Still reproduces at the head. In a scratch repo, the change ++ x; followed by setMox(false) → setMox(true) in the same src/models/RadioModel.cpp gives TX surface: not touched., exit 0, with --pr 1. The in_hunk patch on the thread fixes it. NF0T's -c core.quotePath=false is also needed for non-ASCII paths.
3. A rename escapes the path entries. tools/check_tx_surface.py:124,138
Still reproduces at the head. git mv src/models/TxController.cpp src/models/TxPermission.cpp plus a one-line code edit gives TX surface: not touched., exit 0. The fix is --no-renames and fixed --src-prefix=a/ --dst-prefix=b/ on both git diff calls. Please add rename, ++ and quoted-path cases to the self-test. It's 12/12 today and covers none of the three.
4. "Already red at parent — look earlier" misdirects. .github/workflows/full-suite.yml:352-355
This happened twice this week on live main, beyond the d3ab349/#6420 case on the thread:
68ccf459f(#6619) addedshow_tx_in_waterfall_client_flag_testandtransmit_seam_drop_notice_testfailures. Its parentb95edf111was already red (run 39453904244), but not on those tests.933f2fdee(#6621) addedhl2_tci_signaling_test. Its parent68ccf459fwas red too (run 39460580028).
With this PR merged, both tracking issues would have pointed away from the commit that broke them. Use the qualified wording from rfoust's or K5PTB's suggestion. Even better, compute the verdict after the fingerprint lookup: a new fingerprint on a red parent means this commit is the first red for that signature.
Needs maintainer decision (@ten9876)
- The RFC hasn't been approved. #6435 is still open with
maintainer-review. GOVERNANCE.md:130-131 says "Do not open a PR until the RFC issue is approved." The approval comment should also settle the scope question triage raised: keying only, or thesafetylabel's wider scope, which takes intransmit set vox_enable(FlexModelControls.cpp:154) and the transmitdaxflag (#6593 above). - Tier-1 review.
@aethersdr/maintainershas one member, who is the author (NF0T). Nobody else can clear the Tier-1 files here. - Bundling. The RFC promised items "approved or rejected on its own". The squash merge collapses them into one, while B and C have far fewer findings than A.
- Rollout. Once the fixes land, open PRs that reach the surface go red: #4921, #4920 and draft #4770, plus any open PR built on the keyer or tuner code above. A note on each is worth posting when this merges.
Nits (non-blocking)
- CODEOWNERS: put
tools/check_tx_surface.pyandtools/test_check_tx_surface.pyunder@aethersdr/maintainers, next to.github/tx-surface.txt. Otherwise, a reviewer-level PR can disable the gate that the Tier-1 list feeds. full-suite.yml:337:|| prs.data[0]can name an open PR as the source of a direct push. Use.find(p => p.merged_at)alone.full-suite.yml:runs.find(r => r.status === 'completed')also matches a cancelled parent run.tools/check_tx_surface.py:46: an indented* p = setMox(true);reads as a block-comment continuation. Accept that as a documented limit.- Body: the window is 381 commits, not 382, and there were eight comment-only matches, not seven. Once the token set changes, re-run the replay and update the numbers.
What I verified, and what held up
- Ran
tools/test_check_tx_surface.pyat the head: 12/12. - Reproduced blockers 2 and 3 with the PR's checker against scratch repos (outputs quoted above).
- Ran the replay of item 1 with the PR's
triggers()andload_surface(), over the 67 commits after the merge base, with the current and the proposed token sets. The only new hits from the proposed set are #6292 and450f7b396, both true keying changes, so the extra tokens cost no false positives in this window. - Item 4's run conclusions come from
gh api …/full-suite.yml/runs?head_sha=. The failing test names come from the tracking issues on main. - Every
pathentry intx-surface.txtstill exists on current main, so no path has gone stale since Oct 13. - The PR merges cleanly onto current main.
- Not exercised: the full-suite issue step itself, which only runs on a red main. This PR has no
src/changes, so I didn't drive the app. The new test is socket-free: temporary git repos, no network.
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit / intent: RFC #6435 proposes explicit TX disclosure, useful red-main attribution, and intent-first review. The three commits match those three purposes. Intent-first review is implemented coherently; TX detection and attribution only partially deliver their stated outcomes. I independently reproduced the existing reviewer leads against unchanged head bd540594 rather than treating those reviews as proof.
Scope
| File/group | Claimed by RFC/body? | Verdict |
|---|---|---|
.github/tx-surface.txt, tools/check_tx_surface.py, tools/test_check_tx_surface.py |
TX acknowledgement | In scope; detection gaps below |
.github/workflows/static-checks.yml, .github/CODEOWNERS |
Enforce and route acknowledgement | In scope |
GOVERNANCE.md, CONTRIBUTING.md, AGENTS.md |
Document acknowledgement | In scope |
.github/workflows/full-suite.yml |
Red-main attribution and read permissions | In scope |
.github/PULL_REQUEST_TEMPLATE.md, .claude/skills/pr-review/SKILL.md |
Intent-first review | In scope |
All 11 files and three signed commits are explained by the RFC. No src/, runtime default, user-interface or CHANGELOG changes. The acknowledgement directory correctly overrides the earlier Markdown ownership rule. The out-of-repository bot allowlist change remains explicitly excluded.
Blockers
-
[P2] Existing keying entry points are absent from the surface —
.github/tx-surface.txt:35–40. A changed call tostartTune,setTune,setAtu,requestProducerPttOn,sendCwKey,setCwKeying, orwithMoxcan produce no hit. These are existing operations, not hypothetical names: for example,MainWindow_SwrSweep.cpp:251callstransmitModel().startTune(),TciProtocol.cpp:1636callssendCwKey,RadioModel.cpp:4577calls backendsetCwKeying, andMetisClient.cpp:1482useswithMoxfor the outgoing MOX bit. Probes using the actual call names returned no hits. The RFC triage already requestedsetTune/setAtu. Cover the canonical key-on APIs across supported backends, add negative tests, and repeat the historical measurement after changing the vocabulary. Scope generic names carefully: an amplifier's ownstartTuneis not necessarily radio keying. -
[P2] Hunk content can replace the parser's current filename —
tools/check_tx_surface.py:97–103. Adding valid C++++ counter;produces a diff line beginning+++; the parser treats it as a file header. A followingsetKeying(true);then exits 0, “TX surface: not touched.” The same keying change alone exits 1 and requests an ack. Git-quoted Unicode source paths also exit 0 becausename[2:]does not decode the quoted filename. Distinguish metadata from hunk content, decode Git filenames, and pin path-prefix assumptions. Add both cases to the self-test; the current 12 do not cover them. -
[P2] Renaming a TX-only file drops its protected old path —
tools/check_tx_surface.py:138. A 98%-similar rename fromsrc/models/TxController.cpptosrc/models/Permissions.cpp, changingbool allowed = false;totrue, exits 0 without an ack. The+++destination overwrites the original path, so removed lines are never checked as changes to the listed TX file. Disabling rename detection retains that evidence. A drop-in replacement for the diff call is:diff = git(repo, "diff", "-U0", "--no-color", "--no-ext-diff", "--no-renames", "--src-prefix=a/", "--dst-prefix=b/", base, "HEAD")
This fixes the rename case and pins prefixes; it does not replace the header/quoted-path fix in finding 2. Add a renamed-and-edited protected-file regression.
-
[P2] A red parent does not establish that this failure began earlier —
.github/workflows/full-suite.yml:355. Executing the actual extracted script with a failed parent and a new failure signature still produces “already failure … look earlier.” The historical example makes the distinction concrete: parent0cce7309failed onlyradio_setup_headphone_output_gate_test, whiled3ab349's tracking issue #6420 addsfm_tone_presentation_contract,icom_family_testandhl2_tci_signaling_test. The new wording would point away from the commit introducing those failures. Either compare signatures or keep the advice conditional. A truthful replacement for the final ternary arm is:: `parent \`${p7}\` concluded ${done.conclusion} (${done.html_url}); compare failing tests before attributing this failure`;
Nits / maintainer decisions (non-blocking)
-
tools/check_tx_surface.py:46–54: the comment heuristic also drops valid* permission = true;in a path-listed file, while// text */ setKeyingis falsely counted as code. Both reproduced. Preserve these as regression cases when improving comment classification. -
.github/workflows/full-suite.yml:337: the fallback attributes a direct push to an unmerged associated PR and mentions its author. The extracted-script fixture reproduces this. At minimum, remove the unmerged fallback:const pr = prs.data.find(p => p.merged_at);
-
.github/CODEOWNERS:264: the surface list is maintainer-owned but the checker and its self-test still use the reviewer catch-all. Jeremy should explicitly decide whether changes that disable the disclosure check need the same ownership as changes that shrink its list. This is a design decision, not a fabricated existing governance rule. -
GOVERNANCE.md:130–131says, “Do not open a PR until the RFC issue is approved.” The RFC thread still has no recorded maintainer approval. Record the ruling, including keying-only versus broader equipment-protection scope and whether the three items should land together. The maintainer is the PR author; this review does not infer a rejection from the absent comment.
Verification and attempts to break it
- 12/12 supplied self-tests pass. Disabling token matching in a disposable copy makes six cases fail; the copied source was restored exactly. Positive keying, empty-ack, add/delete, comment-only and unrelated-change controls behave as expected. Parser and rename failures above were exercised through the actual CLI against temporary Git commits.
- Replayed the history with an explicit cutoff of
2026-09-13T19:06:52Zthrough base47aa4098: 382 commits / 13 triggers. Midnight UTC includes more commits, so the cutoff matters. This measurement describes the current narrow vocabulary; it does not establish complete keying coverage. - Executed the actual workflow JavaScript with stubbed APIs and captured issue bodies only. Both create and sticky-comment paths include attribution; lookup exceptions degrade to unknown without preventing the captured issue. A cancelled parent is also inconclusive, rather than evidence to “look earlier.” Historical GitHub reads confirm both the failed-parent example and the successful parent of
37fbdbc. - Both workflows parse as YAML. Strict engine-boundary, touchpoint-manifest, capability-record and command-plane checks pass identically at base and head. All three commit signatures verify. Existing CodeGuard leads were examined; lexical matches in comments/docstrings, standard shebangs, the reserved fixture email and unchanged workflow lines do not establish defects. No optional model-invocable automated review skill was available.
- Required checks are green, but those runs were created on October 13 at 19:16:40Z; current main is
6bac6a08, October 21 at 04:28:28Z. Branch protection hasstrict: false, so that CI does not prove the current merge. Locally, the checker correctly reports “not touched” for this PR's current merge9deb3f51against current main. Stale-review dismissal is enabled, as the proposal assumes. I did not rerun every unrelated Static Checks step or dispatch the live Full Suite issue-writing workflow.
Preflight found no socket-owning tests. No application build or bridge session was needed because this PR changes governance and CI only. No hardware, transmit operation, issue creation, workflow dispatch or secret-value access was used.
Recommendation: Request changes. Fix the detection and attribution defects and add focused regressions, then refresh CI on the resulting head. Approval and the RFC scope/ownership decisions remain with the maintainer. This review uses COMMENT because the authenticated account is also the PR author, which prevents a self-request-changes event.
|
Closing in favor of a split, per the reviews above:
Thanks to K5PTB, rfoust, NF0T and the agent review. Their replays and probes are what showed the name list couldn't hold. |
…6435 B, C). Principle VIII. (#6652) ## Summary Implements items B and C of RFC #6435. Item A, the transmit-path acknowledgement, is being reworked in the RFC and stays out of this PR. B and C are in separate commits. 1. **Red-main attribution** (Principle VIII). The `full-suite.yml` tracking issue now gives: - The commit's source PR and its author. A direct push says so instead of naming an unmerged PR. - How the commit compares with its parent's full-suite run: - A green parent makes this commit the first red one. - A red parent is compared test by test against the failed list filed for the parent run the verdict links, matched by run id. Tests failing here but not there are named as starting at this commit. If nothing fails here that passed there, it says "look earlier". - If either side has no complete test list, the verdict states both possibilities instead of guessing. That covers a missing parent entry, two lists for one run, a build failure, "unknown" and a list capped at 25 names. - The failed list keeps every test in CTest's FAILED summary, whatever its status. A crash (`ILLEGAL`, `NUMERICAL`) can no longer make a partial list look complete. - A cancelled parent run is skipped. - A pointer to the #4703 flake umbrella, and a reminder that reverting is a human decision. This adds `actions: read` and `pull-requests: read` to the workflow's permissions. A new Static checks step, `node tools/test_full_suite_attribution.mjs`, runs the issue step's script against stubbed GitHub data, plus the failed-list producer against a CTest summary. 2. **Intent-first review** (Principle VII). - The PR template gets an **Intent** section, and `/pr-review` checks it against the linked issue before reading the code. - Asking for validation inside trusted code, or for abstraction the stated intent doesn't need, is a nit, not a blocker. Part of #6435. Supersedes the B and C commits of #6436. ## Intent After this merges: - A red main names its likely PR, and which of its failing tests started at that commit. - Reviews start from what a PR is for. Out of scope: - The transmit-path gate (RFC #6435 item A). - Any automatic revert. ## Constitution principle honored - **Principle VII:** the review rule cites "so the interior can treat parsed values as sound". - **Principle VIII:** a red main carries its own attribution. ## Test plan - [x] `node tools/test_full_suite_attribution.mjs`: 18/18. - Against the previous commit it fails 6 of 18. - Breaking the run-id match fails the two-run case. - Accepting the first of several lists fails the re-run case. - [x] Ran the issue step's script against real main commits, with `github` stubbed by read-only `gh api` calls: - `37fbdbc` (#6413): green parent, so it is the first red commit. - `d3ab349` (#6199): names `fm_tone_presentation_contract`, `icom_family_test` and `hl2_tci_signaling_test` as starting there. #6436's version said "look earlier" here. - `68ccf459f` (#6619): names `show_tx_in_waterfall_client_flag_test` and `transmit_seam_drop_notice_test`. - `933f2fdee` (#6621): names `hl2_tci_signaling_test`. - [x] `full-suite.yml` and `static-checks.yml` parse as YAML. - [ ] The step only runs on the next red main, so the live issue write is unexercised until then. ## Checklist - [x] Commits are signed (`docs/COMMIT-SIGNING.md`) - [x] No `src/` changes - [x] Documentation updated: PR template, `/pr-review`. No `CHANGELOG.md` entry. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_012U8wmr3AQGVrmQLp2zNBwa --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
4 participants
Summary
Implements RFC #6435, three review-and-CI changes based on Anthropic's Redesigning source control. Each one is a separate commit:
Transmit-path acknowledgement (Principle VI). Keying code is spread across Tier-3 files that are too large to gate by path, so this gate looks at what the diff contains instead.
.github/tx-surface.txt(Tier 1) lists the transmit surface: whole files, plus tokens such as thexmit/transmit tune/atu start/cwx sendwire commands,setKeying/setMox/setPtt, keying-control registration and the bridge's TX opt-in.tools/check_tx_surface.pyruns in Static checks. A PR whose non-commentsrc/lines reach the surface fails until it adds.github/tx-acks/<PR number>.md, stating what changes on the TX path and how it was verified..github/tx-acks/is owned by@aethersdr/reviewers. Covered in GOVERNANCE §Transmit-path changes, CONTRIBUTING and AGENTS.md.Red-main attribution. The
full-suite.ymltracking issue now gives:This adds
actions: readandpull-requests: readto the workflow's permissions.Intent-first review.
/pr-reviewchecks it against the linked issue before reading the code.Closes #6435
Intent
After this merges:
Out of scope:
.github/tx-acks/. That is a separate one-line change to the bot'svalidate-diff.sh, which lives outside this repo. Until it lands, a bot PR that touches TX code needs a human to add the ack.Constitution principle honored
Test plan
tools/test_check_tx_surface.py: 12 cases. A keying change with no ack fails, one with an ack passes, and an empty ack fails. Comment-only lines and a UI string pass. A#defineor code after/* */still counts as code.python tools/…step instatic-checks.ymlpasses locally on this branch.gh api: d3ab349 (refactor(aetherd): route remaining slice and transmit model controls #6199) reports that main was already red at its parent, and 37fbdbc (feat(gui): Radio Setup is a CanonWindow workspace, with a UX pass over every page. Principle VIII. #6413) reports itself as the first red commit.full-suite.ymlandstatic-checks.ymlparse as YAML.Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls (nosrc/changes)CHANGELOG.mdentry.🤖 Generated with Claude Code