[ten9876]

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 Three tests fail on main at high rates on Linux, and CI cannot see them (it runs ~6 of 242) #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 [RFC] Route review by what a diff does: TX-path acknowledgement, red-main attribution, intent-first review #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

• 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.
• Ran the issue step's script against real main commits, with github stubbed by read-only gh api calls:
  • 37fbdbc (feat(gui): Radio Setup is a CanonWindow workspace, with a UX pass over every page. Principle VIII. #6413): green parent, so it is the first red commit.
  • d3ab349 (refactor(aetherd): route remaining slice and transmit model controls #6199): names fm_tone_presentation_contract, icom_family_test and hl2_tci_signaling_test as starting there. feat(governance): route review by what a diff does — TX-path ack, red-main attribution, intent-first review (RFC #6435) #6436's version said "look earlier" here.
  • 68ccf459f (refactor(aetherd): consolidate M4 desktop utilities #6619): names show_tx_in_waterfall_client_flag_test and transmit_seam_drop_notice_test.
  • 933f2fdee (refactor(aetherd): type slice lifecycle and split routing. Principle VI. #6621): names hl2_tci_signaling_test.
• 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

• Commits are signed (docs/COMMIT-SIGNING.md)
• No src/ changes
• Documentation updated: PR template, /pr-review. No CHANGELOG.md entry.

🤖 Generated with Claude Code

https://claude.ai/code/session_012U8wmr3AQGVrmQLp2zNBwa

[ten9876]

How #6652 handles the review feedback on #6436 for items B and C. #6436's other findings are about item A, which is out of this PR and being redesigned on #6435.

Item B (red-main attribution, .github/workflows/full-suite.yml)

Feedback on #6436	From	Here
"Already red at parent — look earlier" misdirects when a commit adds failures to an already-red main (d3ab349 → #6420)	K5PTB, rfoust, NF0T, my review (#6619, #6621)	Fixed. The verdict compares failing tests against the parent's tracking-issue entry and names each test that starts at this commit. Only an identical set says "look earlier". Against d3ab349 it now names fm_tone_presentation_contract, icom_family_test and hl2_tci_signaling_test.
Sharper version: decide per failure signature, not by the parent's overall result	K5PTB	Done at the test level, which is finer than the fingerprint.
|| prs.data[0] names an open PR as the source of a direct push	K5PTB, rfoust	Fixed: prs.data.find(p => p.merged_at) alone, so a direct push reads "none (direct push)".
runs.find(r => r.status === 'completed') matches a cancelled parent run	NF0T	Fixed: only a success or failure run counts. Otherwise it reads "no finished run for parent".
Each lookup must stay non-blocking, and the attribution must reach both the new-issue and sticky-comment paths	aethersdr-agent, rfoust (held up)	Still holds. The new parent lookup is inside the same try, and body is unchanged in shape.
Body: the d3ab349 example claimed "already red at its parent"	K5PTB, rfoust	Test plan rewritten with the six real and constructed cases.
New, found while checking this PR: a build failure on either side parsed as a test named none, and "unknown" parsed as "the same failures"	—	Fixed in 7995b921c: no test-by-test verdict without two complete lists.

Item C (intent-first review, PR template and /pr-review)

No reviewer raised a defect. K5PTB: "C is complete and correct". aethersdr-agent: "B and C I found nothing to fix in". rfoust: "implemented coherently". The commit is carried over unchanged.

Process

Feedback	Here
The RFC promised items approvable on their own, and #6436 bundled them	Split: B and C here, A redesigned on #6435.
RFC #6435 had no approval comment (GOVERNANCE.md:130-131)	The amendment on #6435 records B and C as approved. Scope for A: keying, plus VOX enable.
Only one member of @aethersdr/maintainers, who is the author, can clear Tier-1 files	Doesn't apply here. full-suite.yml, the PR template and SKILL.md all resolve to @aethersdr/infrastructure (CODEOWNERS:118, 205), so any infrastructure code owner can approve.
No CHANGELOG.md entry	None.

Not exercised: the issue write itself, which only runs on a red main.

[ten9876]

Issue fit / intent: This implements approved RFC #6435 items B and C: identify newly failing tests on a red main and start reviews from stated intent. The four historical examples reproduce correctly, and the earlier direct-push, cancellation and unknown/build-list cases are handled. Two remaining cases can still produce a definite but incorrect attribution, so item B is only partially complete. Item C is coherent; item A is correctly excluded.

Scope

File/group	Claimed by RFC/body?	Verdict
.github/workflows/full-suite.yml	B: attribution and read permissions	In scope; findings below
.github/PULL_REQUEST_TEMPLATE.md	C: Intent section	In scope
.claude/skills/pr-review/SKILL.md	C: intent-first and trusted-interior guidance	In scope

All three files and commits are explained. The October 13 author dates are the disclosed B/C commits carried over from #6436; the third commit fixes incomplete-list handling. All signatures verify. The maintainer explicitly approved B/C in RFC comment 6315384675. No TX-gate, runtime source, unrelated policy or CHANGELOG changes.

Blockers

1. [P2] Compare the report for the selected parent run — .github/workflows/full-suite.yml:354–357, :370–380. done selects a workflow run, but failedAt(p7) returns the first report containing that commit's short SHA, without checking the run linked in the verdict. A parent can have multiple runs with different failure sets. Issue ordering is by most recently updated, so a later run of another commit can move an older signature's issue ahead of the selected parent's report.

   Reproduced with two distinct, correctly fingerprinted issue buckets, each containing only its own signature: an older parent run failed alpha_test; the selected/latest parent run failed beta_test; a subsequent main run repeated alpha and updated its issue. With current failures beta, the actual script linked the beta parent run but printed “beta_test failed here and not there: those start at this commit.” Reversing the entries also made the “same failures” verdict describe a different set from the linked parent run. No edited or mixed-fingerprint issue was needed.

   Tie the archived list to the selected run and attempt, or use the existing unknown/incomplete-list fallback when matching reports are ambiguous. Matching only the shortened commit is insufficient. Preserve this multiple-run/multiple-issue case in a regression test.

2. [P2] A nonempty FAILED summary is not necessarily a complete list — .github/workflows/full-suite.yml:384–385, using the producer at :257. Real CTest output can contain ILLEGAL and NUMERICAL; the existing producer drops both. With a parent failing old_test, and the current run failing that test plus two new crashing tests, the new listed() guard accepts the remaining old_test (Failed) as complete and prints “with the same failures — look earlier.” The new failures are absent from the attribution even though CTest's full summary and fingerprint contain them.

   A local socket-free CTest fixture produced exactly:

   old_test (Failed)
   new_sigill_test (ILLEGAL)
   new_sigfpe_test (NUMERICAL)
   

   Feeding the production-extracted summary to the actual issue script reproduced the false verdict. Including the omitted statuses made the script name both new tests correctly. At minimum, the producer needs this replacement for those demonstrated cases:

          allfailed=$(sed -n '/^The following tests FAILED:/,$p' full-suite.log | grep -oE '[A-Za-z0-9_.-]+ \((Failed|Timeout|Subprocess aborted|Not Run|SEGFAULT|ILLEGAL|NUMERICAL|Child aborted)[^)]*\)' | sort)

More generally, only assert completeness when the comparison data preserves every failed name from the CTest summary; the existing all-name extraction used for the fingerprint is another available source. Test an existing ordinary failure plus a newly crashing test, so a partial nonempty list cannot silently pass this guard.

Nits (non-blocking)

• .github/workflows/full-suite.yml:395: “the same failures” also covers a strict subset of the parent's failures. Parent {a,b} and current {a} reproduce that wording. No current failure is incorrectly cleared in this case, but “no newly failing tests” would accurately describe the branch and match the PR's intent.
• .github/workflows/full-suite.yml:345–395: the substantial comparison logic has manual test evidence but no committed regression harness in this PR. Keep the demonstrated edge cases in repeatable socket-free coverage so later changes can verify them without live issue writes; no synthetic firmware peer is needed.

Verification and attempts to break it

• Executed the actual extracted issue-step JavaScript with local API stubs, capturing both create and sticky-comment bodies. Green parent, added failures, equal/subset sets, missing/capped/build/unknown lists on either side, cancelled runs, direct pushes and lookup exceptions behaved as described, apart from the subset wording. The two production-shaped repeated-run cases above did not survive.
• Independently fetched read-only GitHub records and replayed all four historical examples through the actual script: 37fbdbc identifies the first red commit; d3ab349, 68ccf459f and 933f2fdee name exactly the new failures claimed in the body. No GitHub write endpoint was called.
• The real CTest crash fixture and recognized-new-failure positive control distinguish the incomplete-list defect from a general script failure. The proposed status extension was checked in a disposable parsing probe; repository sources remain unchanged.
• Workflow YAML parses. Strict engine-boundary, touchpoint-manifest, capability-record and command-plane checks pass with identical per-file output on base 6bac6a08 and head 7995b921. No baseline or manifest changes. Optional automated review skill unavailable; independent direct audits performed.
• Static Checks and CodeQL are green; Linux, Windows and macOS CI are still running at this check. Main remains 6bac6a08 (04:28:28Z), older than the exact-head CI/Static run creation at 12:03:45Z, so no stale-main gap was observed. These filtered checks do not establish a full-suite pass.

No application/bridge session was needed because there are no runtime source changes. No hardware, transmit operation, real issue write or workflow dispatch was exercised. The live issue-writing step remains untested, as disclosed in the body.

Recommendation: Request changes. Bind comparisons to the correct parent run and preserve all failed tests before making a definite attribution, then rerun the focused cases. This is a COMMENT review because the authenticated account is also the PR author, so GitHub does not allow a self-request-changes event.

[ten9876]

Reply to review 5599459747. Everything is fixed in 0ed3cc4c1.

1. Compare against the parent run the verdict links (.github/workflows/full-suite.yml, failedAt). Fixed.

• failedAt(done.id) now matches the entry whose **Run:** line names that run id, not the commit's short SHA.
• Two distinct lists for one run (a re-run attempt) count as ambiguous and give the two-sided wording.
• Your reproduction is the case parent with two runs, older one in the fresher issue in the new harness. The newest parent run (id 11) failed beta_test, an older run (id 10) failed alpha_test in the fresher issue, and the current run fails beta_test. The verdict now reads "nothing fails here that passed there". The previous commit named beta_test as new.
• Mutations: hard-coding the run id fails that case, and accepting the first of several lists fails two lists for one parent run.

2. A nonempty FAILED isn't necessarily complete (the extract step's allfailed=). Fixed in the producer.

• It now takes every N - name (status) line of CTest's FAILED summary, whatever the status. That's the same anchor the fingerprint uses.
• Case crashing tests on a red parent runs the producer line from the workflow on a summary with old_test (Failed), new_sigill_test (ILLEGAL) and new_sigfpe_test (NUMERICAL). The verdict names both new tests. The previous producer dropped them, and the verdict said "look earlier".
• A second case checks that the producer keeps all eight statuses, including a line with labels after the status.

Nit: subset wording. Fixed. With no new failures the verdict says "red at parent …, and nothing fails here that passed there — look earlier". It's correct for an equal set and for a subset; case fewer failures covers the subset.

Nit: committed regression coverage. Added as tools/test_full_suite_attribution.mjs, run in Static checks as "Red-main attribution". It's stdlib Node with no network.

• It pulls the issue step's script out of the workflow, runs it against an in-memory GitHub, and reads the written issue body.
• 18/18 pass. Against the previous commit's workflow, 6 fail.
• I also replayed the four historical commits read-only after the change. 37fbdbc, d3ab349, 68ccf459f and 933f2fdee give the same verdicts as before, so run-id matching works against the real tracking-issue format.

Still unexercised: the live issue write, which only runs on a red main.