Repository navigation
docs: cut review latency — comment policy, AGENTS.md split, condensed comments, Constitution 2.1.0, body-only reviews. Principle VIII. - #6072
Merged
Conversation
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
Contributor
|
Checked the HL2 side of this: the 19 files under Three lines need a fix:
Everything else holds. The code is unchanged: my own comment-stripping comparison gives identical hashes for all 37 source files. The register addresses, bit positions, calibration tables and transmit safety notes I looked for are all still there, and the Two notes for our side. Much of the comment volume this PR measures is mine; I will keep to the policy in my open PRs from here on. And eight of them (#5906, #5976, #5977, #5979, #5980, #5982, #6005, #6011) conflict with this sweep in the files it condenses. Do not wait on them: once this lands I will merge main into each and keep the condensed comments. 73, ON8ST |
ten9876
force-pushed
the
docs/review-latency
branch
from
October 1, 2026 16:59
a17b20c to
3b26283
Compare
ten9876
changed the title
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
16 tasks
ten9876
changed the title
6 of 16 tasks
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1. Issue fit
No linked issue, and per GOVERNANCE.md a bug fix with a clear root cause doesn't need one — but this isn't a bug fix. It's a documentation restructure, a change to the review policy itself, and a test retirement/re-home pass. The PR body carries the problem statement (median TTM 0.07h → 29h, 63% of it first-reviewer wait) and that framing is sound. Part 1 solves what it claims, and I was able to independently reproduce its central claim. Part 2 is a different kind of change riding the same branch — see scope.
A note on review mechanics: this PR grew five commits (b83e4b19…5cf537f5) and was retitled docs: → chore: after my review task was created. My task snapshot carried only the four docs commits and a 30-file diff. I reviewed the real head 5cf537f5 (637 files, +14,811/−46,048), not the snapshot.
2. Scope
| Files | What it changes | Claimed? | Verdict |
|---|---|---|---|
581 files under src/ |
Condenses every 10+ line // block |
Yes (10f7517, 3b26283) |
Verified comment-only — see below |
AGENTS.md, docs/agents/{backends,settings,gui,tests-ci,flex-protocol}.md |
1,285 → 301 lines + 5 path-scoped sub-docs (total 1,093) | Yes (f4ec9c9) |
In scope; content audited, no rule dropped |
GEMINI.md, .github/copilot-instructions.md, docs/DEVELOPER-GUIDE.md, docs/aetherd-*.md, docs/architecture/aetherd-iradiobackend-design.md |
Inbound pointer updates | Yes | In scope |
.claude/skills/pr-review/, pr-land/, release-prep/SKILL.md |
Changes the review and landing policy, incl. removing the "deleted comment naming a symptom" heuristic | Yes (cf7fdf2) |
Needs maintainer decision — self-referential; see Blockers §1 |
.github/CODEOWNERS |
docs/agents/ @aethersdr/infrastructure |
Yes, and self-flagged | In scope; ordering verified correct |
tools/check_engine_boundary.py, tests/tests.cmake (1 line) |
Doc-reference text inside comments only | Yes | In scope; no behaviour |
~39 files under tests/ + tests.cmake |
Retire 4 tests + 3 manual clients, re-home 11 as socket-free, delete IcomFakeRadio.h |
Only in the updated title/body | Recommend unbundling — see Blockers §2 |
Nothing in the diff is unexplained by the body. The body's self-certification holds. No CHANGELOG.md entry — correct, and the PR correctly does not add one.
3. Blockers
None that break users or fail on current main. Two items are maintainer calls rather than defects:
1. The policy change is self-referential — it needs a ruling, not my approval. (inline on .claude/skills/pr-review/SKILL.md)
cf7fdf20 rewrites /pr-review so that "deleting or shortening a comment while its guard and test stay … do not flag it", and drops both the #[0-9]{3,} grep term and the "a removal that deletes a comment naming a symptom — quote it back and ask what now prevents it" instruction. The same PR then deletes ~24,000 comment lines. Under GOVERNANCE.md that ordering is the thing a maintainer should rule on, because no independent ruling preceded the exemption.
To be fair to the change: I reviewed this PR under the old rule (my task shipped the pre-merge skill text), and it is clean under both. My comment-stripping comparison shows no guard, early return, confirmation, or any other code construct was removed anywhere in the 581 files — every deletion is prose. So the rule change didn't launder anything here. It is still a review-governance edit that outlives this PR, and the head file is internally consistent (no leftover contradictory row in the verdict table).
2. Part 2's blast radius makes it effectively unreviewable in this PR. (inline on tests/tests.cmake)
The test half deletes 15 test files and adds 11, including hl2_backend_test.cpp → hl2_backend_seam_test.cpp + hl2_backend_session_scope_test.cpp and icom_backend_test.cpp/icom_session_test.cpp → four new targets. Verifying assertion parity across those D/A pairs is the whole review — and nobody is going to do it inside a 46,000-line deletion. The work itself looks good (details below); the ask is to land b83e4b19…5cf537f5 as their own PR. Note the substance is canon-aligned remediation, so this is a packaging request, not an objection.
4. Nits (non-blocking)
AGENTS.md§"Comments" says "The guard and its test are the protection; the prose is not" — andb83e4b19moves a protection into prose. That commit retireswdsp_nb_hold_invariant_testand states the NB-hold invariant is "now guarded only by theWdspChannelcomment". Deleting a source-text grep test is exactly whatdocs/agents/tests-ci.mdasks for ("prefer behavioral seams over source-text assertions"), so the deletion is right — but the replacement should be a seam test (driveprocessIq()with hold set, assert noflush_anbEXT), not a comment. I did check the comment survived condensation:src/core/dsp/WdspChannel.cpp:1022-1025still states it ("No flush is scheduled… The onlyflush_anbEXTis insetNoiseBlanker"), so nothing was lost twice over.- The four new coverage gaps don't appear in #5254's body. The body says "All four are listed on #5254"; #5254's
updated_atmatches this PR's to the minute, so they were likely added as a comment. #5254 records its existing residues as body checkboxes — promoting these four there too would keep them tracked the way the others are. (Conversely: this PR appears to close several existing #5254 residues — the IC-9700 consumer half, the bridgeidrejections, the HL2 link-stats non-event predicates. Worth claiming those explicitly.) src/core/backends/icom/IcomProtocol.h:38— the rewrite reads "Renew the token 20 s before the 60 s expiry", but the constant it documents (kTokenRenewEarlyMs = 20'000) meant renew every 20 s, i.e. three attempts inside the contract; that rationale was the deleted text's whole point. Low stakes because the same comment correctly flags the constants as dead — which brings up the better fix: all fourkToken*constants are unreferenced (I greppedsrc/andtests/), so deleting them beats documenting them. Out of scope for this PR either way.- Body says AGENTS.md was 1,304 lines; at merge-base
538ba69ait is 1,285. Trivial, but this PR is about accurate docs.
5. What I tried to break
- "No behaviour change" across 581 files — attacked hardest, survived. I wrote an independent C++-aware comment stripper (handles
"/'/raw strings/digit separators) and compared stripped base vs stripped head for all 581 files: 0 files differ. My first attempt flagged 7 files; all 7 were my own char-literal bug, and reading each diff by hand confirmed comment-only (e.g.IcomModels.cpponly adds/removes/*hasNetwork*/-style inline labels around identical values;AnanDroopCalibrator.hdrops a} // namespacetrailer). This independently reproduces the body's "581 files, 0 non-comment changes" claim rather than taking it. - Comment-deletion failure modes that a stripped-text diff would miss. Grepped added lines for
//comments ending in\(would comment out the next line): none. Grepped removed lines forNOLINT,IWYU pragma,clang-format off/on,cppcheck-suppress,NOSONAR,coverity: none — no tool-directive comment was deleted. Unbalanced/*would have shown as a stripped-code difference; none did, and all three compile jobs were green on3b26283. - Meaning drift in condensation — sampled, not exhaustive. I read 3 of ~2,000 condensed blocks line-by-line against their originals:
CwSidetoneGenerator::setKeyDown/reset(spinlock +m_lastQueuedStamprace invariants preserved),AnanDroopCalibrator::saveTables(the load-bearing MERGE semantics preserved; only the "Principle XIV" citation dropped, per the new policy), andWdspChannel::setNoiseBlankerHold. One rewrite I expected to be slop turned out to add a verifiable fact the original lacked: thekToken*"currently unused" note is true (grep: zero references), and theIcomSession::Paramsvalues it redirects to are exactly right (30 s / 60 s / 3 s / 80 s atIcomSession.h:53-60). 3 of ~2,000 is a thin sample — I cannot assert the other ~1,997 preserve meaning. - Rule loss in the AGENTS.md split. Diffed every
##/###heading base→head and traced each to its new home. All 37 land somewhere. The five small sections that lost their own heading (MeterSmoother, on-screen labels, BandPlanManager, CHAIN, auto-generated Contributors) are all indocs/agents/gui.md§"Widget conventions", substance intact. Spot-grepped 19 load-bearing tokens (SettingsCredentialPolicy,FROZEN_BOOL_COUNT,settingsScope,exit 77,verify_slice0_rx,VENDOR_STEMS_PINNEDneighbours, …) — all present. Every relative markdown link inAGENTS.md/CLAUDE.md/GEMINI.md/copilot-instructions/docs/agents/*resolves; the#5554anchor target exists atbackends.md:9. - Stale inbound references. Grepped the whole tree for
AGENTS.md: ~40 remain, including comments in live tests ("AGENTS.md's test-layer boundary", "AGENTS.md 'Settings Migration'",tests.cmake:4905"the AGENTS.md carve-out"). I checked each against the newAGENTS.md§"Where moved sections live" table — it covers every one of them, including therelease-prepskill's three section citations, which still point at sections that stayed in the core file. CODEOWNERSlast-match-wins.docs/is at line 150,docs/agents/at 166 — later, so Tier 2 wins. The added comment's reasoning is correct. Also confirmed.claude/skills/pr-review/has no explicit rule, so it falls to*.md→ infrastructure; the policy edit is gated at Tier 2, not ungated.- Did the comment sweep move a CI ratchet? Ran every static checker on both heads:
check_engine_boundary --strict(102 findings, 0 blocking),check_capability_records(71, at the frozen 71),check_command_plane --strict(413 raw commands, baseline 413 — flat, so no Flex wire text was being counted out of comments),check_test_registration --strict,check_ci_test_gate,gen_touchpoint_manifest --check,gen_seam_probe_table --check, plus qt-pin/theme-seed/network-timeouts/shader. All pass on5cf537f5. Registered-test count went up (585 → 593 by the gate checker's own count), so the retirement pass is not a registration cliff. - Attacked the re-homed tests for vacuity. They are not thin:
icom_backend_seam_test83 checks/786 lines,hl2_backend_seam_test45/380,automation_boundary_core_test36/461,icom_session_lease_test29/409. I did not re-do the body's mutation proof (no build here), so "224 checks mutation-proven" is a claim I could not verify — it's the right evidence to ask for and it's disclosed, but take it on the author's word. - CodeGuard: all 14 CG-PATH-001 refuted, none reported. Since the PR changes zero lines of code, no finding can be PR-introduced; they fired only because comment-only edits put these files in the scanned set. Two spot-checks confirm the heuristic:
QsoRecorder.cpp:7is#include "../models/SliceModel.h", andcheck_engine_boundary.py:155is a docstring containing the literal"../core/backends/flex/RadioConnection.h". Both are../in a string, not path handling.
Socket-test surface (task §2 disclosure, not a finding). This PR is net-negative on sockets and fake peers, which is the #5254 remediation direction:
- Removed:
tests/IcomFakeRadio.h,hl2_metis_client_test.cpp(fake Metis peer),hl2_backend_test.cpp,icom_backend_test.cpp,icom_session_test.cpp,hl2_link_stats{,_model}_test.cpp, and fiveautomation_*listener tests. - Added: all 11 new test files are socket-free — I grepped each for
QTcpServer|QTcpSocket|QUdpSocket|QLocalServer|QLocalSocket|QWebSocketServer|connectToHost|listen(|Fake*Radioand every one came back empty. hl2_signal_stop_testreturns behindoption(AETHER_ENABLE_HL2_SIGNAL_STOP_TEST … OFF)withSKIP_RETURN_CODE 77(tests.cmake:4873-4886) — correctly opt-in and absent from the default graph.docs/agents/tests-ci.md:66-69still names four legacy exceptions; I verified all four still exist (thumbdv_queue_testis a.cfile, which is why a first pass looked like it had vanished). The list is still accurate.
Limits, stated plainly. I am headless: no build, no ctest, no GUI. Every finding above is reasoned from the code and from checkers I actually ran — nothing was reproduced at runtime. CI on the real head 5cf537f5 was still in progress when I looked (Static checks, Sanitizer option configures, and both CodeQL jobs green; build, check-macos, check-windows in flight), so the compile evidence I have is for 3b26283 (all 7 green), which predates the test commits — the platform compile jobs on Part 2 are unconfirmed. I also could not verify signatures on the five newer commits (no gpg in this environment; the four docs commits are confirmed signed), so branch protection is the real check there.
6. Recommendation
Approve with nits for Part 1; Needs maintainer decision on Part 2 and on the policy change. Part 1 is the most rigorously-evidenced large diff I have reviewed on this repo — the comment-only claim is the kind of assertion that is usually unfalsifiable at this scale, and it survived an independent reproduction rather than an inspection. The AGENTS.md split drops no rule, the "Where moved sections live" table correctly anticipates every stale inbound reference in the tree, and the CODEOWNERS line is right.
Two things want a maintainer rather than me: the /pr-review policy edit, because a PR shouldn't author its own review exemption even when the result is defensible (and it is — this PR is clean under the old rule too); and the bundling, because Part 2's D/A test pairs deserve a diff someone will actually read. Concrete next step: split b83e4b19…5cf537f5 into a follow-up PR, keep the disclosed gaps as #5254 body checkboxes, and let Part 1 land on its own once @aethersdr/infrastructure rules on the skill edit.
Thank you for the body — the gap disclosures, the "needs maintainer review" section, the Tier-1 drift you declined to fix, and the honest flake accounting are what made it possible to review 637 files at all.
🤖 aethersdr-agent · cost: $12.8093 · model: claude-opus-5
GitHub rejected the inline anchors (GitHub API 422: Unprocessable Entity); findings below:
.claude/skills/pr-review/SKILL.md:250
Needs a maintainer ruling — this is self-referential. This hunk removes the #[0-9]{3,} grep term and the "a removal that deletes a comment naming a symptom — quote it back and ask what now prevents it" instruction, in the same PR that deletes ~24,000 comment lines across 581 files.
I want to be fair to the change, so: I reviewed this PR under the old rule (my task shipped the pre-merge skill text), and it is clean under both. A comment-stripping comparison of all 581 files shows zero code differences — no guard, early return, or confirmation was removed anywhere, so every deletion really is prose and the new rule didn't launder anything here. The head file is also internally consistent: no leftover "quoting the deleted comment" row survives in the verdict table.
The concern is ordering, not content. This edits review governance for every future PR, and no independent ruling preceded the exemption it grants this one. Per GOVERNANCE.md that is a maintainer call. Landing cf7fdf20 separately — ruled on its own merits, before the sweep that benefits from it — would cost one PR and remove the objection entirely.
AGENTS.md:168
This section says "The guard and its test are the protection; the prose is not" — and commit b83e4b19 in this same PR retires wdsp_nb_hold_invariant_test with the note that the NB-hold invariant is "now guarded only by the WdspChannel comment". That moves a protection into prose, whic
(truncated)
Contributor
|
Cisco CodeGuard — static analysis of this PR (14 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: $13.3667 · model: claude-opus-5 |
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
1 similar comment
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
Contributor
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
1 similar comment
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
ten9876
force-pushed
the
docs/review-latency
branch
from
October 1, 2026 18:27
918c677 to
95e7f83
Compare
11 tasks done
ten9876
changed the title
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
Collaborator
Author
|
Addressing the review feedback: @on8st: both of your remaining findings are fixed in AetherClaude, blocker 1 (the Blocker 2 (bundling): split. The test commits and the QSettings ratchet are now #6075, stacked on this PR. This PR is docs and comments only, plus the removal of four unreferenced Your other nits:
Scanner comments:
|
rfoust
reviewed
Oct 1, 2026
rfoust
left a comment
rfoust
left a comment
Collaborator
There was a problem hiding this comment.
PR #6072 — Review of 95e7f830
Issue: No closing issue. Reviewed against the PR's stated goal of reducing review reading cost through comment condensation and an agent-guide split.
Proposed fix: Split AGENTS.md into a core guide and five scoped documents, condense source comments, amend review/landing instructions and the Constitution, preserve guide ownership, and remove four unused Icom lease constants.
Does it solve the issue? Partially verified: the reading reduction and restricted executable diff are independently confirmed. I have not independently reproduced the time-to-merge statistics or demonstrated a resulting reduction in review latency. One Constitution classification mismatch needs correction or an explicit maintainer ruling before landing.
Scope
| Files/group | Change | Claimed? | Verdict |
|---|---|---|---|
| 581 source files | Comment condensation; four unused constants removed from IcomProtocol.h | Yes | Independently checked; only the disclosed constant removal changes C++ tokens |
| AGENTS.md, docs/agents/*, inbound documentation references | Core/scoped guide split | Yes | In scope; checked key rules and local file links |
| .claude/skills/pr-review, pr-land, release-prep | Review/landing policy and reference changes | Yes | Disclosed; removed-guard policy has a maintainer ruling |
| Constitution and canonical mirror | Gate references and gate applicability change | Yes | Governance finding below |
| .github/CODEOWNERS | docs/agents ownership exception | Yes | Correct ordering; summary-document nit below |
| tests/tests.cmake, tools/check_engine_boundary.py | Comment/reference updates | Yes | No test assertion or checker-logic change |
The test retirement/re-homing and QSettings ratchet were split into #6075 and are absent from this head. I am not claiming assertion parity or approval of that separate PR. Nothing in this diff is unexplained by the current body; no CHANGELOG entry was added.
Blockers
- P2 — Classify the gate-scope amendment as MINOR, or preserve the existing requirement. See the inline comment on
CONSTITUTION.md:4, also applying to.specify/memory/constitution.md:4. The old Development Workflow says “every PR must pass the AetherClaude validation gate”; the new text atCONSTITUTION.md:430limits this to PRs AetherClaude opens, and Principle VIII at lines 234–237 makes the same restriction. That changes the documented applicability of a verification requirement. The unchanged versioning policy at lines 496–500 calls scope narrowing MINOR and reserves PATCH for changes with no change to a principle's requirements. Version2.0.1and “no principle's requirement changes” therefore misclassify this amendment. If narrowing is intended, use a MINOR version (normally2.1.0), update both mirrors, and describe the requirement change accurately in the sync report and PR body. This is a governance-document finding, not a reproduced runtime defect; whether to adopt the narrower gate is a maintainer decision.
Integration status: GitHub currently reports CONFLICTING / DIRTY against main, now 7cde2ae4. Conflict resolution and verification of a new integration result remain necessary. I did not resolve conflicts or test an integration with that newer main.
Nits — non-blocking
- P3 — Update the ownership summaries for the new exception. See the inline comment on
.github/CODEOWNERS:166.CONTRIBUTING.md:98–99and GOVERNANCE.md's CODEOWNERS summary still describe docs/ as Tier 3 without naming docs/agents/ as Tier 2. CODEOWNERS correctly enforces the exception; add it to those summaries so contributors know who reviews these documents.
Verification
- Head:
95e7f83071e33f473379e2d59a585dd4c1cdfe3f; original reviewed base:538ba69a991597f811597626ef2b14f7fccc8c22; original merge ref:2ab924eb278666af687a6756ef4a6c2f88862e06. The original merge-ref tree exactly matched the head tree. These results do not prove integration with newer main. - Native macOS
RelWithDebInfoAetherSDR build passed in an isolated checkout using the existing pinned Qt 6.12.0 generation. ASR and optional QtKeychain were disabled. macOS icon generation initially failed in the sandbox and succeeded with normal host access; no source change was needed. ctest --no-tests=error -R '^(icom_protocol_test|hl2_auto_gain_policy_test)$' --output-on-failure: 2/2 passed. No new socket-owning test is introduced here; these selected tests are socket-free.- Eleven static checks passed. Base/head results for engine boundary, touchpoint manifest, capability records, command plane and CI test selection matched after normalizing source line numbers. Constitution mirrors are byte-identical.
- All seven displayed CI checks reached SUCCESS, including Linux build, check-windows, check-macos and Static checks. Those successful checks precede the current-main conflict; they do not settle it.
- A safe
--config pathlaunch with an isolated settings directory succeeded. Loader tracing confirmed all 11 loaded Qt frameworks came from the pinned generation. No GUI/demo session, full CTest suite, live radio, TX or screen-reader validation was performed. The posted findings concern documentation and governance.
What I tried to break
- Independently compared C++ tokens across all 581 touched source files using a lexer with literal/raw-string/digit-separator self-checks: 580 identical, with only the four disclosed Icom constants removed. Each constant had no references in the base src/ or tests/ tree. No removed tooling directives or added trailing-backslash comments were found.
- Checked ON8ST's current findings against the head: asynchronous disconnect wording is restored at
IRadioBackend.h:105–106; the board-specific LNA fault claim is removed; Hl2ControlRequest correctly excludes 0x3b from the read path. These are resolved, not repeated findings. - Sampled lifecycle and invariant comments, including sidetone producer ordering/reset, WDSP blanker hold/close, pan identity mapping and TX-status ownership. No additional actionable drift was found in that sample. This was not an exhaustive semantic audit of every condensed block; token equality alone does not establish comment accuracy.
- Checked key settings/architecture/test rules in the guide split, relative file links and CODEOWNERS last-match ordering. The critical sampled rules remained present. The maintainer's accepted removed-guard ruling is recognized; the previous bundling objection is resolved by #6075.
Recommendation
Needs maintainer decision; request changes for the Constitution classification mismatch. Correct the amendment's classification and ownership summaries, then resolve the current-main conflicts and obtain fresh integration checks. The executable-diff restriction, local application build, focused tests and exact-head CI were independently verified; this review is not an approval or merge authorization.
…stop /pr-review treating a deleted comment as a regression. Principle VIII. New code since August lands at ~30% comments, and about half of those comment lines are incident narratives (bug retellings, "used to", review attribution). Review time rises with the comment ratio of a diff even after controlling for size, era, author and subsystem. /pr-review §3 quoted any deleted symptom-naming comment back as evidence of a regression, so prose could only ever grow. The finding is now the removed guard, not the removed prose, and narrative comments are a nit. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ub-docs under docs/agents/. Principle VIII. Every agent read all 1,300 lines (~19.6k tokens) of AGENTS.md before every change, though most of it only applies to one area. The core now holds what every change needs: claim protocol, autonomy limits, style, comments, signing, version files, CI. It routes to five sub-docs read only when the diff touches their paths: backends.md (aetherd, the seam, engine-boundary ratchets, conformance checklist), flex-protocol.md, settings.md, tests-ci.md and gui.md. Rules are relocated, not dropped. Incident narratives and dated history are cut. The aetherd step-3/4 progress log moves to an "Implementation status" appendix in the RFC, and its normative rules stay in backends.md. A "Where moved sections live" table keeps older code comments that cite AGENTS.md "<section>" resolvable. Also: - Fix stale facts: the line count, ~5 min builds, the Qt 6.8.3/6.11 split (the image pins 6.12), and the required-check list (four, not three). - Point skills, tools, DEVELOPER-GUIDE, tests.cmake and the RFC/staging docs at the new locations. - CODEOWNERS: docs/agents/ takes AGENTS.md's tier (infrastructure) instead of falling to docs/ Tier 3. - GEMINI.md and copilot-instructions.md: stop calling bin/validate-diff.sh the merge gate for everyone (it gates AetherClaude's PRs from the orchestrator repo), fix the "five" must-knows count and a garbled sentence. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…iant. Principle VIII. Applies AGENTS.md §"Comments" to the 52 comment blocks of 50 or more consecutive `//` lines under src/ (37 files): each now states what is true now and why, with the incident narratives, "used to"/"an earlier revision" corrections, review attributions and issue retellings removed. Load-bearing contracts keep their substance and their names (IRadioBackend.h's THREADING AND LIFETIME CONTRACT and its rules 1-6, gateware register facts, the derivation tables behind kFullScaleDbmAtZeroGain and kMinForwardCountsForSwr, the AutomationServer.h verb catalog the MCP bridge_command tool points at). 4,083 comment lines become 1,420. Comment-only: a C++-aware stripper (strings, char literals, raw strings, digit separators) finds identical token streams in all 37 files against origin/main, and every changed diff line is a `//` line. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…iant. Principle VIII. Applies AGENTS.md §"Comments" to the remaining 1,952 blocks of ten or more consecutive `//` lines across 581 files: incident narratives, "used to", review attribution and Principle citations go; invariants, threading and lifetime contracts, wire/register facts, derivations and anything another file cites by name stay. 78 blocks remain at 10+ lines on purpose (wire tables, schemas, the AutomationServer.h verb catalog, IRadioBackend.h's numbered contract). Comments found contradicting the code were corrected to match it (e.g. the automation bridge also starts from the Radio Setup toggle; WdspChannel's NB hold schedules no flush; HL2 register 0x3b returns no read value), and comments sitting above the wrong declaration were moved. Comment-only: a C++-aware stripper (strings, char and raw literals, digit separators) shows identical token streams for every changed file. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
on8st
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 2, 2026
…VIII. main condensed MetisClient's comment blocks (aethersdr#6072); four of them sat on code this branch restructured. Resolution: the paced restart is this branch's code, unchanged; main's condensed wording is taken for the start-retry, stage-1 watchdog, stale-datagram and retry-budget comments, re-indented into the restart's steps. main's additions in the same file (the one-shot guard, the CL1 sequence, stop()'s bank drops) are kept. The comments this branch adds in src/ are brought under the comment rule. No behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
on8st
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 2, 2026
Main's comment sweep (aethersdr#6072) condensed blocks next to this branch's additions. Four files conflicted. - Hl2Backend.cpp: main now holds the receiver by pointer in finishReceiverDspBuild, so pushSquelch beside pushNoiseBlanker takes *r. Main's condensed mode-list comment is kept, without "and no squelch" for FM, which this PR makes false. Main's condensed TX-slice comment is kept, with the squelch echo above it. - Hl2Backend.h, Hl2RxDsp.cpp: main's condensed comments, this branch's members and the pending-squelch retry beside them. - WdspChannel.h: main's condensed noise-blanker comment, this branch's setSquelch declarations above it. The automatic merge had placed refuseChannelControlForTest() between main's comment for channelConfig() and that function in Hl2RxDsp.h; it now follows the function. This branch's own added comments are cut to the comment rule. The setSquelch contract stays as one table (stage and threshold per mode); the measurement narrative behind the maps is in the PR body. No behaviour changes in the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
on8st
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 2, 2026
Main's comment sweep (aethersdr#6072) condensed every block this branch had rewritten. Five files conflicted, all in comments except the one constant this PR exists to change. Resolution, per file: - Hl2AutoGainPolicy.h, Hl2GainSplit.h: main's text as is. The fold narrative this branch corrected there is gone from main, so there is nothing left to correct. - Hl2Backend.h: main's condensed comments, with the three statements that this PR makes false corrected: the 26 dB default is not a measured bound, the arming ceiling is the top of the native range (kAutoRfGainMaxBaselineDb = hl2::kLnaGainMaxDb), and the shipped +20 dB default is armable. - Hl2Backend.cpp: main's condensed comments; the restore comment no longer says arming by default would be refused, and the refusal comment no longer says the issue number stays on the qWarning (this branch removed it). - Hl2DbReference.h: main's condensed block; the fold is stated as one unit's hardware defect instead of an open question. This branch's own added comments are cut to the comment rule: the kAutoRfGainFloorMaxDb derivation (7 lines to 3) and the DEFENSIVE note on the refusal guard (4 lines to 2). No behaviour changes in the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
on8st
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 2, 2026
Main's comment sweep (aethersdr#6072) condensed the blocks this branch had edited. Two files conflicted, Hl2Backend.h (four hunks) and Hl2Backend.cpp (one), all in comments and one friend declaration. Resolution: main's condensed text, with what this PR makes true added to it. - The floor comment says the floor belongs to the law: 24 dB for "bandscope" (what a backend starts with) and "probe", 26 for "ramp" and "binary". Main's sentence about the 26 is kept for the ramp. - The law table gains "default" and the note that neither law nor floor is persisted. - m_autoGainMode keeps no initialiser; installDefaultAutoGainLaw() is the one copy of the default. - Both friend declarations are kept: main's Hl2Cl1ReferenceTestAccess and this branch's Hl2AutoGainLawTestAccess. - The bandscope.enable comment says the loop starts the gate itself. This branch's own added comments are cut to the comment rule (history of the defect removed; it is in the PR body and the commit messages): the linkUp re-ask, applyRestoredState, defaultAutoGainLaw, the mode branch, the test header and section 7, the gain-split floor check and the tests.cmake block. No behaviour changes in the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
on8st
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 2, 2026
Two files conflicted, both where main condensed a comment block this branch had edited. Hl2Backend.cpp, healthSnapshot(): main's condensed comments are taken for the not-reported block, the haveObservation/haveBlock block and the crest row. The two put() calls this branch adds (adcDcDbfs, adcDcCodes) are kept under them. Main's crest comment said a large crest can mean a large DC offset, tracked at aethersdr#5856; with the row published it now says DC level and points at adcDcDbfs. MetisProtocol.h, Ep4Stats: main's condensed scale comment is taken; its last clause (a signed pedestal row is tracked at aethersdr#5856) now names dcDbfs() and meanCodes(), which this branch adds. This branch's own comment blocks are brought under the comment rule in the same merge: the dcDbfs() and meanCodes() declarations, the two rows in healthSnapshot() and the section-12 header in hl2_ep4_bandscope_test. No code line differs from the two parents' union. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
on8st
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 2, 2026
Two conflicts, both where main condensed a comment (aethersdr#6072) beside code this branch moved. AutomationServer.cpp: the tokenizer and deny-token helpers stay moved to TxKeyingMarker.h, and main's condensed TX-guard comment is taken over this branch's isTransmitControl(). main's copy of the moved code is identical to the one moved. TxKeyingMarker.h: main's condensed marker comment, plus one clause naming transmitControlMatch(). This branch's own comments are brought under the comment rule: the guard header drops from 41 lines to 11 and no longer retells what main did before, and two citations of a Constitution principle leave the code. No behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tropo1234
added a commit
to tropo1234/AetherSDR
that referenced
this pull request
Oct 2, 2026
…Principle VIII. ## Summary The second of the two follow-ups `jensenpat` left on his aethersdr#5992 approval: > `std::clamp` returns `v` when neither `v < lo` nor `hi < v`, and every > comparison against NaN is false, so NaN passes straight through and > `static_cast<qint16>(NaN * 32767.0f)` is undefined. Guard with > `std::isfinite`. He is right, and the guard has to be separate from the clamp rather than part of it. Only reachable if the DSP emits a non-finite sample, which is why it was a follow-up and not a blocker — the radio-speaker feature ships off by default. **The first follow-up (capping catch-up at the estimated FIFO room) is NOT in this PR.** It was implemented, it passed 571/571 and nine mutation proofs, and then it failed on the radio: the ANAN-G2's speaker FIFO fell from a median of 995 locations to 117 and underflow reports went from 1 per 14 minutes to 782 per 11. The measurement found something larger than the follow-up, and it is written up as a separate comment on aethersdr#5992 rather than guessed at here. That item needs a maintainer's direction before it can land; this one does not depend on it. ## The change The interleave loop's two lines move into `AnanSliceAudio::sliceAudioToInt16()`, which carries both halves of the law where they can be unit-tested — the clamp before the scale, and the guard a clamp cannot provide. The call site in `AnanBackend.cpp` loses four lines and gains two. Written to aethersdr#6072's comment policy: five lines, stating the invariant and the constraint, with no review narrative in the code. The five comment lines dropped from the `AnanBackend.cpp` call site are the clamp rationale, which moves to the function **together with tests for it** — the shortening AGENTS.md permits, since the guard and its test are what protect the behaviour. An infinity *does* clamp correctly, so covering it is a choice rather than a consequence: it is the same class of fault and clamping it emits a full-scale click, so one predicate covers both. Silence is the right value for a sample that has no value, and it is what the mute path already sends — so a bad block degrades into something the radio's codec and the send pacer both already handle, rather than into a click or into undefined behaviour. ## Verification - **`ctest` 595/595, 0 failures** on `f10aa757`, one deliberate skip (`app_settings_safety_explicit-profile-path-isolation`). No new test target: the checks land in `anan_p2_protocol_test`, which is standalone with no Qt and no `aethercore`. - **4/4 mutation proofs caught** — the guard removed, the guard narrowed to NaN only (so the infinity case is proven, not incidental), the clamp removed, and full scale off by one. - All eleven `tools/check_*.py --strict` ratchets pass, `check_qsettings.py` included. The hardcoded-colour ratchet does not apply; nothing here touches `src/gui/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Oct 2, 2026
14 of 16 tasks
ten9876
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 3, 2026
Conflicts were comment-only: main's aethersdr#6072 comment condensation against this branch's rewrites of the same comments in Hl2Backend.{cpp,h}, Hl2TxLevelPolicy.h, MetisClient.cpp and MetisProtocol.h. Resolved by keeping main's condensed form, restated for the window-maximum input, and keeping this branch's code unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AADGw3HVn5FMMVrx2RQZ9g
ten9876
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 3, 2026
Conflicts were comment condensation from aethersdr#6072 against this branch's comment edits, the private-declaration block (applyFilterTaps beside aethersdr#5982's pushSquelchToChannel) and adjacent tests.cmake registrations (hl2_rxdsp_adaptive_taps_test beside wdsp_minphase_workspace_test). Kept both sides; the addNotch raise guard is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AADGw3HVn5FMMVrx2RQZ9g
ten9876
added a commit
to on8st/AetherSDR
that referenced
this pull request
Oct 3, 2026
Conflicts resolved: - Hl2Backend.cpp rxShiftHz(): main condensed the comment (aethersdr#6072); kept main's text and this branch's rxTunedHz(r) argument. - Hl2Backend.h: kept both friend declarations (Hl2RitXitTestAccess, and main's Hl2Cl1ReferenceTestAccess / Hl2AutoGainLawTestAccess). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AADGw3HVn5FMMVrx2RQZ9g
9 of 11 tasks
ten9876
added a commit
that referenced
this pull request
Oct 3, 2026
…2 — 43 ms sooner. Principle VIII. (#5981) Fixes #5578 (thanks to W5TSU for the report and the 64 ms arithmetic), together with #5954, which already removed the delay outside CW with minimum phase — this is the CW half. Also closes the CW half of row 4.1 of #5678. CW keeps linear phase by design, so there the 8192-tap RX bandpass still delays by (taps − 1)/2. ## What changes `Hl2RxDsp::rxFilterTapsFor()` now picks the filter length: | case | taps | |---|---| | CW | 4096 | | CW with any notch, or a passband under 100 Hz | 8192 | | every mode outside CW | 8192 (unchanged) | - The length is raised *before* WDSP sees the first notch, so the 50 Hz notch floor holds. - Switching uses `WdspChannel::setFilterTaps` (#5878). - Dropping back to 4096 needs 20 Hz of margin above the threshold, so dragging a filter edge doesn't flip the length on every step. A rebuild (sample-rate change) measures that margin from the outgoing chain's length, so the length at the same settings is the same before and after. - **No hysteresis on the notch transition, by design.** Adding the first notch or removing the last costs one filter refill each way, but each one happens at a moment the operator caused, by a discrete action, not on every step of a drag. Holding the long filter after the last notch would keep CW at the longer latency with nothing needing it. - If the raise to 8192 is refused (`WdspChannel::setFilterTaps()` losing `beginControlOperation()`), `addNotch()` refuses the notch the same way it handles a notch WDSP refuses, so a 50 Hz notch is never realised at 100 Hz on the 4096 filter. The next notch, mode or filter change retries. - `setFilterTaps()` holds `g_setupMutex` (the process-global FFTW planner lock) on the hl2-io thread for the six FIR re-plans only; the channel stop stays outside it. That is the same class of cost `setMode()` already pays for the phase change. ## Why 4096 and not the 2048 in row 4.1 Measured through a real `WdspChannel`, 2048 taps leave a signal 50 Hz outside the CW filter edge at **−33 dB**, against −109 dB at 4096 and −114 dB at 8192. That holds at every CW width, because the skirt is a fixed number of Hz. 2048 would let a CW neighbour through about 80 dB louder than today. ## Measured (offline, AGC off) - **CW onset** (on8st's #5578 method): 128.00 → **85.33 ms** (exactly 2048 samples less). - **CW return after unkey:** 128 → 86 ms. - **4096 vs 8192 for widths ≥ 100 Hz:** same centre gain and −6 dB width, ≥ 109 dB down from 50 Hz outside the edges. At the threshold width itself (100 Hz) 4096 taps lose −0.00 dB at the centre (98 Hz −6 dB width at both lengths); at 50 Hz 4096 would lose 0.40 dB, which is why narrower passbands stay at 8192. - **Live 4096 → 8192 switch under a steady tone:** no NaN and no spike (peak 1.00× steady), level unchanged, and a **155 ms refill** during which audio is more than 6 dB down. The operator hears that once when adding the first notch, removing the last, or crossing 100 Hz. - **Notch across the switch:** −103.4 dB, against −103.5 dB on a chain already at 8192. - **Outside CW:** 8192 vs 2048 differs by 0.3 ms at minimum phase, so those modes are untouched. ## Tests New `hl2_rxdsp_adaptive_taps_test` (socket-free, ~10.5 s wall clock). It includes a negative control asserting that 2048 fails the skirt criterion. The exact 2048-sample latency delta is asserted on the impulse response's centre of energy (within 2 samples); the half-amplitude onset is checked within 3 ms. Live legs cover the refused raise, a rebuild where the mode moved mid-build (both directions) and a rebuild that keeps the width hysteresis; each goes red when its line in `Hl2RxDsp.cpp` is mutated. `hl2_rxdsp_unmute_return_test`'s CW-vs-USB margin goes from 60 to 30 ms, because CW without a notch now delays (4096−1)/2 samples (42.7 ms) instead of 85 ms. The check still catches the regression it exists for: with minimum phase dropped outside CW, `usbReturn` goes to ~128 ms, the CW−USB gap goes negative, and both that check and `usbReturn < 80 ms` fail (mutation-checked). **Listening test (2026-09-30, at `bf9fe0a5`):** on the air, receive only: an HL2 on 40 m CW at 500/250/100 Hz, with this PR's head in an integration build. It was clean, with no ringing, clicks or smearing heard. This is one operator's ear, not a side-by-side comparison against `main`. **Update against main:** merged current `main` into this branch (no rebase). The conflicts were comment condensation from #6072 against this branch's comment edits, the private declarations beside #5982's `pushSquelchToChannel()`, and adjacent `tests.cmake` registrations; both sides were kept. A length change does not disturb #5982's squelch: `RXASetNC` resizes the filter cores (including fmsq's noise filter) but leaves every squelch stage's run flag and threshold alone, and CW has no squelch stage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Jeremy [KK7GWY] <kk7gwy@aethersdr.com>
11 tasks done
ten9876
added a commit
that referenced
this pull request
Oct 4, 2026
…e. Principle VIII. (#6089) ## Summary `std::clamp` returns `v` unless `v < lo` or `hi < v`, and every comparison against NaN is false, so a NaN passes straight through the clamp and the following `static_cast` to a 16-bit integer is undefined behaviour. `jensenpat` raised it on his #5992 approval for the ANAN radio-speaker quantiser; the same clamp-then-cast shape sits at every float→int16 conversion that consumes DSP, resampler or capture audio. This guards all of them, each at its own site and at its own existing scale: | Site | Path | Direction | Test | |---|---|---|---| | `AnanSliceAudio.h` `sliceAudioToInt16()` (call site `AnanBackend.cpp`) | ANAN radio-speaker wire | RX | `anan_p2_protocol_test` | | `MetisProtocol.cpp` `ep2WriteTxIq` | HL2 TX IQ on the wire | TX | `hl2_metis_protocol_test` | | `TciIoWorker.cpp` `encodeRxAudio` (int16 format) | TCI audio to external clients | RX | `tci_rx_audio_test` | | `Hl2Backend.cpp` `forwardSpeakerAudioToCodec` `toI16` | HL2 radio-speaker codec | RX | — | | `ClientPuduMonitor.cpp` playback | monitor after the resampler | RX | — | | `AudioEngine.cpp` RADE capture float→int16 | RADE mic | TX | — | | `AudioEngine.cpp` RADE resampler output | RADE mic | TX | — | | `RADEEngine.cpp` LPCNet input | RADE TX | TX | — | The guard is separate from the clamp because it has to be: a clamp cannot test a value that fails every comparison. It covers infinity too. An infinity clamps correctly, but to a full-scale rail — a click on a speaker feed, a full-scale sample on a TX feed — so one `std::isfinite` predicate makes both silence, which is also what each path already sends when muted or idle. **Finite input is unchanged.** Every edit is a guard in front of the existing expression; no scale, rail or rounding changes, and the symmetric ×32767 sites (ANAN, HL2, Metis) and the ×32768 sites (AudioEngine, RADE, TCI, monitor) keep their own convention. A 1,000,000-bit-pattern probe of the ANAN conversion found zero mismatches between old and new for every finite input. **A non-finite sample becomes silence for that sample only**; finite samples in the same block are untouched. The ANAN speaker interleave moves into `sliceAudioToInt16()` so it can be unit-tested; the five comment lines the `AnanBackend.cpp` call site loses are #6072's clamp rationale, which moves into that function with tests for it — the shortening AGENTS.md sanctions ("as long as the guard or test it describes stays"). Not in scope, and why: - **Already guarded:** `AudioEngine.cpp` TX float→int16, `CwDecoder.cpp`, `TxMicChannelNormalizer.cpp`, `QsoPcmConverter.cpp` (whole block validated first). - **Not undefined:** `IcomAudio.cpp`, `TxVoiceProcessor.cpp`, `RemoteAsrBackend.cpp` round with `lround()` and then narrow an integer, which C++20 defines. - **Own tone generator as input:** `CwSidetoneQAudioSink.cpp`, the CW-sidetone record path in `AudioEngine.cpp`, `ClientQuindarTone.cpp`, `ClientTxTestTone.cpp`. - **Not a cast:** `asr/Fbank.cpp`, `RNNoiseFilter.cpp` scale float into float. This guards the conversion, not the source. A NaN that reaches a resampler or IIR filter can latch in its state; that would now produce steady silence rather than undefined behaviour, and nothing here detects or resets it. `jensenpat`'s other #5992 follow-up — capping catch-up at the estimated FIFO room — is deliberately not here. It was implemented and then failed on an ANAN-G2 (speaker FIFO median 995 → 117 locations, underflow reports 1 per 14 min → 782 per 11); the measurement is written up on #5992 for a maintainer's direction. ## Constitution principle honored **Principle VIII — Evidence Over Assertion.** The existing comments asserted that clamping before the cast made the conversion safe. It does for overshoot and not for NaN. Each tested guard is shown red without its fix below. ## Test plan - [x] Local build passes (`cmake --build build`) - [x] Existing tests pass — 651/653 locally at `44d7cb29` (Arch, Qt 6.12, `ctest -j16`). One deliberate skip (`app_settings_safety_explicit-profile-path-isolation`); one failure, `connection_panel_size_test` ("horizontal overflow is reachable, not clipped"), deterministic on this machine with ThemeManager reporting its resources failed to load. This PR touches no file under `src/gui/`. - [x] Behaviour on a real radio — the ANAN quantiser was in the audio path for two ANAN-G2 sessions (14 min and 11 min, Saturn gateware 27) with clean audio. A NaN cannot be induced from a radio, which is why the guards are covered by arithmetic tests instead. Mutation proofs — each check below goes red with its guard removed: | mutation | caught by | |---|---| | ANAN guard narrowed to `std::isnan` | `an infinity quantises to silence…` | | ANAN clamp removed | `an overshoot clamps, not wraps` | | ANAN full scale 32767 → 32768 | `full scale is the positive peak` | | ANAN guard removed, built with `-fsanitize=float-cast-overflow` | `runtime error: nan is outside the range of representable values of type 'short int'` | | Metis EP2 guard removed | `an infinite Q sample is silence, not a full-scale rail`, `a negative infinity is silence too`; NaN under the sanitizer as above | | TCI int16 guard removed | `non-finite samples encode as int16 silence` | Without the sanitizer, a NaN check alone cannot go red: the undefined cast lands on 0 on both x86-64 (`cvttss2si` → `0x80000000`, low 16 bits zero) and arm64 (`fcvtzs` → 0). The sanitizer build is the proof for NaN; the infinity checks are the deterministic ones. The HL2 speaker, monitor and three RADE sites have no test seam short of a connected backend or a running audio engine; each is the same one-line `std::isfinite` guard as the tested sites, verified by build and by reading. The four aetherd gates (`check_engine_boundary`, `gen_touchpoint_manifest --check`, `check_capability_records`, `check_command_plane`, all `--strict`) produce identical output on the merge base and this head. No new tests target; the new checks land in existing, socket-free targets. ## Checklist - [x] Commits are signed (`docs/COMMIT-SIGNING.md`) - [x] No new flat-key `AppSettings` calls — no settings touched - [x] Code is clean-room (Principle IV) - [x] All meter UI uses `MeterSmoother` — n/a, no UI - [x] Documentation updated if user-visible behavior changed — n/a: nothing changes for finite audio - [x] Security-sensitive changes reference a GHSA if applicable — n/a, local DSP values, not untrusted input 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Jeremy [KK7GWY] <kk7gwy@aethersdr.com> Co-authored-by: Jeremy [KK7GWY] <105957118+ten9876@users.noreply.github.com>
This was referenced Oct 4, 2026
sq9fk
added a commit
to sq9fk/AetherSDR
that referenced
this pull request
Oct 7, 2026
…comments. Principle VIII. - Tag core/ShuttleRateIntegrator.h as ui-support (same class as HidEncoderManager.h) and regenerate aetherd-touchpoints.md: the new row, HidEncoderManager.h includers 2 -> 3, totals +1. - Shuttle Ring group styles its group, note and combos through ThemeManager::applyStyleSheet, so the setStyleSheet ratchet stays at +0. - Help: the first step lands after about 60 ms, not "straight away". - Comments condensed to the AGENTS.md comment policy (aethersdr#6072); the hardware measurements behind kFirstStepDelaySec live in aethersdr#5933. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jensenpat
pushed a commit
that referenced
this pull request
Oct 7, 2026
…ciple IV. ## Summary WDSP ships two impulse blankers. #5824 landed the first (ANB, `nob.c`); this adds the second (NOB, `nobII.c`), which deskHPSDR, pihpsdr and Thetis all call NB2. Instead of gating the blanked window to zero it reconstructs it from the samples either side: better when the impulse lands on a wanted signal, worse when the noise is dense enough that those samples are not clean. Every reference client offers both and replaces neither, and so does this. Host-side DSP on our own IQ throughout. No wire protocol, no radio firmware, and no behaviour change for a Flex or an Icom, whose blankers are the radio's. - **`WdspChannel` opens both stages** for the life of a receive channel and runs at most one: `processIq` feeds exactly one stage per block, chosen by the kind. Both are created even when no blanker is on, because `create_nobEXT` allocates and a lazily built stage would allocate on the real-time path the first time someone clicked NB2. - **The transmit hold covers both**, for the reason it covered one: the trigger is relative to a running average, so a transmit period of zeros would decay that average and gate the first real receive samples. - **Kind and fill ride #5919's typed route** (ruling 1). `SliceDspRequest` gains `kind` and `fill` members, carried on every `Feature::Nb` request, and one new field, `Field::Fill`. A change of blanker is an ordinary `Field::Enabled` request: `enabled` says whether one runs, `kind` says which, so a radio with one blanker (Flex, Sim) reads `enabled` exactly as before. `SliceDspRequest::requestedBlanker()` derives the three-state answer in one place, and a request built without a kind still means the first blanker. There is deliberately no `Field::Kind`: the NB button calls `setNbKind()` on every radio, so a new field would have reached Flex and Sim, which do not handle it. - **Host backends apply it through one verb**, `IRadioBackend::setSliceNoiseBlanker(id, kind, level, fill)`, which the HL2, ANAN and Icom `requestSliceDsp` call. Kind, level and fill arrive together, so no ordering between them exists. Icom treats anything but `Off` as on. Flex refuses `Field::Fill` as `Unsupported` rather than encoding it; the fill is only reachable where `hasHostNoiseBlanker` is set (HL2, ANAN), so no operator sees that refusal. - **Two enums, one conversion.** `core/backends/NoiseBlankerKind.h` is the seam's vocabulary; `WdspChannel` keeps its own, because `core/dsp` is a leaf. `core/backends/WdspNoiseBlanker.h` holds the conversion, with `static_assert`s that fail the build if an existing value is renumbered. - **`SliceModel` gains `nbKind()`/`nbFill()`, and `nbOn()` is now derived from the kind**, so there is one source of truth. `setNb(bool)` keeps its meaning: `true` selects the first blanker (ruling 2). rigctl's `NB`, SmartCat's `NB`, TCI's `rx_nb_enable`, `get slice`, the MIDI mapping, the keyboard shortcut and the band-stack entry are untouched. A radio's bool echo cannot downgrade a host NB2 to NB. - **UI: the NB button cycles Off → NB → NB2 → Off** where this host runs the blanker, and stays a plain toggle elsewhere (ruling 2). The label follows the running kind; the `objectName` does not, since the bridge addresses the button by it. An **NB2 fill combo** appears under the DSP grid while NB2 runs, and leaves with the NB button in FM (ruling 4). - **Bridge** (ruling 3): `slice dsp nb2 on <level> <fill>`, refused on a radio without `hasHostNoiseBlanker`, and `get slice` / `get hostnb` report the kind and fill beside the existing `on`. - **`aether_wdsp.h` gained the NOB declarations.** `nobII.c` was already compiled; only the curated header was missing it. ## Review and rulings | Item | Where | |---|---| | Blocker 1: #5919 removed the routing this was built on | Rebased onto `85f2a124` and moved onto `SliceDspRequest` in `340d00b8` | | Ruling 1: widen `SliceDspRequest` with kind and fill | `340d00b8` | | Ruling 2: the cycling button, and `setNb(true)` dropping NB2 to NB | Unchanged, as accepted | | Ruling 3: the bridge surface | Unchanged, as accepted | | Ruling 4: the fill row showing in FM | `3ef4cd36` | | Blocker 2: the run-flag ratchet | Removed in `3ef4cd36` | | Nits: stale line cites, the retired-test reference, the `static_assert` claim, comment length | `3ef4cd36` | **Blocker 2.** The review's mutations showed the ratchet guarded an inert property. A run flag left on the unchosen stage blanks nothing, because `processIq` never feeds that stage. The one mutation that does double-blank (both stages fed, the ANB run flag left on under NB2) passed the ratchet and failed DISCRIMINATION 2 in `hl2_noise_blanker_test`. `tools/check_nb_exclusivity.py`, its `Static checks` step and its `docs/agents/backends.md` entry are gone. The test's closing comment now names the discrimination checks as the exclusivity pin, and the run-flag comment in `WdspChannel::setNoiseBlanker` names the dispatch as the guarantee. **Ruling 4.** The fill row follows `m_nbModeOk`, and `refreshNbControls()` now runs at both places that recompute it: the `modeChanged` handler, and `syncFromSlice()`, whose earlier refresh ran before `m_nbModeOk` was set. The shared DSP level row is unchanged. ## Rebase onto `85f2a124` Main was 38 commits ahead and the branch was `dirty` in eight files. The move onto #5919 is a port, not a marker resolution: - **`SliceModel`** no longer has `noiseBlankerCommandIssued`. `setNb`, `setNbKind` and `setNbFill` publish through main's `notifyReceiveDspIntent`, and `currentDspRequest()` fills `kind` and `fill` into every Nb request. The per-field intent revisions grow from two fields to three. - **`RadioModel`**: the block of direct receive-DSP `connect()`s this branch carried is dropped; `wireSliceReceiveIntentsToBackend()` owns that wiring now. - **`WdspChannel.h`**: main's `FmReceive` and this branch's two enums landed at the same spot; both kept. - **Tests from main, ported**: `backend_receive_contract_test` (from #5919) asserted `Config::noiseBlankerEnabled`, which this branch replaces with the `noiseBlanker` kind, so it now asserts `Impulse`. `anan_noise_blanker_readback_test` keeps main's typed dispatch. New rows in the contract test pin that a Flex reads a host kind as `nb=1` and refuses the fill with no wire text, and `noise_blanker_kind_model_test` asserts on the `SliceDspRequest` the model emits. Main has since taken two Icom commits (to `12df6943`). They touch `IcomCivBackend.{h,cpp}`, which this PR also changes. The merge is clean, and neither side uses a signature the other changed. CI on `3ef4cd36` runs on the merge with `12df6943`. Earlier rebases, onto `b714cd3d` and `732b99a9`, resolved comment-prose conflicts from #6072, accepted #6075's retirement of `wdsp_nb_hold_invariant_test.cpp` (its behavioural successor `wdsp_nb_hold_test` is ported), kept #5982's squelch API, and ported two tests main had added since the earlier bases: `hl2_rxdsp_adaptive_taps_test.cpp` and `reroute_dead_controls_test.cpp`. ## Constitution principle honored Principle IV: clean-room. WDSP's NOB is in-tree. deskHPSDR was read for which knobs exist and what its defaults are (the 3-way choice, the shared threshold, the zero-fill default), and no code was copied. Principle VIII: the blanking, the fill reaching WDSP, single-stage dispatch, the model's echo handling, the request shape, the fill row's FM rule and the applied-state readback are each pinned by a test with a mutation run behind it, listed below. Squashed-from: #5966 Co-authored-by: tropo1234 <299180028+tropo1234@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
ten9876
added a commit
that referenced
this pull request
Oct 9, 2026
…I. (#6304) ## Summary Nearly every change to the codebase touches the user guide, so the docs workflow now lives directly in `AGENTS.md`, in a new **User documentation** section, instead of a four-line pointer. That was a maintainer call; the file grows past the 300-line core convention from #6072. The section covers: - **Same PR:** a user-visible change updates its docs page in that PR, never in a follow-up. - **Edit `docs/user/docs/` only:** `versioned_docs/` is the Stable snapshot that `/release-prep` replaces with `snapshot_stable.py`. - **The Docs Style Guide:** section order, status banners, Known issues citing open issues, Troubleshooting as symptom → fix, current behaviour only, exact UI labels. - **New pages** go in `sidebars.js` and use relative `./slug.md` links. - **Generated pages:** `python3 tools/docs/gen_reference.py` after touching the shortcut, MIDI, controller, log-category or TCI registries. - **Log Analyzer rules:** a reworded log message means updating its rule; `test_log_rules.mjs --update` refreshes moved lines. - **Screenshots:** re-shoot stale ones with `capture_screenshots.py --only <id>`, never transmitting and redacting IP, MAC and serial, or say so in the PR. - **Before pushing:** `npm ci && npm run build` in `docs/user`, which is the link and anchor check. The old bullet in *AI Agent Guidelines* now links to the new section. ## Testing - Every path and command named in the section exists on main (`tools/docs/*`, `docs/user/docs/docs-style-guide.md`, `sidebars.js`, `screens.json`, `rules.json`). - All Static-checks scripts pass and `git diff --check` is clean. The new lines are wrapped to the file's ~80-column style. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
jensenpat
pushed a commit
to sq9fk/AetherSDR
that referenced
this pull request
Oct 17, 2026
…comments. Principle VIII. - Tag core/ShuttleRateIntegrator.h as ui-support (same class as HidEncoderManager.h) and regenerate aetherd-touchpoints.md: the new row, HidEncoderManager.h includers 2 -> 3, totals +1. - Shuttle Ring group styles its group, note and combos through ThemeManager::applyStyleSheet, so the setStyleSheet ratchet stays at +0. - Help: the first step lands after about 60 ms, not "straight away". - Comments condensed to the AGENTS.md comment policy (aethersdr#6072); the hardware measurements behind kFirstStepDelaySec live in aethersdr#5933. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Since March, the median time to merge a PR has gone from minutes to more than a day (0.07h in March, 29h in September). About 63% of that time is the wait for a first reviewer, and about 34% is review rounds. This PR cuts the reading cost every review pays:
The test half of this work moved to #6075, stacked on this PR, at the reviewer's request.
d97a1c04f/pr-reviewno longer treats a deleted comment as a regression; the finding is the removed guard.04ae46d5fdocs/agents/(backends, settings, gui, tests-ci, flex-protocol). Rules were moved, not dropped. Stale facts fixed; inbound references updated.4ec4f1b13,ecd7bf97d//lines condensed (about 2,000 blocks, 581 files).//lines insrc/went from about 100k to 76.7k. About 40 comments that contradicted the code were corrected.0a5aab3d9.specifymirror stays byte-identical.512ef72b1/pr-reviewputs every finding in the single review body, located byfile:line. Inline comments become a rare exception, because each inline thread blocks the merge under conversation resolution./pr-landreads the review body as its primary finding list.0eeafd8e0disconnected()"before returning or asynchronously", and a single-unit LNA fault is dropped from Hl2AutoGainPolicy.5780b8797docs/agents/Tier-2 exception, matching CODEOWNERS (@rfoust).b50395d2cIOfflineHealthSource's forward declaration gets its own comment back, andHl2DspSetupPolicy.hsays its bounds are for one receive chain, not anumRx>1connect (which also opens the TXA chain). Comment-only.1eca2e66dkToken*constants fromIcomProtocol.h; the live lease timing isIcomSession::Params. This is the only non-comment code change, and it changes no behaviour.Needs maintainer review
.github/CODEOWNERS: addsdocs/agents/ @aethersdr/infrastructure, so the sub-docs keep AGENTS.md's tier.0a5aab3d9) and the CONTRIBUTING/GOVERNANCE tier summaries (5780b8797) are maintainer-owned.ci.ymland design-doc mentions of retired tests.Test plan
04ae46d5f..ecd7bf97d(after rebasing onto current main) it reports 581 files and 0 non-comment changes. AetherClaude's review reproduced the result independently.0eeafd8e0andb50395d2care also comment-only.AetherialAudioStrip.cpp, is resolved by keeping main's new code with the condensed comment. Full build and full ctest on this head (5780b8797): 574 of 575 pass. The only failure,connection_panel_size_test, fails identically on a cleanmainbuild on the same machine and passes in CI. All 20icom_*tests pass after the constant removal.static-checks.ymlpasses, plusgen_bridge_docs.py --check. The colour ratchet is +0/+0/+0. The 41 a11y warnings on touched files are identical on base and head, so none are new.Current version:from the slimmed AGENTS.md.Checklist
CHANGELOG.mdentry🤖 Generated with Claude Code