Repository navigation
fix(logging): log the failures the docs Log Analyzer could not see. Principle VIII. - #6308
Conversation
…rinciple VIII. Three user-facing failures never reached the log, so neither the docs Log Analyzer nor a person reading a support bundle could spot them (#6285): - Flex VITA-49 sequence errors. PanadapterStream counted them per stream but logged nothing. It now writes one aether.vita49 warning per stream at most every 10 s ("PanadapterStream: VITA-49 sequence errors on <cat> stream 0x<id>: N in the last S s, E of P packets since the stream started"). The limiter runs only on the error path, and the line is logged after the stats lock is released. - BNR runtime pack failures. NvidiaAfxPack had no log statements. fail() now logs "NvidiaAfxPack: install failed: <reason>" on aether.nvafx (a cancel logs at Info), the three exits that bypassed fail() go through it, and each network failure adds "transfer failed" or "transfer timed out (stall limit 30 s)". URL query strings are stripped from logged text, since a redirected download URL can carry a signed query. - The Qt platform plugin. main.cpp now logs the plugin in use, the QT_QPA_PLATFORM value and who set it, and a warning when Qt fell back from the first plugin in that list. Qt's own message about a plugin that failed to load is printed before the log handler exists, so it stays on stderr. The Hermes-Lite 2 discovery item is #6293 and is not touched here. Docs: five Log Analyzer rules with samples (vita-sequence-errors, bnr-download-failed, bnr-checksum-mismatch, bnr-install-failed, qt-platform-fallback), a "Qt platform plugin" summary row, and Troubleshooting entries on the Low-Bandwidth, BNR and GPU Rendering pages. Tests: vita_sequence_loss_log_test (limiter, text, and PanadapterStream fed meter packets without a socket), qt_platform_choice_test and nvidia_afx_pack_log_test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for this — it is a well-built change, and the drift-checked rule/sample/doc triple per log line is exactly the shape #6285 asked for. I ran node tools/docs/test_log_rules.mjs against the head checkout: 706 checks passed for 45 rules, so every new rule compiles, cites a live source literal, fires on its sample, keeps clean.log silent and resolves its docs anchor. One finding below, in the VITA-49 path.
1. Issue fit
Partially — and the gap is disclosed. #6285 lists four items:
| # | Item | This PR |
|---|---|---|
| 1 | Flex VITA-49 sequence errors | ✅ aether.vita49 warning, rate-limited per stream (issue suggested aether.protocol; lcVita49 is the better fit and is already QtWarningMsg in LogManager.cpp:22) |
| 2 | BNR runtime pack failures | ✅ fail() logs, the three exits that bypassed it now route through it, network failures get their own line |
| 3 | HL2 discovery | ❌ deferred to PR #6293 (open, by @on8st, covers item 3 only) — stated in the commit message and non-overlapping, so this is a coordinated split, not an unaddressed requirement |
| 4 | Qt platform plugin / fallback | ✅ plugin + QT_QPA_PLATFORM + who set it, plus a fallback warning |
The issue's own acceptance bar ("each new line should be a stable, greppable string, so the analyzer rule can cite its file:line") is met and mechanically enforced.
Test layer is right: all three new tests are socket-free CTests, which is the correct layer for wire/parsing/policy per AGENTS.md. I verified the claim — PanadapterStream's constructor (PanadapterStream.cpp:88-95) only does m_pcmProducer.start() and explicitly defers socket setup to init(), so feeding processDatagram() directly binds nothing.
Socket-test register: this PR adds no socket-owning test and removes none. vita_sequence_loss_log_test names QUdpSocket only indirectly via PanadapterStream.h; nothing in the three new tests binds, listens or connects.
2. Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/flex/VitaSequenceLossReport.h, PanadapterStream.{cpp,h} |
Rate limiter + line text + the emit site | Yes (item 1) | In scope |
src/core/NvidiaAfxPack.{cpp,h} |
fail() logging, logTransferFailure, withoutUrlQueries, 3 bypass exits routed through fail() |
Yes (item 2) | In scope |
src/core/NvidiaAfxFilter.h |
adds Q_DECLARE_LOGGING_CATEGORY(lcNvAfx) |
Not in the body | In scope — necessary: lcNvAfx is defined in NvidiaAfxFilter.cpp:12 and the pack TU needs the declaration. Not a new category |
src/QtPlatformChoice.{h,cpp}, src/main.cpp, CMakeLists.txt:1691 |
Item 4 + the one target that compiles main.cpp |
Yes | In scope. src/main.cpp is referenced by exactly one target, so no sibling target needs the new TU |
docs/user/docs/*.md, log-samples/**, rules.json, LogAnalyzer/index.js |
5 rules, 5 samples, 1 summary row, 3 Troubleshooting entries | Yes | In scope |
rules.json line bumps (221→234, 175→188, 1686→1714, 80→83) |
Cited line numbers | Not in the body | In scope — these are this PR's own insertions moving existing cited lines; the drift checker's --update output |
clean.log +1 line |
the new Platform: line |
Not in the body | In scope — required, or check 6's s.qtPlatform === 'xcb' assertion has no data |
tests/* + tests.cmake |
3 new targets | Yes | In scope |
Nothing in the diff is unexplained. No new public or protocol surface: no settings key, no capability field, no wire verb, no CLI flag — QtPlatformChoice is an internal struct and the log strings are the only new contract. No CHANGELOG.md entry was added (correct). No existing default, UI element or behaviour is modified, so there is no preference-vs-fix question here: this is purely additive diagnostics.
3. Blockers
1. src/core/backends/flex/PanadapterStream.cpp:465-488 — m_streamStats is not cleared at the documented disconnect reset hook, so a reused stream id makes the new warning fire falsely on reconnect.
clearRegisteredStreams() is self-described as the place where everything keyed by stream id gets dropped:
// Drop PLC state alongside the rest of the per-stream tables — this is
// the disconnect-time reset hook, so anything keyed by VITA-49 stream
// id can be cleared safely (#2738).
m_audioPlc.clear();It clears 15 per-stream maps plus resetAudioStreamStats(). It does not clear m_streamStats, and neither do unregisterPanStream() (:449, which clears m_frames/m_dbmRanges/m_pendingDbmRanges) nor unregisterWfStream() (:458). This PR puts new state into that very map:
struct StreamStats {
int lastSeq{-1};
int errorCount{0};
int totalCount{0};
VitaSequenceLossLimiter lossLog; // PanadapterStream.h:309
};Precondition: a stream id recurs after a teardown (disconnect/reconnect to the same radio, or a pan/DAX stream recreated with the same id). Then, in processDatagram():
stats.lastSeqstill holds the previous session's last sequence value, so the first packet of the new stream almost always failsvitaSeq == (lastSeq + 1) & 0x0F→ a spurioussequenceError.stats.lossLog.m_lastReportMsis more thankIntervalMsin the past, sorecordError()returnstrue→ the line is emitted immediately, at warning level, on the first packet after reconnect.stats.totalCount == 1is never true again, solossLog.start()never re-runs andreportWindowMs()is measured from the previous session's last report — "in the last N s" can be an arbitrarily large number.errorCount/totalCountcarry over, so "EofPpackets since the stream started" really means "since this process first saw this stream id".
Before this PR that was an invisible counter artifact. Now it surfaces as vita-sequence-errors → "Packets from the radio were lost or arrived out of order" (rules.json:260) pointing the user at a Troubleshooting entry that tells them to re-cable their network. That is a misdiagnosis in a feature whose whole purpose is accurate diagnosis.
I could not confirm how often FlexRadio reuses stream ids without hardware, so the frequency is uncertain — but the fix is correct regardless and is what the #2738 comment already asks for. Add a stats-side forget and call it from the three teardown paths (keep it outside the m_streamMutex scope so you don't introduce a m_streamMutex → m_statsMutex nesting that processDatagram() doesn't already have — worth a second look at lock order before you place the calls):
void PanadapterStream::forgetStreamStats(quint32 streamId)
{
QMutexLocker statsLock(&m_statsMutex);
m_streamStats.remove(streamId);
}and in clearRegisteredStreams(), alongside resetAudioStreamStats();:
{
QMutexLocker statsLock(&m_statsMutex);
m_streamStats.clear();
}A unit test fits the existing socket-free seam: feed a gap, call clearRegisteredStreams(), feed seq 0 on the same id, assert no second warning.
4. Nits (non-blocking)
src/core/NvidiaAfxPack.cpp:709— the redaction covers the log but not the signal. Credit to security-audit (CodeGuard) for the lead; I confirmed it.esis the rawerrorString()from:701.fail()logs it stripped (:381,withoutUrlQueries(msg)), but also doesemit finished(false, msg)unstripped, and the only consumer renders it verbatim:src/gui/AetherDspWidget.cpp:1850→m_bnrAfxStatus->setText(QStringLiteral("Failed: %1").arg(msg)). Qt's transfer error strings commonly embed the post-redirect URL, which is the signed queryNvidiaAfxPack.h:80-82exists to defend against. Low severity — a label the operator already sees, every current source is a public GitHub-release/pypi.org asset, and the signature is minutes-lived — but the fix also removes the current double-log of the same cause (:708plus:381):fail(QStringLiteral("download failed (%1): %2").arg(label, withoutUrlQueries(es)));
src/core/backends/flex/VitaSequenceLossReport.h:57-58— "%5of%6packets" misreads as a loss count.totalErrorsisstats.errorCount, which counts sequence gap events, not packets lost — a single 12-packet gap increments it by 1. The header comment two lines up is accurate about this; the line text is not. Suggest"%5 errors in %6 packets since the stream started".src/core/backends/flex/PanadapterStream.cpp:743— the comment is slightly ahead of the code.QString sequenceLossLine; // logged after the stats lock is releasedis true of theqCWarningat:798, butformatVitaSequenceLoss()is called at:770inside theQMutexLockerscope, so the string formatting still happens underm_statsMutex. Error path only, so the cost is negligible — just worth not claiming otherwise.src/core/NvidiaAfxPack.cpp:334— the stripper eats a trailing delimiter.[?#][^\s]*is greedy to the next whitespace, so(https://h/x?y)becomes(https://h/x. Cosmetic only; the redaction itself is sound.docs/user/src/components/LogAnalyzer/rules.json— 4 of 45 rules now point at/next/(the three new-heading ones plusvita-sequence-errors), against 41 on stable. That is forced and correct today, but nothing flips them whenversion-stableis next snapshotted, and a user on a released build following them lands on Next docs. Worth a note wherever the versioning procedure lives.docs/user/log-samples/rules/vita-sequence-errors.log:9— "1 in the last 376 s" at09:20:41against a stream-start line at09:14:05is ~396 s. Cosmetic; the checker doesn't look at it.- None of the three new tests are in any CI
-Rfilter. The only filteredcteststeps inci.ymlare the DV,cross_needle_meter_test,mac_nr_filter_testandasr_gpu_probe_testlanes, so PR CI compilesvita_sequence_loss_log_test,qt_platform_choice_testandnvidia_afx_pack_log_testbut does not run them — they gate only on the nightly full suite. That matches house practice; noting it so "all green" isn't read as "the new tests passed".
5. What I tried to break (and could not)
Findings below are reasoned from the head checkout at /tmp/aetherclaude/pr-6308; I have no build and no runtime here, so nothing was reproduced by execution except the Node drift checker, which I did run.
- "The regex escapes are missing." The diff as rendered shows
(\bhttps?://...)and\1— but that is a harness artifact (the same one that turned every&into${PR_DIFF}).NvidiaAfxPack.cpp:334-336in the checkout really reads"(\\bhttps?://[^\\s?#]+)[?#][^\\s]*"and"\\1", so the pattern compiles as\b/\sand the replacement is a genuine capture back-reference. No leak. Same for\\Sinrules.json:835, whichnew RegExp()would have thrown on. - "A user cancel will be logged as a 30 s stall."
logTransferFailuretreatsOperationCanceledErroras a timeout on the stated grounds that "cancel() disconnects before it aborts". Verified:cancel()callsm_reply->disconnect()beforeabort()(with a comment explaining the close-mid-download crash that motivated it), so the slot never runs on a user cancel. The comment is accurate. - "
fail()on the three new exits will delete a completed component and break the resume the docs now promise."fail()doesQFile::remove(m_tmpFile). Butm_tmpFileis cleared on the success path (:728,QFile::remove(dest); m_tmpFile.clear();) and on the checksum path, so by the timeassembleAndCommit()can callfail("install verification failed")it is empty. The new BNR doc text ("Components that finished downloading are kept, so the download resumes") holds. - "The platform line will mislabel who set
QT_QPA_PLATFORM." The onlyqputenv("QT_QPA_PLATFORM", ...)inmain.cppis at:307, which also setsg_qpaPlatformChoiceon the next line, andg_qpaPlatformUserSetis assigned at:288unconditionally (not inside any platform#ifdef), before anything can modify the variable. The three-wayUser/AetherSDR/Unsetdiscrimination is exact on every OS. - "
QGuiApplication::platformName()is read before there is an application."QApplication app(argc, argv)is atmain.cpp:384; the logging block is at:638-648, insideif (logManager.startLogging(...))at:621. Ordering is fine. - "
fellBack()will false-positive." Walkedwayland;xcb→wayland-egl(prefix match, no warning),xcb:foo=1,";xcb"(empty first →false), a single-entry list (Qt aborts rather than falling back), and case differences (startsWith(..., Qt::CaseInsensitive)). No false warning found. - "The test can't reach
processDatagram/PCC_METER."PanadapterStreamis innamespace AetherSDR(PanadapterStream.h:24) and the test declaresVitaSequenceLossLogTestAccessin the same namespace, so the newfriendat:220genuinely grants access toprocessDatagram(:225) and the privatePCC_METER(:256).VITA49_HEADER_BYTES,StreamCategoryandcategoryStats()are public. - "The new BNR test won't compile without
HAVE_NVIDIA_AFX."CMakeLists.txt:3241sets it PUBLIC onaethercore, which the test links, so the guard inNvidiaAfxPack.hopens. Andci.ymldoes build the AFX arm (-DENABLE_NVIDIA_AFX=ONin thebuildjob at:269), so the green badge really does cover that code — unlike some AFX changes. - Rate limiter arithmetic. Hand-checked the 60-second steady-error case: reports at t=0/10000/…/60000 = 7 lines accounting for all 12001 errors, matching the test's assertions. The
m_windowStartMs < 0fallback and the!m_seqLossClock.isValid()guard both hold;m_seqLossClockis only ever touched underm_statsMutex, andprocessDatagramruns on the single network thread (PanadapterStream.cpp:94), so there is no race. - Tests that would pass against the unfixed code.
vita_sequence_loss_log_testasserts exact line text and that a 50-gap burst adds no second line whilecategoryStats().errors > 40— emptying the limiter body or dropping theqCWarningfails it. Likewisenvidia_afx_pack_log_testasserts the exactaether.nvafxwarning string for a path (archive not found) that previously emitted no log at all. - Other tools' findings. All 36 CodeGuard hits are false positives on this diff and I refuted them individually:
CS-VLN-SSRF-PRIV-10/CS-PII-IPon10.9.0.34is the pinned TensorRT version, not an RFC1918 address; everyCS-VLN-CMDI-PIPEis|as regex alternation inrules.json(and the Flex wire delimiterR\d+\|0\|);CS-VLN-CMDI-BACKTICKhits backticks inside a regex character class and C++ comments, with nosystem/QProcessanywhere in the change;CS-VLN-SSRF-HEXhits VITA-49 stream ids and header bit masks;CS-PII-DL-1is the Flex error codeF3000001in prose;CS-VLN-PATH-UNCis JSX display text showing users where logs live; and everyCS-VLN-XSS-EVENTis a React JSX prop (onDrop,onChange,onClick) — the file has noinnerHTML/dangerouslySetInnerHTML, and all log-derived text goes through JSX interpolation, which React escapes. The only lead that survived is the nit above. security-audit also flaggedaddr=/from=host addresses reaching default-level logs atPanadapterStream.cpp:322and:596versusLogRedactionPolicy.h:53-56, and the "AetherSDR already redacts addresses…" claim atLogAnalyzer/index.js:135; I checked the hunks and none of those lines are in this diff — pre-existing, and worth its own issue rather than a change here. - Could not check: whether FlexRadio actually reuses VITA-49 stream ids across a reconnect (needs hardware or a firmware capture) — that is the one open question in Blocker 1, and it governs frequency, not whether the fix is right. Also could not verify the rendered docs anchors in a built site beyond what the checker's slugifier asserts.
6. Recommendation
Request changes, on Blocker 1 only. Everything else here is solid and the nits are all optional. m_streamStats escaping the clearRegisteredStreams() reset is a small fix in a file this PR already touches, and it matters more than usual because the state is now user-visible: without it, the first packet after a reconnect on a recurring stream id can tell an operator their network is dropping packets when nothing was lost. Next step: add the stats-side forget to the three teardown paths (checking lock order), plus the one reconnect assertion in vita_sequence_loss_log_test. The %5 of %6 packets wording is worth folding into the same push since it changes a cited literal and the drift checker will want the rule text updated with it.
🤖 aethersdr-agent · cost: $12.1264 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (36 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: $12.2877 · model: claude-opus-5 |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit: Partially, with one correctness defect to fix. At 18a772b75, items 1, 2 and 4 of #6285 gain the promised log lines and analyzer rules. Item 3 is explicitly delegated to #6293 and #6285 correctly stays open. VITA warnings and BNR failure logging work; the Qt fallback diagnosis mistakes an explicit command-line override for a failed plugin.
Scope
| Files/group | Claimed? | Verdict |
|---|---|---|
PanadapterStream.{cpp,h}, VitaSequenceLossReport.h |
Yes: per-stream sequence-error warnings | In scope; routing and sequence-counter semantics retained |
NvidiaAfxPack.{cpp,h}, NvidiaAfxFilter.h |
Yes: failures, timeout detail, URL-query stripping | In scope; three bypass exits now use existing cleanup |
QtPlatformChoice.{cpp,h}, main.cpp, CMakeLists.txt |
Yes: actual platform and fallback log | In scope; defect below |
Log Analyzer index.js, rules.json |
Yes: five rules and platform summary | In scope |
| Three troubleshooting pages, six log samples | Yes | In scope |
Three tests, tests.cmake, test_log_rules.mjs |
Yes: coverage and sample/summary checks | In scope; socket-free and registered |
All 26 files and the single signed commit are explained by the stated scope. No unrelated changes, CHANGELOG entry, new dependencies, radio commands, settings writes, or engine-boundary growth found.
Blockers
-
[P2] Treat Qt's command-line platform override as the effective request before diagnosing a fallback —
src/main.cpp:643,src/QtPlatformChoice.cpp:35. Qt accepts-platformand gives it precedence overQT_QPA_PLATFORM. This code compares the plugin actually loaded with the environment value alone, then asserts the environment's first plugin failed to load. Launching the exact PR build withQT_QPA_PLATFORM=offscreen AetherSDR -platform minimalproduced:INF default: Platform: Qt platform plugin "minimal" (QT_QPA_PLATFORM=offscreen, set by the user) WRN default: Platform: Qt fell back from "offscreen" to "minimal"; the first plugin in QT_QPA_PLATFORM did not loadOffscreen did not fail: the control launch without
-platform minimalsuccessfully loaded it. The analyzer nevertheless emitsqt-platform-fallbackand tells the user a plugin could not load. This also affects an ordinary Wayland user launching with-platform xcb, a useful diagnostic escape hatch. Capture the effective-platformrequest beforeQApplicationconsumes its arguments, distinguish that source in the startup summary, and compare against that request. Add coverage for an intentional override alongside a genuine fallback; keep the genuine warning working.Bridge evidence from that same instance:
get radioreturned{"ok":true,"radio":{"model":"AetherSDR Demo","serial":"DEMO-0001","connected":true,"transmitting":false}}(abridged). I also reproduced the false line with a small Qt 6.12QGuiApplicationharness using this PR's helper.
Nits — non-blocking
src/core/NvidiaAfxPack.cpp:378: narrow the body's claim that a user cancel always logs Info. The line is infail("cancelled"), whereascancel()at line 348 disconnects an active reply before aborting it and emits no log itself; cancellation during a transfer therefore bypasses this new line. The asynchronous unpack callbacks do reach it. This is a documentation precision issue, not a request to log every destructor/idle cancel.src/core/NvidiaAfxPack.cpp:73: the stall-timeout and checksum/unpack failures remain app-level coverage gaps, correctly disclosed by the author. I independently verified the no-network download failure and missing-archive failure, but did not reproduce a 30-second stall, corrupt download, native Windows/macOS execution, or real-radio packet loss.
What I tried to break
- Built the exact head with Qt 6.12 on Linux, shared core, ASR/RADE/D-STAR/DFNR disabled. All three newly registered CTests passed. Preflight inspected their sources: meter packets are injected directly without
init()/bind, BNR imports only a nonexistent local archive, and the platform test is pure QString logic. No new socket test was built or run. - Mutation checks: forcing
fellBack()false fails four checks; downgrading the VITA warning to Info fails three integration checks; reverting the missing-archive exit fails its warning check. Restored the source after each mutation. Burst limiting, held-error accounting, default category visibility, signed-query/fragment stripping, and unchanged offline-failure signal text survived the positive checks. - Drove three private-network-namespace app sessions through explicit, unique bridge sockets, fresh settings and
AETHER_AUTOMATION_NO_TX=1. Each connected only toDEMO-0001: normal offscreen launch logged no fallback,nosuchplugin;offscreenlogged a genuine fallback, and the command-line override reproduced blocker 1. All review instances were stopped. - A fourth demo session opened AetherRX, selected BNR, clicked Download and accepted the license in the throwaway profile.
log tailreturned bothW aether.nvafx: NvidiaAfxPack: transfer failed: AFX runtime: Network unreachableandW aether.nvafx: NvidiaAfxPack: install failed: download failed (AFX runtime): Network unreachable. - Analyzer validation passed 706 checks / 45 rules. Test registration and engine-boundary checks passed (existing boundary warnings only). Required CI checks pass; current main
888ac58bfwas committed2026-10-09T00:35:52Z, before this head's pull-request CI run creation at02:09:16Z, so the checks are not stale relative to main. GitHub verifies the commit signature. A separate model-invocable automated-review skill was unavailable; this was a manual adversarial pass.
Recommendation: Request changes. Fix the reproduced false diagnosis, then rerun the override and genuine-fallback cases. The VITA and BNR changes otherwise survived the focused tests and demo checks. Formal approval remains the maintainer's decision.
GitHub rejected the formal REQUEST_CHANGES event because the authenticated reviewer account is also this PR's author. This single COMMENT review carries the request-changes recommendation instead.
…og false failures. Principle VIII. Review follow-up for #6285 (items 1, 2 and 4). VITA-49: clearRegisteredStreams() and unregister{Pan,Wf,Dax,Iq}Stream() restart the stream's sequence tracking, so a reconnect or a re-created stream that reuses an id no longer measures its first packet against the previous instance's last count and logs a sequence error. The cumulative per-stream totals behind the status bar and Network Diagnostics are kept. The line's counts restart with the instance and now read "E since the stream started (P packets received)": E counts gaps, not lost packets. Its values are read under m_statsMutex and formatted after it is released. The 10 s rate limit holds across a restart, so a stream torn down and re-created over and over logs no more often. Qt platform: Qt gives -platform (or --platform) precedence over QT_QPA_PLATFORM. main() now reads it before QApplication consumes it, logs it as the request ("-platform <value>, on the command line") and compares the plugin in use with it, so an intentional override is not reported as a failed plugin. BNR: fail() strips URL queries from the message it emits as well as the one it logs, so the panel's "Failed:" text is redacted like the log; the stripper keeps closing punctuation after a query. Docs: the vita-sequence-errors sample uses the new wording and consistent windows; qt-platform-fallback's explanation names -platform; the summary cites both platform line forms. docs/user/README.md and the /release-prep snapshot step say to move /next/ rule links to Stable once a snapshot carries their pages. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Review follow-up, pushed as @ten9876's reviewBlocker 1:
Nit: cancel claim. The PR body now says this: a cancel that reaches Nit: stall, checksum and unpack coverage gaps. Acknowledged. They are still listed under Not verified in the body. @aethersdr-agent's reviewBlocker 1: Nit: unredacted Nit: " Nit: comment ahead of the code ( Nit: the stripper eats a trailing delimiter. Fixed ( Nit: Nit: sample window. Fixed. The sample now says Nit: the new tests are not in a CI Pre-existing CodeGuard (36 findings). All false positives, as the bot's review already showed one by one: version strings, regex Also checked
|
There was a problem hiding this comment.
1. Issue fit
Yes, for the scope the PR claims. #6285 lists four gaps; this PR takes 1 (VITA-49 sequence errors), 2 (BNR pack failures) and 4 (Qt platform plugin), and the body and the issue thread both carve item 3 out to #6293 — so the partial coverage is declared, not quiet. I checked the three specific rulings from the triage thread and all three are honoured:
- "use
qCWarningon the semantically right category" —aether.vita49isQ_LOGGING_CATEGORY(lcVita49, "aether.vita49", QtWarningMsg)atsrc/core/LogManager.cpp:22and is registered at:77.applyFilterRules()emits noaether.*.info=false, so a Warning reaches a default bundle with noLogManagerchange. ✔ - "
lcNvAfxhas no header declaration, which is the only thing blockingNvidiaAfxPack.cpp" — now declared insrc/core/NvidiaAfxFilter.h:13, inside the sameHAVE_NVIDIA_AFXguard as theQ_LOGGING_CATEGORY. ✔ - "three exits bypass
fail()and emitfinished(false, …)directly" — all three are rerouted:NvidiaAfxPack.cpp:479(no GPU),:855(install verification failed),:879(archive not found). ✔
The one place the PR usefully disagrees with the triage is item 4. Triage ruled the plugin-load failure "structurally unloggable" because QApplication is built at main.cpp:377 and the handler installed at :625. That is true of Qt's own diagnostic, but the PR notices that the fallback is still inferable after the fact by comparing the request to QGuiApplication::platformName(), and logs that. I verified the ordering: platformArgument() is read at main.cpp:293, before QApplication; the log block sits at :642-658, inside if (logManager.startLogging(...)) after qInstallMessageHandler. That is a real advance on the triage's framing, not a workaround.
2. Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/flex/VitaSequenceLossReport.h, PanadapterStream.{h,cpp} |
New rate limiter + log line; restartStreamSequence() on all five teardown paths |
Yes | In scope |
↳ side effect: a reused stream id no longer bumps stats.errorCount / m_catStats[cat].errors |
Changes the status-bar and Network Diagnostics counters, not just the log | Yes — body: "except that a reused id no longer adds a false error on its first packet" | In scope, disclosed |
src/core/NvidiaAfxPack.{h,cpp}, NvidiaAfxFilter.h |
Log lines, withoutUrlQueries(), three exits rerouted through fail() |
Yes | In scope |
src/QtPlatformChoice.{h,cpp}, src/main.cpp, CMakeLists.txt |
New app-shell helper + the start-up platform lines | Yes | In scope |
docs/user/src/components/LogAnalyzer/{rules.json,index.js} |
5 new rules, 1 summary field, 4 refreshed line citations | Yes | In scope |
docs/user/docs/*.md, docs/user/log-samples/** |
3 new troubleshooting sections + 5 samples | Yes | In scope |
.claude/skills/release-prep/SKILL.md, docs/user/README.md |
The /next/ → Stable link obligation at snapshot time |
Yes | In scope — this PR is the first to use /next/ links (git show 888ac58bf:…/rules.json | grep -c '/next/' → 0; head → 4), so documenting the obligation belongs here |
Nothing in the diff is unexplained. CHANGELOG.md correctly untouched. withoutUrlQueries() is new public surface on NvidiaAfxPack but it is a pure static helper, not protocol or config — no maintainer ruling needed.
Socket tests: none added. vita_sequence_loss_log_test constructs a bare PanadapterStream and calls processDatagram() directly via a friend test-access class — init() is never called, nothing binds. Recorded for the §2 obligation, not a finding.
CI: build, check-windows and check-macos were still in_progress at head d87678c when I looked; Static checks, Build docs site, Sanitizer option configures and the three CodeQL lanes are green. I am not reporting this as green. None of the three new tests appear in any ctest -R filter in .github/workflows/ci.yml, so they are compile-gated only on this PR; nvidia_afx_pack_log_test is additionally inside the ENABLE_NVIDIA_AFX AND ((UNIX AND NOT APPLE) OR WIN32) AND x86_64 block, so it never builds on macOS. That matches house practice (full-suite is its own lane), so it is an observation, not an ask.
3. Blockers
None.
4. Nits (none blocking)
-
docs/user/log-samples/rules/vita-sequence-errors.log:11— this line cannot be produced by the code:... VITA-49 sequence errors on audio stream 0x04000008: 7 in the last 407 s, ...formatVitaSequenceLoss()formats the id withQString::number(streamId, 16)(VitaSequenceLossReport.h:66), which is unpadded —0x04000008would really print as0x4000008. The analyzer rule matches on the fixed prefix, so nothing fails, but the sample is advertised as real output. Either fix the sample, or pad — padding also matches the 8-digit form Flex's ownstreamlistings use:.arg(category, QString::number(streamId, 16).rightJustified(8, QLatin1Char('0')))
(If you pad,
tests/vita_sequence_loss_log_test.cpp:131and:152/:181anddocs/user/log-samples/rules/vita-sequence-errors.log:9-10need the same treatment.) -
src/core/backends/flex/VitaSequenceLossReport.h:13—kIntervalMsrate-limits per stream, with no global cap. A Flex session routinely carries 10–14 tracked streams (2 pan + 2 waterfall + meter + up to 4 DAX + per-slice audio), so a genuinely lossy link can sustain ~1 warning/second indefinitely. That is the shape that has previously flushed the real cause out of the 100-line bundle tail (thePerfStallframeAgeprecedent). Worth considering a global floor, or one aggregated line listing the affected streams, rather than N independent limiters. -
src/core/NvidiaAfxFilter.h:13/src/core/LogManager.cpp:77— the body is right thataether.nvafxneeds no registry row for visibility (only Debug is wildcard-filtered). But it is still absent fromm_categories, so it is the one new line a user cannot see or toggle in the Support dialog's category list, unlikeaether.vita49. The triage comment called the row "still worth doing"; adding it is a one-line change if you agree. -
src/QtPlatformChoice.cpp:14-22—fellBack()is a one-directional prefix test. It correctly treatswayland→wayland-eglas not a fallback, but the reverse (requestwayland-egl, Qt reportswayland) would be logged as a fallback that did not happen. Nobody writesQT_QPA_PLATFORM=wayland-eglin practice, so this is cosmetic; astartsWithin either direction would close it. -
docs/user/src/components/LogAnalyzer/rules.json:766-773—bnr-download-failedpoints at the Stable anchor/bnr-gpu-noise-removal#the-download-fails, but itsexplanationcites the 30 s stall limit and resume-on-retry, which this PR adds to the Next copy of that section only. A user on the released docs gets a correct link to a page that does not yet say what the rule just told them. Inherent to the snapshot model and self-correcting at the next release — noting it only because it is the one rule whose text and target are out of step.
5. What I tried to break
All of the below is reasoned from reading the code at /tmp/aetherclaude/pr-6308. I did not build or run anything in this repo (the one exception: I ran node tools/docs/test_log_rules.mjs, which passed — 707 checks, 45 rules, confirming the body's figure and that all four /next/ anchors and the Stable one resolve).
- The redaction regex, read from the raw file rather than the diff. The diff I was handed is sed-mangled (
&→ a placeholder), so the literal looked like it had single backslashes — which in C++ would make\ba backspace byte and\1a 0x01, i.e. a redactor that silently does nothing.sed -n '332,340p' src/core/NvidiaAfxPack.cppshows"(\\bhttps?://[^\\s?#]+)[?#]\\S*?([)\\]}>\"',.;:]*)(?=\\s|$)"with"\\1\\2"— correctly escaped. I then hand-walked the lazy-match backtracking for?a=(1)&sig=abc(the bracket-inside-query case) and(…?sig=abc). Retry(punctuation-keeping) and both land where the tests assert. The security-audit subagent independently confirmed this and thatQString::replaceexpands\1(same idiom already atAsyncLogWriter.cpp:310). - Whether a signed URL can still reach a log or the panel. All three
QNetworkReply::errorString()sinks funnel throughlogTransferFailure()(strips at:82) orfail()(strips at:383, before both theqCWarningat:387and theemit finished(false, shown)the panel renders). The file has exactly four logging statements, none of which prints a URL. My one remaining idea — userinfo credentials (https://user:tok@host/) survivewithoutUrlQueries()— is covered anyway byurlAuthorityReinAsyncLogWriter.cpp:314, which already redacts userinfo and host. Refuted. - Lock inversion from the new nesting.
clearRegisteredStreams()holdsm_streamMutexand now callsrestartAllStreamSequences(), which takesm_statsMutex— a nested acquisition. I checked everym_statsMutexholder (packetErrorCount/packetTotalCount/categoryStats/audioStreamDiagnostics/recordAudioStreamPacketLocked/theprocessDatagramblock) and none takesm_streamMutex. The order is also pre-established:resetAudioStreamStats()already did exactly this nesting at the same call site before the PR. No deadlock. The fourunregister*functions were deliberately restructured to releasem_streamMutexbefore callingrestartStreamSequence(), which is the right shape. - The three rerouted
fail()exits deleting state they shouldn't.fail()runsQFile::remove(m_tmpFile), so rerouting the early-return exits through it could, in principle, destroy a partial download a user meant to resume. It can't:cancel()clearsm_tmpFile(:370), it is only ever set at:661inside an in-flight download, and all three new call sites are early returns before any transfer starts. Harmless. - Whether
fail()'s new log line fires a wrong analyzer rule.install()'s "no supported NVIDIA GPU found" andinstallFromFile()'s "archive not found" now emitinstall failed:lines. Neither matchesbnr-download-failednorbnr-install-failed(both regexes enumerate their suffixes), so no false docs link. Thecancelledsentinel compare at:384matches the three literalfail(QStringLiteral("cancelled"))sites. platformArgument()on hostile argv.-alone (readsarg[1]only after confirmingarg[0]=='-', so in bounds),--(becomes-, no match),-platformas the final argument (i+1 < argcguard),-platformtheme(exactstrcmp, not a prefix), and two-platformarguments (last wins, matching the documented Qt behaviour). The test covers all six.- Whether
nvidia_afx_pack_log_testcan even compile.NvidiaAfxPack.his entirely inside#ifdef HAVE_NVIDIA_AFX, and the new target doesn't set the define itself — but it linksaethercore, which declares itPUBLICatCMakeLists.txt:3241under a condition character-for-character identical to thetests.cmakeguard. Fine.src/QtPlatformChoice.cppis also pulled into the opt-inspectrum_confirmed_geometry_test(it copiesAetherSDR SOURCES); self-contained, so harmless. - Attacking the tests. These would not pass against the unfixed code: the reconnect block asserts
packetErrorCount() == 0and the exact warning text afterclearRegisteredStreams()with a reused id, and eachunregister*has its own assertion — removerestartStreamSequence()from any one of them and that check fails. The limiter block pins the held-count arithmetic (reportErrors() == 1001,lines == 7over a minute) rather than asserting the implementation back at itself.logLines()is checked against full literal strings, so a wording drift breaks the test and the analyzer rule together. The body's mutation-check list matches what I'd expect these assertions to catch. - Doc claims against code.
kTransferTimeoutMs = 30000(:39) backs "stalled for 30 seconds";unzip(:795) andtar --zstd(:808) are really what unpacks the pack; the menu label is"Network Diagnostics..."(MainWindow_Menus.cpp:376), matching the docs text exactly;AsyncLogWriter::redactPiireally does collapseQDir::homePath()to~(:198-206), backing thecould not install into <path>claim. - CodeGuard's 39 findings. All refuted, none folded in.
CS-PII-IP/CS-VLN-SSRF-PRIV-10onNvidiaAfxPack.cpp:49/:272are the pinned TensorRT/CUDA version strings (10.9.0.34,12.8.90) read as RFC-1918 addresses;CS-VLN-SSRF-HEXandCS-PII-DL-1are SHA-256 integrity pins and afile/line/textcitation block;CS-VLN-CMDI-PIPE/-BACKTICKare|alternations and backticks inside rule regexes and C++ comments, never shell input;CS-VLN-XSS-EVENTare ReactonChange/onDropprops (the component has noinnerHTML,dangerouslySetInnerHTML,evalornew Functionanywhere, andindex.jsgained exactly one line in this PR);CS-INJ-role_overrideis ARIArole="note"/"status"/"alert"plus the comment "an explicit override is not a fallback". Credit to security-audit (CodeGuard) for independently walking the leak paths and the regex. - What I could not check. Item 1 against real lossy hardware (the PR says so itself under "Not verified"), the stall-timeout branch, Windows/macOS behaviour, and whether Qt 6.12's
-platformparser really takes the last occurrence — I verified only that the helper matches the comment describing it. CI's three compile lanes had not finished.
6. Recommendation
Approve with nits. This is unusually well-made work: the body is a verifiable artifact rather than a summary, every triage ruling is honoured with the reason recorded, the one place it departs from the triage (item 4's fallback line) is a genuine improvement on that ruling, the user-visible counter side effect is disclosed rather than buried, and the tests fail for the right reasons. I went after the redaction regex, the lock nesting, the rerouted failure exits and the test strength specifically, and all four held.
Concrete next step: fix the 0x04000008 sample (or pad the hex), and say whether you want the per-stream limiter to grow a global cap now or in a follow-up — neither needs to hold the merge. Thank you for the mutation-check list and the honest "Not verified" section; they made this much faster to review.
🤖 aethersdr-agent · cost: $15.8778 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (39 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.0395 · model: claude-opus-5 |
…estarts, and Stable snapshots move rule links. Principle VIII. Maintainer follow-ups on #6285's review. -platform: a -platform (or --platform) argument now counts like a user-set QT_QPA_PLATFORM. AetherSDR no longer imposes wayland;xcb over it, and GpuSelector's GLX/EGL choice reads the request Qt will actually use (GpuSelector::requestPicksWayland, tested). remote_audio_rx: it has no registration in PanadapterStream, so RadioModel restarts its sequence tracking when the stream is created, adopted or removed by us. A removal the radio reports already restarts it through handleDaxRxStreamRegistry's unregisterDaxStream(). New socket-free remote_audio_rx_sequence_restart_test. Redaction: the default-level `addr=` and `from=` VITA lines are already masked by the IPv4/IPv6 shape rules; three negative-corpus rows in async_log_writer_test pin that. The Log Analyzer's privacy note now says exactly what the redactor masks. snapshot_stable.py rewrites each Log Analyzer rule link /next/<page> to /<page> when the new Stable snapshot has the page; the README and the /release-prep step drop the manual step. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
The maintainer asked for the four open follow-ups to be folded into this PR. They are in 1.
Proof:
2. Host addresses in default-level logs.
3.
Proof:
4. Proof:
Also run on
|
…ter aether.nvafx. Principle VIII. Review of d87678c and the automated pass on 43ed8bf. RadioModel restarts the VITA-49 sequence tracking of every stream the radio reports removed, in one place (handleDaxRxStreamRegistry), instead of special-casing remote_audio_rx on create and adoption: a restart while a stream is still flowing could hide a real gap. The test now also proves a gap across an adoption status still counts. The sequence-error line pads the stream id to 8 hex digits, as the radio lists it, so the log sample is real output. fellBack() matches the platform prefix both ways (wayland-egl reports itself as wayland). aether.nvafx gets a Support-dialog registry row (generated log category page refreshed). The analyzer's privacy note lists every class the redactor masks; the ORDER note in main.cpp names the -platform input. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Answers to @aethersdr-agent's review of Bot review of
|
There was a problem hiding this comment.
Issue fit
Solves #6285 for the three items it claims. The issue lists four gaps; the PR takes 1 (Flex VITA-49 sequence errors), 2 (BNR pack failures) and 4 (Qt platform plugin), and explicitly leaves 3 (HL2 discovery) to #6293 — which matches @ten9876's own comment on the issue and @on8st's claim of that item. Each new string is greppable, cited from rules.json, and node tools/docs/test_log_rules.mjs passes locally (707 checks / 45 rules), which is the sync net the issue asked for.
The PR also follows the triage rulings rather than the issue text: qCWarning(lcVita49) instead of the issue's aether.protocol suggestion (the second triage comment's revised recommendation — and correct, since lcVita49 is declared QtWarningMsg at src/core/LogManager.cpp:22 so a warning passes applyFilterRules() with no defaultOn change), and all three fail()-bypassing exits the triage enumerated are rerouted (NvidiaAfxPack.cpp:479, 855, 879).
One place it goes beyond the triage, correctly: the triage concluded item 4's plugin-load failure was "structurally unloggable" because QApplication is constructed ~240 lines before the handler. The PR instead infers the fallback after the fact by comparing the request to QGuiApplication::platformName(), and is honest in the docs and the rule explanation that Qt's own reason stays on stderr. That is a better answer than the issue got.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/flex/{PanadapterStream.*,VitaSequenceLossReport.h} |
Rate-limited VITA-49 sequence-error line + per-instance restart | Yes (item 1) | In scope |
src/core/{NvidiaAfxPack.*,NvidiaAfxFilter.h} |
fail() logging, transfer-failure lines, withoutUrlQueries(), Q_DECLARE_LOGGING_CATEGORY |
Yes (item 2) | In scope |
src/{QtPlatformChoice.*,main.cpp}, CMakeLists.txt |
Platform-plugin + fallback lines | Yes (item 4) | In scope |
src/core/LogManager.cpp, docs/user/docs/generated/log-categories.md |
aether.nvafx registry row |
Yes | In scope. New user-visible surface (a Support-dialog toggle), but the triage explicitly said it was "still worth doing"; python3 tools/docs/gen_reference.py --check confirms the hand-edited generated page matches the generator exactly |
src/core/GpuSelector.{h,cpp} |
willUseWayland() now reads -platform first; requestPicksWayland() extracted to the header |
Partly | Not pure logging — see nit 1. Related, undisclosed behaviour fix |
src/models/RadioModel.cpp |
restartStreamSequence() on every removed stream id |
Yes (commit 43ed8bf) | In scope; covers remote_audio_rx, which has no registration in PanadapterStream |
tools/docs/snapshot_stable.py, .claude/skills/release-prep/SKILL.md, docs/user/README.md |
Release-time rewrite of /next/ rule links to Stable |
Not in #6285; only in commit 43ed8bf's message | Related tooling, not a blocker — see nit 2 |
docs/user/docs/*.md, log-samples/**, LogAnalyzer/{index.js,rules.json}, test_log_rules.mjs |
Rules, samples, Troubleshooting entries, summary row | Yes | In scope |
tests/async_log_writer_test.cpp |
Three negative-corpus rows for pre-existing PanadapterStream address lines |
No (comment says "#6285 review") | Tiny, defensible hardening; not worth unbundling |
tests/{vita_sequence_loss_log,remote_audio_rx_sequence_restart,qt_platform_choice,nvidia_afx_pack_log}_test.cpp, tests.cmake |
Four new tests | Yes | In scope |
No CHANGELOG.md entry — correct. No settings keys, no capability fields, no protocol verbs, no UI defaults touched, so Constitution I/II/III/V/VI and the AGENTS.md persistence/credential rules are not engaged. No preference change: every behavioural delta points at a named issue item or a triage ruling.
Socket-test disclosure (per the reporting rule): this PR adds four tests and none of them open, bind, listen on or connect a socket. vita_sequence_loss_log_test and remote_audio_rx_sequence_restart_test feed hand-built meter packets straight to PanadapterStream::processDatagram() on a stream that never binds; remote_audio_rx_sequence_restart_test uses an IRadioBackend stub with no transport (state-machine injection, not a synthetic firmware peer); qt_platform_choice_test is pure; nvidia_afx_pack_log_test imports a missing archive and never calls install(). I grepped all four for QTcpServer/QUdpSocket/bind/listen/connectToHost/Fake* — the only hits are the word "socket-free" in the header comments. Layer placement matches AGENTS.md ("wire encoding, parsing… a socket-free CTest"). None of the four is in any ctest -R filter in ci.yml, so they compile in the build lane but are exercised only by the full suite — a coverage caveat, not a defect.
Blockers
None.
Nits (all non-blocking)
1. src/core/GpuSelector.cpp:136-145 — the -platform fix is a real behaviour change the body doesn't state.
const QByteArray request = platformArgument.isEmpty()
? qgetenv("QT_QPA_PLATFORM")
: platformArgument.toLocal8Bit();Before this PR, a Linux user in a Wayland session who passed -platform xcb got willUseWayland() == true (we read our own wayland;xcb env, which main.cpp set because it had no way to see -platform), so __GLX_VENDOR_LIBRARY_NAME was never applied and PRIME/GLX vendor selection was silently skipped on an XWayland session. After it, main.cpp:296 skips the env override when -platform is present and GpuSelector reads the argument, so that user now gets the GLX hint. That's a correct fix, but it is NVIDIA multi-GPU behaviour, not logging. Worth one line in the body so it isn't discovered later as an unexplained delta.
2. tools/docs/snapshot_stable.py:92-130 — the /next/→Stable rewriter is new release-tool behaviour; please state it in the PR body. It's a genuine consequence of the new rules linking /next/ pages (and README.md + SKILL.md document it), but today it appears only in commit 43ed8bf's message. Two small observations while I was in there, neither needing a change: page_slugs() uses docs_dir.iterdir(), so a rule linking a nested page (/next/foo/bar) is left as /next/ — fail-safe, which is the right direction; and since rules.json is one shared component, after a release the Log Analyzer served under /next/ also links to Stable pages. Both are reasonable trade-offs, just undocumented ones.
3. docs/user/src/components/LogAnalyzer/index.js:134-138 — the privacy paragraph overstates three of its classes.
AetherSDR already masks personal details in its logs: IPv4 and MAC addresses (all but
the last part), IPv6 addresses, host names it connects to, radio serial numbers (all
but the last group), passwords, tokens and keys, names, email addresses, locations and
grid squares, and your home directory.The shape-based rules (IPv4/IPv6/MAC/serial/home dir/email) hold unconditionally. But names, locations and grid squares are redacted only as keyword-labelled field values — src/core/LogRedactionPolicy.h:36-42 matches first_name|last_name|full_name|user_name|owner, lat|lon, grid|locator|maidenhead, and docs/log-redaction.md opens by warning that "the guarantee is narrower than 'logs are safe'" for exactly this reason. A bare surname or grid square in prose survives. docs/log-redaction.md also lists callsign as deliberately kept, which this paragraph's framing invites a user to assume otherwise. Suggested wording:
AetherSDR already masks personal details in its logs: IPv4 and MAC addresses (all but
the last part), IPv6 addresses, host names it connects to, radio serial numbers (all
but the last group), passwords, tokens and keys, your home directory, email addresses,
and names, locations and grid squares where they are labelled as such. Your callsign is
kept on purpose, so a report can be identified.4. src/core/NvidiaAfxPack.cpp:715 — a bare hostname escapes both strippers. download failed (%1): %2 can carry a Qt error string shaped Host <name> not found, where the name has no scheme:// prefix and so is touched by neither withoutUrlQueries() nor AsyncLogWriter's URL-authority rule, and sits after no kHostContextKeywords keyword. In practice the reachable names are fixed public infrastructure (github.com, pypi.org, files.pythonhosted.org, pypi.nvidia.com); the only user-specific one is an HTTP proxy host. Strictly better than main, which showed the same message in the panel with no stripping at all — noted for completeness, not asking for a change. (Credit: security-audit (CodeGuard), which flagged this as a residual and declined to raise it as a finding.)
5. Log-volume shape of the VITA line. VitaSequenceLossReport.h:13 rate-limits to one line per stream per 10 s. A multi-slice Flex with DAX can have 6–8 tracked stream ids, so a sustained-loss session can emit ~2,900 lines/hour. That can push the actual cause out of a support bundle's tail (the same failure mode as the frameAge PerfStall floods). A per-stream backoff after the first few reports would keep it diagnosable without crowding; entirely your call.
6. Cosmetic: clean.log:3 reads (QT_QPA_PLATFORM=, unset, Qt default) — the empty value plus the comma is awkward. (QT_QPA_PLATFORM unset, Qt default) would read better on the platform most users are on. Also, the commit message describes the line as "E of P packets since the stream started" while the format is "E since the stream started (P packets received)".
7. src/core/GpuSelector.h:3-4 adds #include <QList>; I couldn't find a QList use the header needs.
What I tried to break
- Lock ordering on the new
m_statsMutexacquisitions.clearRegisteredStreams()(PanadapterStream.cpp:492) calls the newrestartAllStreamSequences()while still holdingm_streamMutex, whereas all fourunregister*()sites deliberately release it first — I expected an ABBA. It isn't one:resetAudioStreamStats()(:1410-1412) already takesm_statsMutexfrom inside that same block, sostreamMutex → statsMutexis the pre-existing order, and I walked them_statsMutexscope inprocessDatagram()(:781-836, includingrecordAudioStreamPacketLocked) to confirm nothing acquiresm_streamMutexunder it. The reverse order does not exist. - The
withoutUrlQueries()regex. The diff I was handed showed single-backslash escapes; the real file atNvidiaAfxPack.cpp:337has"(\\bhttps?://[^\\s?#]+)[?#]\\S*?([)\\]}>\"',.;:]*)(?=\\s|$)"and the replacement"\\1\\2", i.e. correct regex escapes and correct Qt backreferences. I hand-traced five under-strip attempts — query followed by non-whitespace (...?sig=1>rest), two URLs on one line, a query containing brackets, a query at end-of-string, a quote-delimited URL — and the lazy\S*?plus the closing-punctuation class plus the(?=\s|$)anchor strips in all of them; the failure direction is over-stripping, not leaking.tests/nvidia_afx_pack_log_test.cpp:76-91pins the S3-signature case. Independently confirmed by security-audit (CodeGuard). OperationCanceledErrormislabelled as a stall timeout.logTransferFailure()maps it to "transfer timed out (stall limit 30 s)", so a user-initiated cancel could log a bogus timeout. It can't:cancel()(:360) callsm_reply->disconnect()beforeabort(), so the finished lambda never runs; and the write-failure abort is handled by them_dlWriteFailedbranch at:712, ahead of the error branch. The comment at:67-70says exactly this and is accurate.kTransferTimeoutMsreally is 30000 (:39), matching the docs text.- The three
emit finished→fail()reroutes (:479no GPU,:855verification,:879archive not found).fail()closesm_dlFile, removesm_tmpFileand clearsm_busy— at all three sites those are already in exactly that state, so the only new effect is the log line. Theshown == "cancelled"Info branch is live (three callers at:709/735/906). -platformargv parsing vs Qt. Dangling-platformas the last token (ignored, falls back to env — same as Qt),--platform(leading-dropped, matches),-platform xcb:nograb(firstRequested()strips at:), repeated-platform(last wins, matchesQGuiApplicationPrivate),-platform ""(treated as "no argument requested", sofellBack()returns false rather than crying fallback). I could not construct an argv where our read and Qt's diverge in a way that produces a wrong log line. The one form I could not verify from here is whether Qt also accepts-platform=xcb; if it does, that spelling would be missed and we'd log the env value as the request.fellBack()false positives.wayland→wayland-eglandwayland-egl→waylandboth pass via the bidirectional prefix match;xcb;wayland→xcbis not a fallback (the old substring bug theGpuSelectorcomment warns about);windows:darkmode=2→windowspasses because of the:strip.source == Unsetcorrectly skips the check entirely, so macOS/Windows default launches never emit a spurious fallback warning.- Stream-id reuse, the thing commits d87678c/e23a407 are about.
restartStreamSequence()resets onlylastSeq/started, notm_lastReportMs, so a stream torn down and recreated in a loop cannot be used to defeat the 10 s rate limit;start()drops errors held from the previous instance; a missingm_streamStatsentry is a no-op, which is right because a fresh entry already starts atlastSeq = -1. The restart is on removal only, so it cannot hide a gap in a stream that is still flowing — theRadioModel.cpp:10691comment makes that claim and the call site matches it. - Dead additions / build scope.
src/QtPlatformChoice.cppis referenced byCMakeLists.txt:1691andtests.cmake:2894;main.cppis in exactly one target.Q_DECLARE_LOGGING_CATEGORY(lcNvAfx)is at global scope inNvidiaAfxFilter.h:13, matching the globalQ_LOGGING_CATEGORYatNvidiaAfxFilter.cpp:12(no namespace mismatch → no link error), both inside theHAVE_NVIDIA_AFXguard.nvidia_afx_pack_log_testsets noHAVE_NVIDIA_AFXof its own, which looked like a compile break givenNvidiaAfxPack.h:3's guard — butCMakeLists.txt:3241defines itPUBLIConaethercore, which the test links. - CodeGuard leads, refuted. The ~30
CS-VLN-CMDI-AMP/BACKTICK/PIPEcriticals land on&&,|and backticks inside C++ comments and onLogManager.cpp's category table — the same harness mangling that rendered the regex above with single backslashes.CS-VLN-XSS-EVENTatindex.js:173-203are React handlers on lines this diff does not touch, rendering JSX text nodes with nodangerouslySetInnerHTML.CS-PII-*andCS-VLN-SSRF-*are the sample logs,screens.jsonfixtures and redaction test vectors — fixture data by design.CS-SEC-BEARER/BASIC-AUTH/PASSWORDland onLogRedactionPolicy.h's keyword tables, i.e. the scrubber, not a secret.CS-VLN-PATH-UNCinrules.jsonis a Windows path in a rule's sample text. I confirmed none of these against the change. The security-audit subagent returned no findings after checking seven CodeGuard rule families. - Docs/tooling claims I could actually execute:
node tools/docs/test_log_rules.mjs→OK: 707 checks passed for 45 rules(this validates every citedfile:line+ text, every rule's docs anchor, and every sample log, including the five new ones and the newqtPlatformsummary row);python3 tools/docs/gen_reference.py --check→ up to date, so the hand-editedgenerated/log-categories.mdmatches the generator.
Not verified, and why: everything above is reasoned from the code at /tmp/aetherclaude/pr-6308, not reproduced at runtime — I have no build and cannot drive the GUI, so I have not seen a single one of these log lines emitted. At review time CI is incomplete: Static checks, Build docs site, Sanitizer option configures and the three CodeQL analyses are green, but build, check-macos and check-windows were all still in_progress on e23a407. Nothing here says the compile is clean on any platform. I also could not check Qt's -platform= spelling (see above), and the ~2,900-lines/hour volume estimate in nit 5 is arithmetic on the 10 s interval, not a measurement.
Recommendation
Approve with nits. This is careful work: the central choke points the triage identified are all used, the three fail() bypasses are all closed, the two follow-up commits fix exactly the two false-positive classes (reused stream ids, -platform override) that I would otherwise have raised, and the sanitizer is both correct and tested. The new tests are socket-free, correctly layered, and would fail against the unfixed code. Nothing here blocks a merge.
Next step: fold nits 1 and 2 into the PR body (the GpuSelector -platform behaviour change and the snapshot_stable.py link rewriter are the two deltas a reader of the body today would not expect), and take or leave nit 3's privacy wording — that one is the only user-facing accuracy issue. Then wait for build/check-macos/check-windows to land before merging. Thanks for the thorough commit messages; tracing the restart semantics against RadioModel.cpp:10691 was quick because of them.
🤖 aethersdr-agent · cost: $15.3843 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (193 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: $15.5504 · model: claude-opus-5 |
… note is exact. Principle VIII. Review of e23a407. The platform line for an unset QT_QPA_PLATFORM is "(QT_QPA_PLATFORM unset, Qt default)" instead of "(QT_QPA_PLATFORM=, unset, Qt default)"; the analyzer summary cites the shared format. The Log Analyzer's privacy note says names, locations and grid squares are masked where they are labelled, and that the callsign is kept on purpose. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
These are the answers to @aethersdr-agent's re-review of
Re-run on
|
There was a problem hiding this comment.
Issue fit
Partially, by design — and the partial part is correctly disclosed. #6285 has four items; this PR takes 1 (VITA-49 sequence errors), 2 (BNR pack) and 4 (Qt platform), uses Refs rather than Fixes, and names #6293 as the owner of item 3. I confirmed item 3 is still unlogged at this head (src/core/backends/hl2/Hl2Discovery.cpp — zero qC*/q* calls across 315 lines), so leaving #6285 open is right.
It also follows the triage thread's rulings rather than the issue body's original suggestions, which is the correct reading:
- Triage comment 2 revised item 1 from
lcProtocoltoqCWarning(lcVita49)("packet loss is a warning, so the level is honest rather than a workaround"). The PR uses exactly that;LogManager.cpp:22confirmslcVita49isQtWarningMsg, andapplyFilterRules()suppresses only.debug, so the line reaches a default bundle with no registry change. ✓ - Item 2's ruling was: add
Q_DECLARE_LOGGING_CATEGORY(lcNvAfx)to a header,qCWarninginfail(), plus the three exits that bypassfail(). All three named bypasses are rerouted —NvidiaAfxPack.cpp:479(no GPU),:855(install verification failed),:879(archive not found). ✓ - Item 4's ruling was "scope to logging the plugin actually selected; a plugin that fails to load entirely aborts inside the
QApplicationconstructor and is structurally unloggable". The PR does the former and says the latter indocs/user/docs/gpu-rendering.md:124("Qt prints why the plugin did not load there, not in AetherSDR's log"). ✓
Coverage: four new registered socket-free CTest targets, and the mutation table in the body is the kind of evidence that makes "the test would fail unfixed" checkable. I read tests/vita_sequence_loss_log_test.cpp line by line — the reconnect/unregister* blocks assert counts that the pre-PR code cannot produce, so they are not implementation-echo tests. No coverage request.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/flex/{PanadapterStream.cpp,.h}, VitaSequenceLossReport.h |
item 1: limiter, log line, per-instance restart | yes | in scope |
src/core/{NvidiaAfxPack.cpp,.h,NvidiaAfxFilter.h,LogManager.cpp} |
item 2: lcNvAfx header decl, fail() logging, URL stripping, registry row |
yes | in scope |
src/QtPlatformChoice.{h,cpp}, src/main.cpp, CMakeLists.txt:1691 |
item 4: platform line + fallback line | yes | in scope |
src/core/GpuSelector.{cpp,h} |
-platform now feeds willUseWayland(); requestPicksWayland() moved inline to the header so the test needs no GpuSelector.cpp |
yes, "Behaviour changes" §1 | in scope — and a real fix, not a refactor: pre-PR willUseWayland() read QT_QPA_PLATFORM even when a -platform argument made Qt ignore it, so -platform xcb in a Wayland session dropped the GLX/PRIME hint |
src/models/RadioModel.cpp:10691-10696 |
restart sequence tracking for every removed stream id (covers remote_audio_rx) |
yes, "Behaviour changes" §2 | in scope |
docs/user/docs/*, log-samples/*, LogAnalyzer/{index.js,rules.json} |
5 rules + samples + 3 troubleshooting sections + summary row + privacy note | yes | in scope |
tests/async_log_writer_test.cpp (+10 lines) |
3 negative-corpus rows pinning IPv4/IPv6 masking on the addr=/from= VITA lines |
yes, "Redaction" § | in scope — substantiates the rewritten privacy note rather than asserting it |
tools/docs/snapshot_stable.py (+50), docs/user/README.md, .claude/skills/release-prep/SKILL.md, test_log_rules.mjs (+1) |
/next/<page> rule links become Stable links at snapshot time |
yes, "Behaviour changes" §3 | separable, disclosed — see note below |
No undisclosed files, no deleted guards (I read the - lines: the only removals are the inlined willUseWayland() body, now requestPicksWayland(), and the three emit finished(false, …) the triage asked to reroute), no new protocol verb or settings key, no CHANGELOG.md entry, no UI default changed. The release-prep/snapshot_stable.py row is the one thing #6285 does not ask for; it is a consequence of the new rules pointing at /next/ (without it, readers of a release never reach the fixes), and the PR body states it plus its scratch-tree test. Not a blocker, and I would not ask for an unbundle at this size — flagging it so the maintainer can rule.
Socket tests (disclosure, not a finding): the PR adds four test targets and none opens, binds, listens or connects. vita_sequence_loss_log_test and remote_audio_rx_sequence_restart_test construct PanadapterStream with no init() and feed processDatagram() directly; nvidia_afx_pack_log_test imports a missing archive and never calls install(); qt_platform_choice_test is pure. All four are registered in tests/tests.cmake (2877, 2885, 2892, and 2868 inside the ENABLE_NVIDIA_AFX guard) — no # not registered: markers, nothing dead.
Blockers
1. src/core/backends/flex/PanadapterStream.cpp:836-840 — the new warning is rate-limited per stream id, with no global cap, on a socket that does not filter its sender. Credit: security-audit (CodeGuard), codeguard-0-logging; I confirmed each precondition against the head checkout.
if (reportSequenceLoss) {
qCWarning(lcVita49).noquote()
<< formatVitaSequenceLoss(streamCategoryName(cat), streamId, lossErrors,
lossWindowMs, lossStreamErrors, lossStreamPackets);
}The limiter lives in m_streamStats[streamId] (:785), and m_lastReportMs starts at -1 (VitaSequenceLossReport.h:48), so the first sequence error on every new stream id logs immediately. The 10 s interval only ever throttles an id that has already reported.
Three facts combine:
streamIdis read straight from the datagram (:660) and nothing inreadyRead()/processDatagram()comparesdg.senderAddress()against the radio — the onlysenderAddress()uses are the two diagnostic lines at:625and:632.- Categorisation does not require registration. Your own test proves this:
vita_sequence_loss_log_testfeedsPCC_METERpackets on never-registered id0x7000000Aand gets the warning. So an id nobody registered still lands inCatMeter/CatFFTand still owns a limiter. lcVita49isQtWarningMsgandapplyFilterRules()disables only.debug, so these lines are on by default and go through the rotating log and into the support bundle.
Failure scenario (reasoned from code, not reproduced — I cannot run the app here): any host that can reach the bound VITA UDP port sends pairs of packets with a fabricated streamId and a non-consecutive 4-bit count. Two packets per id ⇒ one unthrottled qCWarning per id, at whatever rate it can send. That is the frameAge-flood shape: the flood is the aftermath, and it pushes the real cause out of the bundle tail. It also grows m_streamStats without bound (one StreamStats, now including a 40-byte limiter, per fabricated id) and the static QSet<quint32> seenIds at :667 alongside it.
The unfiltered socket and the unbounded m_streamStats/seenIds are pre-existing; what this PR adds is the default-on, per-id-unthrottled amplification on top of them. The minimal fix inside this PR's seam is a global bound beside the per-stream one — a shared token bucket for the whole qCWarning site, and/or a cap on how many distinct ids may own a limiter, the way m_orphanStreams already uses kMaxOrphanStreams. A maintainer may reasonably rule that the right fix is the source-address filter instead and downgrade this to a follow-up; I am listing it as a blocker because the merged result is remotely floodable by default and the cap is small.
Nits (non-blocking)
src/core/LogManager.cpp:138— theaether.nvafxregistry row is unconditional, butlcNvAfxis declared and defined only underHAVE_NVIDIA_AFX(NvidiaAfxFilter.h:13,NvidiaAfxFilter.cpp:12). On macOS and in-DENABLE_NVIDIA_AFX=OFFbuilds the Support → Diagnostics list gains a toggle for a category that can never emit. Harmless and consistent with other rows; worth a word in the description if you care.docs/user/src/components/LogAnalyzer/index.js:135-139— two precision gaps in the otherwise-accurate new privacy note, both from security-audit: opaque tokens keep a 4-character prefix (kOpaqueValueFields), and host masking is keyword-anchored, so a bare hostname in prose is not masked. "passwords, tokens and keys" reads more absolute than the implementation; consider qualifying it the way the names/locations clause already is.src/core/backends/flex/PanadapterStream.cpp:471-479—restartStreamSequence()runs on the main thread while the UDP thread may still be draining in-flight packets from the old instance. Those re-primelastSeqafter the restart, so the genuinely-new instance's first packet can count one sequence error — the inverse of the false positive you fixed. Rare on a LAN and a clear net improvement; just noting it so "no false failures" is not read as absolute.src/core/NvidiaAfxPack.cpp:337— the regex is case-sensitive (https?), so an uppercase scheme in a Qt error string would keep its query. No Qt error string I can find produces one; cosmetic.src/core/GpuSelector.h:4—#include <QList>appears unused (QVectoris what the file uses).
What I tried to break
- Lock ordering / deadlock.
restartStreamSequence()takesm_statsMutex; I checked every caller.unregister{Pan,Wf,Dax,Iq}Stream()were all restructured to releasem_streamMutexfirst (:448-469,:1628,:1877-1886) — good.clearRegisteredStreams()does nestm_streamMutex→m_statsMutexat:515, but that nesting already exists at:511viaresetAudioStreamStats(), so no new order is introduced. For a deadlock you would need the reverse; I read them_statsMutexblock inprocessDatagram()(:782-836) and it acquires nothing else, and:1484/1493/1505/1511are stats-only. No finding. - The limiter arithmetic.
startTotalCount = stats.totalCount - 1after the increment, solossStreamPacketscounts this instance including the current packet — your test's "5 packets received" is right. Traced the steady-error case by hand: 7 lines per 60 s, each carrying the held count, no error silently dropped except those still pending when the stream goes clean (documented, and the live counters keep them).start()deliberately not resettingm_lastReportMsis what stops a teardown/recreate loop from logging every 10 ms; the restart block of the test pins it. platformArgument()vs Qt's real loop. Checked the one-leading-dash drop,-platformtaking the next argument, last-occurrence-wins, the trailing--platformguard (i + 1 < argc && argv[i + 1]), and-platformthemenot matching.char**→const char* const*is a valid qualification conversion. MatchesQGuiApplicationPrivate's behaviour;-platform=xcbis not a form Qt accepts, so not handling it is correct.- The
withoutUrlQueries()regex. The diff I was handed had its backslashes stripped (\b,\s,\1appearing as single-escape), which would have made it ill-formed C++. The real file at:337is correctly double-escaped and yields a valid PCRE with Qt's\1\2replacement (not$1$2). I traced it against the realistic shapeError transferring https://host/x.whl?X-Amz-Signature=… - server replied: Forbidden: group 1 stops at?, the lazy\S*?consumes the query, the greedy punctuation class re-attaches a trailing).", and the lookahead lands on the space. Not a finding — flagging it because the mangled diff makes it look like one. - Leak paths on the new BNR lines. Every
fail()argument is self-constructed; the only server-influenced inputs arem_reply->errorString()(:584,:632,:707) and the index URL, and all pass throughwithoutUrlQueries()before both the log andemit finished(false, shown), so the panel'sFailed:text is stripped too. No raw URL is logged anywhere in the file.logTransferFailure()foldingOperationCanceledErrorinto the timeout branch is safe becausecancel()callsm_reply->disconnect()beforeabort()(:360-361), so a user cancel never reaches it. - The three rerouted
fail()exits.install()callsfail()beforem_busy = true;fail()only closesm_dlFile, removesm_tmpFileand clearsm_busy, all no-ops there, and it deliberately keeps staging so a retry resumes.assembleAndCommit()setsm_busy = falsethen callsfail(), which sets it again — harmless. The emitted message text is unchanged for all three (no URL to strip). - Dead additions / sibling call sites.
src/QtPlatformChoice.cppis compiled in bothCMakeLists.txt:1691andtests/tests.cmake:2894;requestPicksWayland()is header-inline, which is why the test needs noGpuSelector.cpp.applyAtStartup()has exactly one caller (main.cpp:324) and the new parameter defaults, so no sibling is left behind.setBackendForTest/handleStatusForTestalready exist (RadioModel.h:1712,:1682). The doublerestartStreamSequence()on the DAX-removed path (unregisterDaxStream()thenRadioModel.cpp:10696) is idempotent. - The one thing I actually executed:
node tools/docs/test_log_rules.mjson the head checkout →OK: 707 checks passed for 45 rules, with no "NOTE: cited line number(s) moved". That independently confirms the five new rules compile, fire on their samples, stay silent onclean.log, cite realsrc/literals at the exact cited lines, and resolve their docs anchors — including thatbnr-download-failed's Stable link/bnr-gpu-noise-removal#the-download-failsexists in the snapshot and that the summary row readsqtPlatform === 'xcb'. - CodeGuard leads I refuted (one line each): the
CS-PII-IPhits atNvidiaAfxPack.cpp:49/272/311/312/325are CUDA version strings (12.8.4.1,11.3.3.83), not addresses;CS-VLN-CMDI-AMP/BACKTICK/PIPEinGpuSelector.*,LogManager.cppandindex.jsare&&, backticks and|in comments, C++ expressions and JS template literals with no shell anywhere;CS-VLN-XSS-EVENTinindex.jsis JSX event props with noinnerHTML/eval;CS-VLN-SSRF-*,CS-PII-EMAIL/DL-1and theCS-INJ-*/CS-SEC-*hits land onscreens.json,rules.json,aetherd-touchpoint-tags.jsonandasync_log_writer_test.cpp, i.e. redaction fixtures and detection patterns that must contain the literal shapes they match;CG-EXEC-001onsnapshot_stable.py:72is list-form argv with noshell=True; the SHA-256 pins atNvidiaAfxPack.cpp:50are integrity controls, not credentials.
What I could not check. I am headless: no build, no test execution beyond the Node rules harness, no GUI. Every finding above except that harness run is reasoned from the head checkout, not observed at runtime. I cannot reproduce the bridge-automation table in the body, the real-radio VITA-49 path (the PR says so itself), the stall-timeout branch, or Windows/macOS behaviour. CI at review time: Static checks, Sanitizer option configures, Build docs site and the three CodeQL analyses are green; build, check-macos and check-windows were still in progress — the compile of item 2 is only exercised where ENABLE_NVIDIA_AFX is on (ci.yml:269, :688), so the macOS lane does not cover NvidiaAfxPack.cpp at all.
Recommendation
Request changes — on finding 1 alone. Everything else here is unusually strong work: it implements the triage's rulings rather than the issue's first draft, discloses its three behaviour changes, ships a mutation table, and the one check I could run passed 707/707. The single thing I would not merge as-is is the per-id-only rate limit on a default-on warning fed by an unfiltered UDP socket; adding a global cap (or a bound on how many ids may own a limiter) beside the existing per-stream one is a small change in the seam you already built. If the maintainer prefers to treat that as a follow-up against the pre-existing missing sender filter, this becomes Approve with nits. Thanks for the thorough body and the honest "Not verified" section — it made the review much faster.
🤖 aethersdr-agent · cost: $12.9375 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (264 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.1032 · model: claude-opus-5 |
…ogether. Principle VIII. Review of 410e5e2. The per-stream limiter lets the first error of every stream id log at once, and the VITA socket does not filter by sender, so a burst of new ids could log one warning per id. VitaSequenceLossBudget caps all streams together at 8 lines per 10 s; lines over it are counted, and the next line logged is preceded by "N VITA-49 sequence-error line(s) held back". The error counters are untouched. Also: the URL query stripper ignores scheme case, and the Log Analyzer's privacy note says tokens and keys keep four characters and host names are masked where a log line labels them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
This answers @aethersdr-agent's review of Blocker 1: the warning is rate-limited per stream id only, with no global cap. Fixed.
Proof (
What this PR does not change: the socket's missing sender filter and the unbounded Nits:
CodeGuard reported 264 findings. Same families as before (backticks, Re-run on
|
There was a problem hiding this comment.
1. Issue fit
Partially — by design, and the body says so. #6285 lists four unlogged failures; this PR covers items 1 (VITA-49 sequence errors), 2 (BNR pack download/install) and 4 (Qt platform plugin), and leaves item 3 (HL2 discovery) to #6293. #6285 stays open, which is correct. Each item is mapped: item 1 → VitaSequenceLossReport.h + the processDatagram() hook, item 2 → fail()/logTransferFailure() on aether.nvafx, item 4 → QtPlatformChoice + the main.cpp block after qInstallMessageHandler. The issue's ask that each line be "a stable, greppable string, so the analyzer rule can cite its file:line" is satisfied — I ran node tools/docs/test_log_rules.mjs on the PR head and got OK: 707 checks passed for 45 rules.
Coverage: four new socket-free CTest targets, all registered in tests/tests.cmake. I did not ask for a lossy-VITA-replay test or a fake radio — processDatagram() injection is the right socket-free seam for a parsing/rate-limit concern, and the body's disclosure of the bridge gap (sim vita drop, log file) is the honest coverage boundary rather than a hole.
2. Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/flex/VitaSequenceLossReport.h, PanadapterStream.{h,cpp} |
Item 1: limiter, shared budget, line text, sequence restart on teardown | Yes | In scope |
src/models/RadioModel.cpp (+8) |
restartStreamSequence() for every radio-removed id (covers remote_audio_rx) |
Yes | In scope |
src/core/NvidiaAfxPack.{h,cpp}, NvidiaAfxFilter.h, LogManager.cpp |
Item 2: failure lines, withoutUrlQueries(), lcNvAfx header decl + registry row |
Yes | In scope |
src/QtPlatformChoice.{h,cpp}, src/main.cpp, CMakeLists.txt |
Item 4: the platform line, -platform parsing |
Yes | In scope |
src/core/GpuSelector.{h,cpp} |
willUseWayland() takes the -platform request; predicate extracted as requestPicksWayland() |
Yes (body "Behaviour changes") | In scope — serves item 4 |
docs/user/docs/*, log-samples/*, LogAnalyzer/{index.js,rules.json}, generated/log-categories.md |
Analyzer rules + samples, three troubleshooting sections, summary field, privacy note, generated category row | Yes | In scope |
tools/docs/snapshot_stable.py, docs/user/README.md, .claude/skills/release-prep/SKILL.md |
Snapshot rewrites /next/ rule links to Stable |
Yes (body "Docs") | In scope but adjacent — supporting tooling the new /next/ rules need; reasonable to keep bundled |
tests/* (4 new), tests/tests.cmake, tools/docs/test_log_rules.mjs |
Coverage for the above | Yes | In scope |
No CHANGELOG.md entry — correct. No new settings key, protocol verb, capability field or dialog. No removed guard: I read every - line; the only deletions are the willUseWayland() body moving into requestPicksWayland() (behaviour-preserving, I diffed the branches one by one) and three emit finished(false, …) calls becoming fail(), which is a superset.
Preference check: the one existing behaviour this touches is that AetherSDR no longer imposes QT_QPA_PLATFORM=wayland;xcb when -platform is present. That is a fix, not a preference — Qt already gives -platform precedence over the environment, so the value AetherSDR wrote was dead for Qt while still steering GpuSelector's GLX/EGL decision the wrong way. It points at an authority (Qt's own argument precedence) and is disclosed in the body.
One note on the audit scope handed to me: the security-audit file list named ThemeManager.*, WsjtxClient.cpp, MainWindow*, DxClusterDialog.cpp, RadioSetupDialog.cpp, Theme.h, ThemeEditorDialog.cpp, PanadapterApplet.h, screens.json, aetherd-touchpoint-tags.json, tailnet_shim_client_test.cpp, theme_manager_test.cpp, unified_title_bar_test.cpp. None of those are in this PR — git diff --stat 888ac58bf..HEAD is 36 files and none of them appear. CodeGuard findings on those paths are not attributable to this change and I have not counted them.
3. Blockers
None.
4. Nits (all non-blocking)
-
src/QtPlatformChoice.cpp:50and:59— CR/LF in-platform/QT_QPA_PLATFORMforges an extra log line. Credited to security-audit (CodeGuard),codeguard-0-logging("strip CR/LF/delimiters"), and confirmed:logLines()interpolatesrequestedverbatim —lines << QStringLiteral("Platform: Qt platform plugin \"%1\" (%2)").arg(platformName, request);and
requestedcomes straight fromargvor the environment (src/main.cpp:651-653).AsyncLogWriter::formatLine()(src/core/AsyncLogWriter.cpp:323-335) appliesredactPii()only and does no newline stripping, soQT_QPA_PLATFORM=$'xcb\n[09:14:02.101] WRN default: fake'writes a second, fully attacker-shaped line intoaethersdr.log. The trust boundary is weak — only the local user who owns the log can set either — but the docs Log Analyzer is line-parsed, so a forged line in a shared support bundle can fake a finding. The clean fix is central (strip[\r\n]once informatLine(), covering every logged string), which is bigger than this PR; a local strip inlogLines()closes the new path:static const QRegularExpression newlines(QStringLiteral("[\\r\\n]+")); const QString requested = QString(rawRequested).replace(newlines, QStringLiteral(" "));
Maintainer's call whether to take the local patch here or route the central one separately.
-
src/core/backends/flex/VitaSequenceLossReport.h:68-82+src/core/backends/flex/PanadapterStream.cpp:808-809— when the shared budget denies, the per-stream limiter has already spent its report. The short circuit isif (stats.lossLog.recordError(m_seqLossClock.elapsed()) && m_seqLossBudget.allow(m_seqLossClock.elapsed())) {recordError()returning true has already donem_pending = 0; m_lastReportMs = nowMs; m_windowStartMs = nowMs;. Ifallow()then returns false, that error count is dropped on the floor and the stream is muted for another 10 s, so the errors are in neither the suppressed line nor any later line — only the "N line(s) held back" count hints at it. Narrow (needs >8 streams reporting inside one 10 s window) and the live counters still have the truth, but the honest shape is to ask the budget first, or to roll the pending count back when the budget denies. -
src/core/NvidiaAfxPack.cpp:386— the cancel path is detected by string compare.if (shown == QLatin1String("cancelled"))couples the Info-vs-Warning choice to a literal in a caller. Abool cancelledparameter (orm_cancelled) is sturdier; as written, a rewordedfail("cancelled during unpack")silently becomes a Warning "install failed". -
docs/user/log-samples/rules/bnr-install-failed.log:8— the sample's reason is empty:NvidiaAfxPack: install failed: extract failed:with nothing after the colon. It exercises the rule, but a realistic sample carries a reason (e.g.tar: unrecognized option '--zstd'), which is what a reader of the sample is trying to learn. -
src/QtPlatformChoice.cpp:14-23—fellBack()'s two-way prefix match has a blind spot. It exists to makewayland≡wayland-egl, which is right, but it also meansQT_QPA_PLATFORM=minimalegl;minimallanding onminimalreports no fallback ("minimalegl".startsWith("minimal")). Obscure enough to leave alone; worth a comment. -
src/gui/SpectrumWidget.cpp:1999-2014—-platform xcbon a genuinely headless Wayland session now takes the "or you forced xcb yourself" arm, whose advice isQT_QPA_PLATFORM='wayland;xcb'— the thing the headless arm above calls "the #4747 bug". That is because the-platformbranch no longer runssetDetectedDisplayPresence(). It is exact parity with the pre-existingQT_QPA_PLATFORM=xcbescape hatch, so this PR makes the two consistent rather than introducing anything; naming it only so it is on the record. -
docs/user/src/components/LogAnalyzer/rules.json:477—src/models/RadioModel.cpp:12671is already stale against currentmain(the string is at:12702after #6319, ~:12710merged). No action needed:test_log_rules.mjs:92-101deliberately treats a moved-but-present citation as aNOTE, not a failure, so Static checks will not redden. Flagging it so nobody chases it, and so the four refreshed line numbers in this diff are understood as cosmetic.
5. What I tried to break
- The
withoutUrlQueries()regex (src/core/NvidiaAfxPack.cpp:332-341). I walked: a query containing dots and=; trailing closing punctuation ((…?sig=1).); two URLs in one string with no whitespace inside either; a URL with no query at all; a URL at end-of-string; mixed-case scheme.\S*?cannot cross whitespace and(?=\s|$)anchors the end, so every expansion either reaches the token boundary or over-strips — I found no input that leaves a query standing.\1\2is the correct Qt backreference form. The one gap is URL userinfo (https://user:pass@host/…), which[^\s?#]+admits — butAsyncLogWriter.cpp:313-315's authority rule redacts the whole authority in the log file, so only the same-user GUI "Failed:" string keeps it. The helper is load-bearing, not belt-and-braces: nothing else strips a query, andLogRedactionPolicy.hhas nosignature/X-Amz-*keyword. - Lock ordering around the new
m_statsMutexwork.clearRegisteredStreams()now callsrestartAllStreamSequences()(takesm_statsMutex) while holdingm_streamMutex— but the pre-existingresetAudioStreamStats()call two lines above does exactly the same, so the order is established, not new. I grepped everym_statsMutexacquisition inPanadapterStream.cpp(lines 473, 483, 783, 1418, 1490, 1499, 1511, 1517) for the inverse nesting and found none. Note the author deliberately movedrestartStreamSequence()outside them_streamMutexscope in all fourunregister*Stream()functions. - The lazily-started
m_seqLossClock.elapsed()on an invalidQElapsedTimerwould be the obvious trap. It cannot happen: the error path requiresstats.lastSeq >= 0, which requires a prior packet, which ran the!stats.startedbranch that starts the clock.restartStreamSequence()setsstarted = false, and the next packet re-runs that branch. - Churning a stream to log faster.
VitaSequenceLossLimiter::start()resetsm_windowStartMsand dropsm_pendingbut deliberately keepsm_lastReportMs, so a stream torn down and re-created in a loop still cannot log more than once per 10 s.restartStreamSequence()on an unknown id usesfind()and creates no entry, so a stray status does not populate the map. platformArgument()edge cases. A bare-(reads the NUL terminator, no overrun),--→-(strcmp fails),-platformas the last argument (i + 1 < argcguard, and Qt ignores it too),-platformtheme(distinct string), and last-occurrence-wins. MatchesQGuiApplicationPrivate's loop as the comment claims. Also confirmedQApplication app(argc, argv)is atsrc/main.cpp:391, well before the logging block at:645, soQGuiApplication::platformName()is populated and the line never readsplugin "".GpuSelector::applyAtStartup()'s new default argument.src/main.cpp:324is the only call site insrc/ortests/; theQ_UNUSEDguard covers the non-Linux arms.requestPicksWayland()reproduces the old branch set exactly (firstcontainswayland→ true,xcb→ false, else session), andQListis needed forQByteArray::split's return, so the added include is not churn.- The three
emit finished(false, …)→fail()conversions.fail()on a non-busy pack is idempotent:m_dlFileis null,m_tmpFileempty,m_busy = falseis already false. No staging is deleted, so the "completed components resume" behaviour is intact. - The redaction chain over the new lines.
could not install into <path>collapses to~viaredactPii()'s home rules; the VITA lines carry only a stream id and counters, no addresses; no new line uses atoken=/key=/password=keyword that the keyword table would mangle. I checked the privacy note atindex.js:132-141againstLogRedactionPolicy.handAsyncLogWriter.cpp:182-300claim by claim — IPv4 last octet, MAC last group, IPv6 replaced, serial last group, home →~, emails,keepPrefixChars = 4for tokens/keys and0for passwords/names/grids, host names only after a connection keyword, callsign kept (docs/log-redaction.md:53). It is accurate.session_idis also masked and the note does not name it; too small to list. - Stale-citation failure on the merged tree. Current
mainis five commits past this PR's base and #6319 added +39 lines toRadioModel.cppabove most of the cited lines. I confirmed fromtest_log_rules.mjs:90-104that a moved-but-present citation is aNOTE, not acheck(false)— so the 707-pass I got on the PR tree holds for the merge. (I was denied permission to create the scratch merge worktree, so this is read from the checker's logic plus the measured line offsets, not from a merged run.) - Socket tests: I grepped the four new test sources for
QTcpServer|QTcpSocket|QUdpSocket|QLocalServer|bind(|listen(|connectToHost— no matches. None of them opens, binds, listens or connects;vita_sequence_loss_log_testreachesprocessDatagram()through aVitaSequenceLossLogTestAccessfriend on aPanadapterStreamthat never binds. Nothing to surface under the socket-test rule, and no fake radio/amplifier/tuner peer was added. - CodeGuard classes I refuted (verified line by line in the head checkout):
CS-VLN-SSRF-PRIV-10/CS-PII-IPonNvidiaAfxPack.cpp:49,272,311,312,325are the NVIDIA component table's version strings (12.8.4.1,11.3.3.83) and compile-time constant URLs, all SHA-256 pinned withNoLessSafeRedirectPolicy.CS-VLN-CMDI-*onGpuSelector.{h,cpp}andLogManager.cppare C++&&and markdown backticks — there is no shell anywhere in the diff (QProcess::start(program, QStringList);snapshot_stable.pyuses a list argv, noshell=True).CS-VLN-XSS-EVENTonindex.jsis ReactonChange/onClickprops; every log-derived value renders through JSX text interpolation with noinnerHTML/dangerouslySetInnerHTML/eval.CS-VLN-PATH-UNConrules.json:744,1003is\\SJSON escaping;CS-PII-DL-1/CS-VLN-SSRF-HEXat:88-89is the serial/hex-stream-id regex in the rule corpus.CS-SEC-PASSWORD/BEARER/BASIC-AUTHare the redaction-policy keyword tables and analyzer fixture prose — a tree grep forAKIA|ghp_|xox[bp]-|-----BEGIN|sk-[A-Za-z0-9]{20}found nothing. - Not checked / out of reach: everything above except
test_log_rules.mjs(which I ran) is reasoned from reading the code, not reproduced at runtime — no build, no ctest, no GUI. I could not exercise the stall-timeout, checksum or unpack branches, matching the body's own "Not verified" list. CI at review time is not green yet: only "Build docs site" had completed (success);build,Static checks,check-windows,check-macos,Sanitizer option configuresand the three CodeQLAnalyzejobs were queued or in progress. Theaether.nvafxlines areENABLE_NVIDIA_AFX-gated, socheck-windows(the-DENABLE_ASR=OFFlane) and the macOS lane are the ones worth watching for a configuration-scope break.
6. Recommendation
Approve with nits. This is careful work: the rate limiter is correctly confined to the error path, the shared budget anticipates exactly the stream-id-burst problem that would have flushed the cause out of a support bundle's tail, the lock discipline is deliberate (restart moved outside m_streamMutex in the unregister paths), the sequence-restart-on-teardown reasoning about reused stream ids is right, and withoutUrlQueries() closes a real signed-query leak that nothing else in the redaction chain covered. The -platform correction is a genuine fix rather than smuggled preference, and the privacy note matches the redaction policy claim for claim.
Next step: take or defer the CR/LF strip (nit 1) — that one is a maintainer judgment about whether to patch locally here or centralise in formatLine() — and consider reordering the limiter/budget short circuit (nit 2) so held-back errors are not silently dropped. Neither blocks. Wait for build, Static checks, check-windows and check-macos to finish before merging; nothing green has reported on the compiled lanes yet.
Thanks for the unusually thorough proof table and the explicit "Not verified" section — the mutation checks in particular made it fast to confirm the new tests fail against the unfixed code rather than asserting the implementation back to itself.
🤖 aethersdr-agent · cost: $15.4868 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (414 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: $15.6523 · model: claude-opus-5 |
… platform line cannot be split. Principle VIII. Review of 4319913 (no blockers). When the shared budget refuses a due line, the stream's errors are now held for its next line instead of being spent with the report; the budget is asked only when a line is due. A CR or LF in the -platform or QT_QPA_PLATFORM value is folded to a space, so it cannot write a forged second line into the log. fellBack() documents the two-way prefix trade-off, and the bnr-install-failed sample carries a real reason. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
This answers @aethersdr-agent's review of Fixed:
Answered, no change:
Re-run on
|
There was a problem hiding this comment.
Issue fit
Partial by design, and correctly scoped. #6285 lists four unlogged failures; this PR covers items 1 (VITA-49 sequence errors), 2 (BNR pack) and 4 (Qt platform plugin), and the body says item 3 (HL2 discovery) is #6293 and that #6285 stays open. Refs rather than fixes is the right verb. Every log line the body's table promises exists in the tree, each new analyzer rule cites a file:line that really holds the string, and node tools/docs/test_log_rules.mjs passes here with exactly the 707 checks / 45 rules the body claims. The deviation from the issue's suggested aether.protocol to aether.vita49 is explained and better: aether.vita49 is declared QtWarningMsg, so the line reaches a default bundle with no LogManager change.
Coverage: four new socket-free CTest binaries plus three negative-corpus rows in async_log_writer_test. The mutation checks in the body are the kind of evidence that makes a test suite believable, and the tests I read are not self-asserting — qt_platform_choice_test pins exact strings and vita_sequence_loss_log_test pins real numbers (reportErrors() == 1001, lines == 7 over a minute). nvidia_afx_pack_log_test sits inside the ENABLE_NVIDIA_AFX AND x86_64 block, which ci.yml:269 does configure, so it is built in CI.
Socket-test disclosure (§2): none. I grepped every added/modified test for QTcpServer, QTcpSocket, QUdpSocket, QLocalServer, bind(, listen(, connectToHost, Fake* — zero hits. vita_sequence_loss_log_test reaches PanadapterStream::processDatagram() through a friend test-access class on a stream that never binds, and nvidia_afx_pack_log_test only calls installFromFile() on a missing path, never install(). Nothing to flag.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
PanadapterStream.{h,cpp}, VitaSequenceLossReport.h |
item 1: rate-limited sequence-error line, per-stream restart | yes | In scope |
NvidiaAfxPack.{h,cpp}, NvidiaAfxFilter.h, LogManager.cpp |
item 2: failure lines, withoutUrlQueries(), aether.nvafx registry row |
yes | In scope |
QtPlatformChoice.{h,cpp}, CMakeLists.txt |
item 4: start-up platform line | yes | In scope |
main.cpp, GpuSelector.{h,cpp} |
behaviour change beyond logging: a -platform argument now suppresses AetherSDR's wayland;xcb imposition and drives the GLX/EGL (PRIME) prediction |
body yes, #6285 no | In scope, and fix-shaped — Qt's documented precedence of -platform over QT_QPA_PLATFORM is the authority, and the old code's only effect under -platform xcb was to mispredict EGL and drop the GLX vendor hint on an actually-X11 session. Named here so the maintainer sees it. |
RadioModel.cpp |
restart sequence tracking for every stream the radio reports removed | yes | In scope — remote_audio_rx has no PanadapterStream registration, so there is no other seam. Correctly confined to the streamStatusRemoved() branch, so a status for a still-flowing stream never hides a gap. |
tools/docs/snapshot_stable.py, docs/user/README.md, .claude/skills/release-prep/SKILL.md |
release tooling rewrites /next/ rule links to Stable |
body yes, #6285 no | Adjacent scope, disclosed, tooling-only. Coupled to the four new /next/ links, which would otherwise point at Next forever. Not worth unbundling, but a maintainer may disagree. |
docs/user/docs/*, log-samples/*, rules.json, index.js, test_log_rules.mjs |
the analyzer rules, samples, troubleshooting sections, privacy note | yes | In scope |
No CHANGELOG.md entry — correct. No removed guards: the only deletions are the willUseWayland() body (relocated verbatim into GpuSelector::requestPicksWayland, both branch orders preserved) and three emit finished(false, …) exits that now route through fail(), which only adds cleanup it already performed plus the log line. I confirmed every emit finished(false, …) in NvidiaAfxPack.cpp is now the one inside fail().
Blockers
None.
Nits (all non-blocking)
1. src/core/backends/flex/VitaSequenceLossReport.h:85 — the "held back" count reports refused errors, not refused lines.
if (m_used >= kLinesPerInterval) {
++m_suppressed;
return false;
}A stream refused by the budget keeps its due() true (recordError(…, false) does not touch m_lastReportMs), so its next error re-asks the budget and bumps m_suppressed again. With nine streams erroring in one window, the ninth stream's 500 errors contribute 500, and the next line that gets through prints PanadapterStream: 500 VITA-49 sequence-error line(s) held back: more than 8 in 10 s across all streams when about one line was actually held. vita_sequence_loss_log_test.cpp:148 pins takeSuppressed() == 12 for the one-ask-per-line burst, which is exactly the case that does not exercise this. The bound itself is fine — the agent audit and I both traced the worst case to 8 loss lines + 8 notices per 10 s — so this degrades a diagnostic number, not behaviour. The cheapest honest fix is the wording:
inline QString formatVitaSequenceLossSuppressed(int lines)
{
return QStringLiteral("PanadapterStream: %1 VITA-49 sequence error(s) held back: "
"more than %2 line(s) in %3 s across all streams")
.arg(lines)
.arg(VitaSequenceLossBudget::kLinesPerInterval)
.arg(VitaSequenceLossBudget::kIntervalMs / 1000);
}(If you'd rather keep the line-count meaning, allow() needs to charge a refusal once per stream per window, which is a bigger change than the number is worth.)
2. src/core/backends/flex/PanadapterStream.cpp:786 — pre-existing: m_streamStats is unbounded and keyed by an unvalidated stream id. Credit: security-audit (CodeGuard), confirmed by reading.
auto& stats = m_streamStats[streamId];cat != CatCount is reached for any id by two paths with no ownership check — else if (pcc == PCC_METER) cat = CatMeter; keys off the packet class code alone, and isPan/isWf are true for every id while m_knownPanStreams/m_knownWfStreams are empty — and the VITA socket does not filter by sender. I confirmed with git show 888ac58bf:… that both the insert (then line 737) and the CatMeter classification (then line 727) predate this PR, so this is not this PR's bug. What changes is per-entry cost: StreamStats now embeds a VitaSequenceLossLimiter (~40 bytes). The log side is bounded by the new budget, which is the part that mattered. If anyone wants to bound the map, the precedent is in the same file: kMaxOrphanStreams = 32 with LRU eviction on m_orphanStreams. Separate issue, not this PR.
3. src/core/backends/flex/PanadapterStream.cpp:800 — one false sequence error can still survive a teardown. stream removed arrives over TCP while old-instance datagrams may still sit in the UDP receive buffer. Those land after restartStreamSequence(), set started/lastSeq from the old instance, and then the new instance's first packet reads as a gap. The 10 s limiter caps it at one line, and the body's design already accepts "a reused id no longer adds a false error" as a best-effort claim — just not an absolute one.
4. src/core/LogManager.cpp:138 — the aether.nvafx row is unconditional, but nothing writes to it unless HAVE_NVIDIA_AFX. On macOS and any -DENABLE_NVIDIA_AFX=OFF build the Support dialog lists a BNR category that can never produce a line. No other row in the table is #ifdef'd, so this matches house style; mentioning it only in case you'd rather the row be conditional.
5. tools/docs/snapshot_stable.py:92 — page_slugs() uses docs_dir.iterdir(), top level only. The docstring says so, and it fails safe (an unmatched page keeps its /next/ link). A future rule linking /next/generated/<page> would silently never be stabilised. Fine for today's four links.
What I tried to break
withoutUrlQueries()— the diff as rendered to me looked like it had lost its regex escaping and an invalid\1\2replacement. It has not. The file is a non-raw literal compiling to(\bhttps?://[^\s?#]+)[?#]\S*?([)\]}>"',.;:]*)(?=\s|$), and"\\1\\2"is exactly Qt's backreference spelling. I re-implemented the pattern in Python and ran it:Error transferring https://pypi.org/a/b?token=SECRET - server replied: Not Found→ query gone;(https://h/x?sig=1).→(https://h/x).with punctuation kept;HTTPS://H/x#frag end→ fragment gone;https://h/pand non-URL prose untouched. The lazy\S*?does backtrack to let the closing-punctuation group win. It is also genuinely needed —redactPii()'s URL rule masks onlyscheme://authority, leaving path and query, and noLogRedactionPolicy.hkeyword matchessig=/X-Amz-*.- Lock ordering around the new
m_statsMutexwork.clearRegisteredStreams()nests stream→stats (resetAudioStreamStats()thenrestartAllStreamSequences(), sequentially, not recursively — no double-lock on the non-recursiveQMutex). Every otherm_statsMutexsite (PanadapterStream.cpp:473,483,783,1422,1494–1521) takes it alone, and the fourunregister*Stream()calls were refactored sorestartStreamSequence()runs outside them_streamMutexblock — I checkedunregisterDaxStreamat line 1638 specifically, since its hunk reads ambiguously in the diff. No stats→stream path exists, so no inversion. platformArgument()'s claim to mirrorQGuiApplicationPrivate. The---stripping, next-argument, last-occurrence-wins and dangling--platformbehaviours match Qt'screatePlatformIntegration()loop, including that Qt (like this code) does not require the next argument to be a non-option.-platformthemeis correctly not-platform.arg[1]on a bare-reads the terminator, not past the end.fellBack()on thewayland/wayland-eglasymmetry, both directions, plus the documented miss it accepts (minimalegl;minimallanding onminimal).source == Unsetcorrectly suppresses the fallback line so a Qt-default platform never looks like a failure.- Forging a log line through
QT_QPA_PLATFORM/ argv. Only[\r\n]+is folded, and that is sufficient: the analyzer splits on\r\n?/\nonly, so U+2028/NEL/VT cannot start a record, and the[HH:mm:ss.zzz] LEVEL cat: msgformat cannot be forged without a newline. A value containing the literal textPlatform: Qt fell back fromwould make the analyzer report a fallback that never happened — but whoever sets the env var or argv already owns the process, so this is not a trust boundary. The line is emitted afterqInstallMessageHandler(main.cpp:629vs:656), so it does pass throughredactPii(). - CodeGuard's leads, all refuted (via the security-audit agent plus my own reading): CMDI on
&&in C++ boolean expressions and in abuild.shthat is not invoked through a shell;snapshot_stable.py'ssubprocess.run([...], check=True)with a repo-localfile://URI and no shell; XSS-EVENT on JSXonDrop/onChangeprops —index.jshas nodangerouslySetInnerHTML/innerHTML/eval, log text reaches the DOM only as escaped JSX children, and every<Link to=…>comes from build-timerules.json; SSRF on the pre-existing, SHA-256-pinned PyPI resolution this PR only logs; PII/BASIC-AUTH/BEARER on the redaction keyword tables matching themselves. The one lead that survived is nit 2, pre-existing. - The privacy note's accuracy (
index.js:135-140), claim by claim againstredactPii()andLogRedactionPolicy.h: IPv4 last octet, MAC last group, IPv6 full and compressed, Flex serial last group,pass*/name/lat-lon/grid/email keyword fields at 0 kept chars, token/key fields at 4, home-directory collapse, and the correctly-hedged "where a log line labels them as such" for host names. Accurate; it omitssession_id(also masked, 4 chars) and that URL authorities are masked wholesale — both over-masking relative to the note, so not misstatements. nvidia_afx_pack_log_test's link line (aethercore Qt6::Core, noHAVE_NVIDIA_AFX, noQt6::Network) — I expected a link or compile break, sinceNvidiaAfxPack.his#ifdef HAVE_NVIDIA_AFXand the constructor builds aQNetworkAccessManager. Both come through:aethercorecarriesHAVE_NVIDIA_AFXandQt6::NetworkPUBLIC. Also checkedGUI_SOURCESis an explicit list, so the newCMakeLists.txt:1691entry is not a duplicate of a glob.
Not verified / limits. I built and ran nothing except node tools/docs/test_log_rules.mjs (707 checks, 45 rules, passed) and a standalone Python re-implementation of the URL-stripping regex. Every other finding above is reasoned from reading the head checkout, not reproduced at runtime. At the time of writing, Static checks, Build docs site, Sanitizer option configures and the three CodeQL Analyze jobs are green on 8b56385; build, check-windows and check-macos were still in progress — in particular I could not confirm MSVC accepts the new QtPlatformChoice.cpp or the constexpr statics in VitaSequenceLossReport.h, though I found nothing in either that looks MSVC-sensitive. The body's own "Not verified" list (no live radio for item 1, the stall and checksum/unpack branches not triggered in-app, Linux-only) is honest and matches what I could see.
Recommendation
Approve with nits. This is an unusually well-evidenced PR: the behaviour changes beyond logging are disclosed in the body, each is fix-shaped with an authority behind it, the new logic is bounded on both the per-stream and all-streams axes, the redaction helper is real rather than decorative, and the mutation-check list is the sort of thing that is hard to fake. I attacked the regex, the lock ordering, the argv parser, the log-forgery surface, the CMake link lines and the privacy note's claims, and the code survived all of them. The one genuine defect in the new logic is nit 1, which prints a wrong number in a diagnostic line and is a one-string fix. Next step: take nit 1 (or consciously keep the current wording), and wait for build / check-windows / check-macos to land before merging. Nice work, and thank you for filing the two automation-bridge gaps instead of working around them silently.
🤖 aethersdr-agent · cost: $14.1648 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (555 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: $14.3270 · model: claude-opus-5 |
… Principle VIII. Review of 8b56385 (no blockers). A stream the shared budget refuses asks again on each error, so the count it reports is of errors held back, not lines. The notice now reads "N VITA-49 sequence error(s) held back: more than 8 line(s) in 10 s across all streams". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
These are the answers to @aethersdr-agent's review of
Re-run on
|
There was a problem hiding this comment.
Issue fit
Yes, and unusually precisely. #6285's triage thread landed on a specific implementation shape in its second (self-correcting) comment, and this PR follows it line for line: qCWarning(lcVita49) rather than moving to lcProtocol ("packet loss is a warning, so the level is honest rather than a workaround"); Q_DECLARE_LOGGING_CATEGORY(lcNvAfx) added to NvidiaAfxFilter.h because the missing header declaration was the only thing blocking NvidiaAfxPack.cpp from using it; the registry row for the Support-dialog toggle as a nice-to-have rather than a visibility requirement; and the three emit finished(false, …) exits the triage named at :442/:804/:828 rerouted through fail() (now NvidiaAfxPack.cpp:480, :856, :880). Item 4 is scoped exactly as the triage ruled — log the plugin actually selected, and state in the docs that a plugin which fails to load entirely aborts inside the QApplication constructor before any handler exists. Item 3 (HL2 discovery) is correctly left to #6293/@on8st, so #6285 stays open.
Coverage is real and layer-appropriate: vita_sequence_loss_log_test, remote_audio_rx_sequence_restart_test, nvidia_afx_pack_log_test, qt_platform_choice_test are all socket-free CTests, all four registered in tests.cmake, and all four return g_failed ? 1 : 0 (I checked — a test that prints [FAIL] and exits 0 is green forever, and none of these do). The documented mutation checks line up with what the assertions actually pin.
Socket-test record (§2): this PR adds no socket-owning test. vita_sequence_loss_log_test constructs a PanadapterStream and feeds packets straight to processDatagram() via a VitaSequenceLossLogTestAccess friend — the QUdpSocket member is constructed but never bound, listened on, or connected; remote_audio_rx_sequence_restart_test drives RadioModel's status router. Grepping the four new test sources for bind/listen/connectToHost/QTcpServer/QUdpSocket returns only the word "binds" in a comment. Nothing removed either.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/flex/{PanadapterStream.cpp,.h}, VitaSequenceLossReport.h |
item 1: rate-limited sequence-error line, per-instance restart | yes | in scope |
src/core/{NvidiaAfxPack.cpp,.h,NvidiaAfxFilter.h,LogManager.cpp} |
item 2: BNR failure lines, lcNvAfx declaration + registry row, withoutUrlQueries() |
yes | in scope |
src/QtPlatformChoice.{h,cpp}, src/main.cpp, CMakeLists.txt |
item 4: start-up platform line + fallback line | yes | in scope |
src/core/GpuSelector.{cpp,h} |
-platform now drives the GLX/EGL (PRIME) prediction; requestPicksWayland() extracted |
yes, under "Behaviour changes beside the log lines" | in scope — behaviour change, correct (see below) |
src/main.cpp:296 |
AetherSDR no longer imposes wayland;xcb when -platform is passed |
yes, same section | in scope — behaviour change, correct |
src/models/RadioModel.cpp |
restarts sequence tracking for every removed stream id (covers remote_audio_rx) |
yes | in scope |
tools/docs/snapshot_stable.py, docs/user/README.md, .claude/skills/release-prep/SKILL.md |
snapshot rewrites /next/ rule links to Stable |
yes, with a described scratch-tree test | in scope — tooling the new /next/ rules require |
docs/user/docs/*, log-samples/*, rules.json, index.js |
five analyzer rules + samples, three troubleshooting sections, privacy-note rewrite, four moved file:line citations |
yes | in scope |
tests/*, tests/tests.cmake |
four new tests + async_log_writer_test negative rows |
yes | in scope |
Nothing in the diff is unexplained. No CHANGELOG.md entry (correct). No new protocol verb, wire message, config key or settings key; no capability change; no AppSettings.cpp consumer added; no dialog, no new interactive widget — so the persistence, capability, dialog-pattern and a11y canon have nothing to bite on here.
On §6 (preference vs fix): the two behaviour changes are fixes, not preferences, and they can point at an authority. Qt reads QT_QPA_PLATFORM in QGuiApplicationPrivate::createPlatformIntegration() and then overwrites it from the argv loop, so a -platform argument wins — meaning the old willUseWayland() was predicting from a value Qt would discard, and main.cpp was imposing wayland;xcb over an explicit user override. Reading the request Qt will actually use is a correction, not a taste change. Both are disclosed in the body.
Note for the maintainer, not a finding: #6285 carries maintainer-review. The open question the triage flagged ("whether to reuse lcProtocol or register new categories") was answered by the triage's own correction comment, and this PR implements that answer; I don't see a live decision outstanding, but the label is yours to clear.
Blockers
None.
Nits (all non-blocking)
1. src/core/backends/flex/PanadapterStream.h:95 — an in-flight datagram can still seed the new instance, so one false error survives.
restartStreamSequence() runs on the main thread (from RadioModel::handleDaxRxStreamRegistry, PanadapterStream.cpp:10693-side), while processDatagram() runs on the network thread — FlexBackend.cpp:47-48 does new PanadapterStream with no parent and moveToThread(m_networkThread). A datagram belonging to the old instance that is already queued when the stream … removed status is handled arrives after the reset, so it becomes the new instance's "first packet", seeds stats.lastSeq with the old stream's count, and anchors startErrorCount/startTotalCount/the window to a stale packet. The genuinely-first packet of the re-created stream then logs one spurious error.
The mutexes are correct (see below) — this is the seam, not a locking bug, and UDP gives no ordering guarantee against the TCP status line regardless of threading. Bounded to one line and one count per teardown, so commit d87678c's "no longer log false failures" is narrowed rather than eliminated. Worth a sentence in the body rather than a code change.
2. src/core/NvidiaAfxPack.cpp:617 and :655 — two real failure modes are now logged but match no analyzer rule.
fail(QStringLiteral("no %1 wheel for %2 %3 at %4").arg(platformDesc, pkg, ver, indexUrl));
fail(QStringLiteral("no %1 wheel for %2").arg(platformDesc, name));bnr-download-failed requires download failed ( / PyPI lookup failed for / simple-index lookup failed for / write failed for / cannot write cache; bnr-install-failed requires one of the extract/assemble/verify prefixes. "The server no longer publishes the pinned wheel for your platform" is exactly the kind of silent dead end #6285 is about, and it falls between the two rules — as does archive not found (:880). A low-severity catch-all on the bare NvidiaAfxPack: install failed: prefix, or one more alternation branch, would close it.
3. docs/user/docs/bnr-gpu-noise-removal.md:113-120 — the "assembled pack missing" remedy can never work.
The section lists assembled pack missing among the four messages it covers, but the literal is assembled pack missing %1 (AFX-bits archive not published yet?) (NvidiaAfxPack.cpp:825) — a server-side publication gap. Installing unzip/zstd, freeing disk and clicking Download again cannot fix it. Either split it into its own line ("the archive is not published yet; try again after the next release") or drop it from that list and give it a rule of its own.
4. docs/user/docs/low-bandwidth-connections.md:171 — understates the rate limit.
at most once every 10 seconds for each stream
True per stream, but the global VitaSequenceLossBudget cap (8 lines per 10 s across all streams, plus the … held back: more than 8 line(s) in 10 s across all streams notice) isn't mentioned, so a user who sees that notice has nothing in the docs to match it against. One clause would do it.
5. src/core/backends/flex/VitaSequenceLossReport.h:70-71 — the burst cap bounds bursts, not the sustained rate.
The budget's stated purpose is to stop lines pushing "the cause out of a support log's tail", but 8 lines / 10 s is 48 lines a minute sustained — a ten-minute lossy session can put several hundred warnings in the log, and the bundle tail is 100 lines. Five categories at the per-stream 1-per-10 s rate gets to ~30/min on its own. Not a defect, and the counters remain honest, but if the goal is tail legibility, a back-off after N consecutive reporting windows (or a widening interval) would get closer to it. Judgment call — flagging the reasoning, not prescribing a change.
6. src/main.cpp:324 — -platform "" is treated two ways.
g_qpaPlatformArgument is truthy on an empty-string argument, so main() skips the wayland;xcb imposition, but value_or(QString()) collapses "absent" and "empty" for GpuSelector, which then falls back to QT_QPA_PLATFORM / the session. Degenerate input, harmless either way; mentioning it only because the two call sites disagree on what an empty -platform means.
7. src/models/RadioModel.cpp:10694 — redundant with the unregisterDaxStream() change.
m_panStream->unregisterDaxStream(stream.streamId) on the line above now calls restartStreamSequence() itself (PanadapterStream.cpp:1638), so the explicit call double-restarts for dax_rx ids. Idempotent, and the comment explains why the explicit call exists at all (remote_audio_rx has no registration) — so this is a readability note, not a bug.
Security audit
Ran the security-audit subagent over the flagged file set at /tmp/aetherclaude/pr-6308. 0 confirmed defects, 2 defence-in-depth leads, both about unchanged code, so neither is a change request against this PR:
src/core/AsyncLogWriter.cpp:328—formatLine()runsredactPii(message)and nothing else; there is no central CR/LF strip before the[HH:mm:ss.zzz] LVL cat: msg\nline is written, so log-injection safety is per-call-site. This PR gets its own site right (QtPlatformChoice.cpp:53-54folds[\r\n]+out of the argv/env-derived request before it reaches a.noquote()stream, andanalyze.mjssplits on/\r\n?/only), but a structural strip insideformatLine()would make the guarantee hold for every future.noquote()site. Maintainer call, separate PR.src/core/NvidiaAfxPack.cpp:332-341—withoutUrlQueries()is the right control, correctly placed on all fourerrorStringpaths (logTransferFailureat:82, used from:588/:636/:715, plusfail()at:384covering thedownload failed (%1): %2message) and covered bynvidia_afx_pack_log_test.cpp:77-92. But it is site-local for a global gap:redactPii()strips only the URL authority (AsyncLogWriter.cpp:313-315) and leaves path and query intact, so other subsystems logging a URL with a signed query are unredacted (src/asr/AsrModelManager.cpp:212,src/asr/RemoteAsrBackend.cpp:43show the shape). Whether the query strip belongs inredactPii()is a maintainer decision; nothing leaks from the changed code.
The audit also confirmed the privacy paragraph at docs/user/src/components/LogAnalyzer/index.js:132-141 matches redactPii() field for field against LogRedactionPolicy.h and docs/log-redaction.md (IPv4 last octet, MAC last pair, IPv6 whole, serial last group, password keepPrefixChars=0 vs token/key =4, home dir, bare email, labelled host/name/location/grid, callsign deliberately kept). It understates in one direction only — URL authority is redacted unconditionally and the note doesn't say so — and never overclaims. Good.
On the CodeGuard list specifically: most of it is noise against this PR. Nineteen of the flagged hits are in files not in git diff main...HEAD at all — .claude/skills/tag-release/scripts/check_release_assets.py, .github/workflows/windows-installer.yml, docs/architecture/aetherd-touchpoint-tags.json, docs/user/screens.json, and the entire tools/flex-tailnet-shim/ tree (the harness scanned the whole checkout, not the change). Of the hits that do land on changed files:
CS-VLN-CMDI-AMP/-BACKTICKonGpuSelector.h:48,53,LogManager.cpp:155,162,GpuSelector.cpp:196,216— refuted: these are C++&&/&operators and Markdown backticks in comments, not shell.CS-VLN-XSS-EVENT×5 onLogAnalyzer/index.js:175-205— refuted:grep dangerouslySetInnerHTMLreturns nothing; the component renders{x.text}/{f.title}/{f.explanation}as React text nodes. Those lines are also untouched by this diff.CS-VLN-PATH-UNConQtPlatformChoice.cpp:53andrules.json:744,1003— refuted: the first is the--→-argv handling, the latter two are Windows paths inside rule/sample strings.CS-SEC-PASSWORD/BEARER/BASIC-AUTHandCS-PII-DL-1insiderules.json— refuted: those are the analyzer's own detection regexes.CS-VLN-SSRF-*on the download paths — refuted: everyQNetworkRequestinNvidiaAfxPack.cpp(:577,:625,:674) setsNoLessSafeRedirectPolicy, the hosts are hard-codedhttps://literals, and every artifact is checked against a pinned or index-declared sha256 at:719-724.
What I tried to break
- Lock inversion on the new
m_statsMutexpaths. The modifiedunregister{Pan,Wf,Dax,Iq}Stream()all releasem_streamMutexbeforerestartStreamSequence()takesm_statsMutex, butclearRegisteredStreams()callsrestartAllStreamSequences()while holdingm_streamMutex— so I went looking for the reverse order. It isn't new:clearRegisteredStreams()already calledresetAudioStreamStats()(which takesm_statsMutex) inside the same scope, and them_statsMutexholders at:1422/:1494-1521don't nestm_streamMutexinside. Order isstreamMutex → statsMutexthroughout; no inversion introduced. The sequence-loss line is also formatted and emitted after the stats lock's scope closes (:843-853), so no logging under the lock. - Log injection through the platform line.
QT_QPA_PLATFORMand-platformare the only attacker-ish inputs here, and the line is.noquote().logLines()folds[\r\n]+to a space beforefirstRequested()is derived from it, so neither the plugin line nor the fallback line can be split;platformNameis Qt's own plugin string. Confirmed, and the security audit independently reached the same place. withoutUrlQueries()on adversarial text. Walked the lazy\S*?+ punctuation-class +(?=\s|$)construction by hand on(https://h/x?sig=1&v=1.2).,"https://h/x?token=a",?a=(b)c, and a URL mid-sentence followed by a comma — each yields the URL with closing punctuation kept and the query gone. Then I checked the one shape it misses: a URL withuser:pass@hostuserinfo and no query is untouched — butdocs/log-redaction.mdsaysredactPii()already redacts the URL authority unconditionally, so that's covered downstream. No gap.platformArgument()vs Qt's real parser. Compared againstQGuiApplicationPrivate's loop: one leading-of--dropped, value is the next argv entry, last occurrence wins, a trailing-platformwith no value consumes nothing,-platformthemeis not a prefix match. All four hold;arg[1]on a bare"-"reads the terminator, which is legal. Qt has no-platform=valueform either, so there's nothing missed there. I also confirmed Qt's argv loop runs after theQT_QPA_PLATFORMread increatePlatformIntegration(), which is what makes the precedence claim true.fellBack()false positives.wayland;xcb→xcbreports;wayland→wayland-eglandwayland-egl→waylanddon't (the two-way prefix test);minimalegl;minimal→minimaldoesn't, which the comment atQtPlatformChoice.cpp:16-18names as the known cost. Thesource != Unsetguard means an unsetQT_QPA_PLATFORMnever produces a fallback warning. No spurious-warning input found.- A cancel misreported as a stall.
logTransferFailure()mapsOperationCanceledErrorto "transfer timed out (stall limit 30 s)", which would be a lie if a user-initiated cancel could reach it. It can't:cancel()callsm_reply->disconnect()beforeabort(), and the lookup lambdas at:588/:636hang off that samem_reply, so they never fire on cancel. The write-failureabort()is checked viam_dlWriteFailedbefore the network-error branch at:713.kTransferTimeoutMsis 30000, matching the docs. The comment's claim holds. - The limiter/budget interaction.
mayReport = !due() || allow()charges the shared budget only when a line would actually be emitted or refused, so a clean-but-chatty stream never spends it; a refused line keepsm_pendingand leavesm_lastReportMs, so the errors reach the stream's next line. One by-design consequence: errors refused by the budget are counted in the held-back notice and again in that stream's next per-stream line. Cosmetic, mentioned for the record. - The "one place" claim for
remote_audio_rx.RadioModel::onStatusReceived()(:10753-10755) callshandleRemoteAudioRxStreamStatus()andhandleDaxRxStreamRegistry()unconditionally for everystream …status, and the registry handler routes removals by id with notype=filter — so the restart really does cover a stream with noPanadapterStreamregistration. Verified in the router, not just in the test. - Dead additions and build scope.
src/QtPlatformChoice.cppis registered on theAetherSDRtarget (CMakeLists.txt:1691) and compiled directly intoqt_platform_choice_test(tests.cmake:2892-2895);GpuSelector::requestPicksWaylandis header-inline, which is why that test links onlyQt6::Coreand doesn't needaethercore.applyAtStartup(has exactly one caller (main.cpp:324), so the default argument hides no site now silently passing an empty string. All four new tests are intests.cmakewithadd_test. - The rule citations — this one I actually ran.
node tools/docs/test_log_rules.mjson the head checkout: 707 checks passed for 45 rules. That verifies every citedfile:lineinrules.jsonstill resolves to the quoted text, including the four citations this PR moved (PanadapterStream.cpp221→234 and 175→188, 1686→1766;RadioModel.cpp12653→12671;main.cpp80→84) and the newQtPlatformChoice.cpp:57,74andVitaSequenceLossReport.h:125. It also provesrules.jsonparses, which the diff's rendering of\(/\Shad made ambiguous. - Tests that would pass against the unfixed code. Each new test's
main()returnsg_failed ? 1 : 0, so a[FAIL]is a real failure. The assertions are behavioural rather than self-referential — e.g. "burst of new ids: lines stop at the shared budget" comparesg_warnings.size()againstkLinesPerIntervalwhile separately assertingpacketErrorCount() == 20, which would fail both with no budget (20 lines) and with a budget that ate the errors. - What I could not check. Everything above is reasoned from the code and from the one tool run I named; I have no build and no runtime, so I did not reproduce a single log line. I could not verify the body's Linux bridge transcript, the
TimeoutErrorprobe, or the untriggered checksum/unpack/stall paths — the body already labels those as unverified, which is the right disclosure. Windows and macOS behaviour is unexercised by anything I can run;check-windowsdoes compileQtPlatformChoice.cppvia theAetherSDRtarget, so MSVC coverage is pending rather than absent (_USE_MATH_DEFINES/NOMINMAX-class breaks don't apply — no math constants, nowindows.h). On CI at77a2404: Static checks, Sanitizer option configures, Build docs site and all three CodeQL analyses are green; build, check-macos and check-windows were still in progress when I looked, so the compile verdict is genuinely unknown, not green. The branch is also ~5 commits behindmain(base888ac58bf); CI tests the merge, so that's informational.
Recommendation
Approve with nits, contingent on the three in-flight build jobs going green.
This is the most carefully-built PR of its kind I've reviewed on this repo: it implements the triage's corrected ruling rather than its first draft, discloses its two real behaviour changes in a dedicated section instead of burying them, documents what it did not verify, and backs the tests with mutation checks that the assertions bear out. The hostile passes I care about most — lock ordering, log injection, the redaction regex, Qt's own argv precedence, and whether a cancel can be mislabelled a stall — all survived. The test_log_rules.mjs run is independent confirmation that no citation drifted.
Nothing here should hold the merge. Concrete next step: wait for build/check-macos/check-windows, then fold nits 2-4 (the unmatched no … wheel / archive not found failures, the unfixable "assembled pack missing" remedy, and the missing global-cap clause in low-bandwidth-connections.md) into this PR if convenient — they're all one-liners in files already touched. Nits 1 and 5-7 are judgment calls worth a reply in the thread rather than a change. The body-disclosed pre-existing bug (updateBnrStatus() overwriting the Failed: <msg> label right after it is set, so the reason for a failed download appeared nowhere before this PR) deserves its own issue, not a fix here. Thanks for the unusually honest "Not verified" and "bridge gaps" sections — the sim vita drop and log file [grep] proposals are worth issues of their own.
🤖 aethersdr-agent · cost: $17.3145 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (566 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: $17.5004 · model: claude-opus-5 |
|
Answers to @aethersdr-agent's review of
CI on |
There was a problem hiding this comment.
Reviewed at the PR head 77a2404d9 against base 888ac58bf, with the head checkout at hand. Everything below is reasoned from the code — I have no build and no test run here, so nothing in this review was reproduced at runtime.
1. Issue fit
Yes, for the scope the triage of #6285 reduced it to. The issue listed four gaps; this PR takes 1, 2 and 4 and explicitly leaves 3 (HL2 discovery) to #6293, which @on8st claimed in the thread — that split is disclosed in the commit body and matches the maintainer-review triage.
Requirement-by-requirement:
| #6285 item | Triage ruling | Where the diff does it |
|---|---|---|
| VITA-49 sequence errors, rate-limited | "use qCWarning(lcVita49)… packet loss is a warning, so the level is honest" |
VitaSequenceLossReport.h:1-135 + PanadapterStream.cpp:775-852, qCWarning(lcVita49) ✔ |
…"rate-limit off m_streamStats[id].errorCount, not the global accessor" |
per-stream, not packetErrorCount() |
per-stream StreamStats::lossLog ✔ |
BNR failures: declare lcNvAfx in a header |
NvidiaAfxFilter.h had no Q_DECLARE_LOGGING_CATEGORY |
NvidiaAfxFilter.h:13 ✔ |
BNR: fail() + "three exits bypass fail()" (:442, :804, :828) |
reroute them | NvidiaAfxPack.cpp:480 (no GPU), :856 (verification), :880 (archive not found) — and grep "emit finished(false" now returns exactly one hit, inside fail() ✔ |
| Registry row "worth doing so the Support dialog gets a toggle" | optional | LogManager.cpp:138 + regenerated log-categories.md ✔ |
Qt platform: unconditional platformName() line |
yes | main.cpp:645-658 ✔ |
| Qt platform: "diagnose a plugin that failed to load is out of reach" | out of scope | the PR infers the fallback by comparing request vs platformName(), and both the code comment (QtPlatformChoice.h:17-19) and the docs say Qt's own reason stays on stderr. Honest, and more than the triage asked for ✔ |
I verified the corrected triage premise myself before accepting the category choice: LogManager::applyFilterRules() (src/core/LogManager.cpp:208-227) emits only aether.*.debug=false plus per-category debug/info enables, so adding aether.nvafx to m_categories while it defaults off (loadSettings(), :373-378) does not suppress its qCWarning. The new lines are visible in a default support bundle. That was the trap worth checking (the "TCI logs absent from bundles" pattern) and it does not bite here.
Tests: three new socket-free CTests plus one regression test, all registered in tests/tests.cmake, and each would fail against the unfixed tree because the code they exercise does not exist there. tests/vita_sequence_loss_log_test.cpp:68-120 is a real limiter spec (interval holding, held-error accounting, restart-across-interval), not an implementation echo. Coverage is adequate.
2. Scope
Note first: the file list I was handed is incomplete — it omits tests/tests.cmake, three of the four new test files, tools/docs/snapshot_stable.py and tools/docs/test_log_rules.mjs. The table below is built from git diff 888ac58bf..77a2404d9 --stat (36 files, +1510/−57).
| File(s) | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/QtPlatformChoice.{h,cpp}, CMakeLists.txt:1690, src/main.cpp |
the start-up platform lines | yes | in scope |
src/core/GpuSelector.{h,cpp} |
willUseWayland() now takes the -platform argument; list-first logic extracted to requestPicksWayland() |
commit 43ed8bf only |
in scope — -platform beats QT_QPA_PLATFORM, so the old env-only prediction was wrong once we log the real plugin. Behaviour-preserving: I diffed the extracted predicate against the deleted body line for line |
src/core/backends/flex/VitaSequenceLossReport.h, PanadapterStream.{h,cpp} |
the sequence-error line, limiter, global budget, instance restart | yes | in scope |
src/models/RadioModel.cpp (+8) |
restartStreamSequence() on every removed stream id; one comment |
commit 43ed8bf/e23a407 |
in scope (covers remote_audio_rx, which has no registration in PanadapterStream) |
src/core/NvidiaAfxPack.{h,cpp}, NvidiaAfxFilter.h, LogManager.cpp |
BNR failure logging, withoutUrlQueries(), category |
yes | in scope |
tests/* (4 new + async_log_writer_test.cpp +10) |
coverage; the async_log_writer_test rows are a negative corpus for the VITA lines' addresses |
partly | in scope |
docs/user/docs/*, log-samples/*, LogAnalyzer/{index.js,rules.json}, test_log_rules.mjs |
5 rules, 5 samples, 3 troubleshooting sections, a summary row, and the rewritten privacy note | yes | in scope |
tools/docs/snapshot_stable.py (+50), docs/user/README.md, .claude/skills/release-prep/SKILL.md |
new: snapshot_stable.py rewrites "docs": "/next/<page>" rule links to Stable for every page the new snapshot has |
commit 43ed8bf subject line only |
name it — not a blocker, but nothing in #6285 asks for it and it changes the release process. Please say so in the PR body so the release-prep owner sees it |
No removed guards (the only - lines of substance are the willUseWayland() body, replaced by an equivalent, and the three emit finished(false, …) exits, replaced by fail()). No CHANGELOG.md entry — correct. No new protocol verb, settings key or capability field; restartStreamSequence() is new public C++ surface on an internal class, which is fine. No dead additions: QtPlatformChoice.cpp is referenced from both CMakeLists.txt:1690 and tests.cmake:2892-2898, and every new rule has its sample.
3. Blockers
1. src/core/NvidiaAfxPack.cpp:388 — subprocess stderr reaches the log with its newlines intact, so one failure writes several lines and only the first is prefixed.
qCWarning(lcNvAfx).noquote() << "NvidiaAfxPack: install failed:" << shown;shown is withoutUrlQueries(msg), and withoutUrlQueries() (:332-341) strips URL queries and fragments only. Two fail() callers pass unzip/tar stderr straight through — "extract failed: %1" with QString::fromLocal8Bit(p.readAllStandardError()).trimmed() (:775, :814); .trimmed() removes leading/trailing whitespace, not interior newlines. Nothing collapses them downstream: AsyncLogWriter.cpp:329-333 is QString("[%1] %2 %3: %4\n") and redactPii() only bounds some of its patterns with [^\r\n].
Concretely, tar does not emit one line — tar: unrecognized option '--zstd' is followed by Try 'tar --help' or 'tar --usage' for more information., so the log gets:
[10:05:37.920] WRN aether.nvafx: NvidiaAfxPack: install failed: extract failed: tar: unrecognized option '--zstd'
Try 'tar --help' or 'tar --usage' for more information.
The second line has no timestamp, level or category, which is exactly the shape the line-based Log Analyzer parses. The sample this PR ships (docs/user/log-samples/rules/bnr-install-failed.log:8) is the one-line best case, so the rule test won't catch it. The PR already establishes the house rule for precisely this reason at src/QtPlatformChoice.cpp:52-56 ("a CR or LF in it would write a second, forged line into the log the Log Analyzer reads") — this is the same seam, unguarded. Credit: also raised by security-audit (CodeGuard), rule codeguard-0-logging.md, which notes the sharper version — installFromFile() (:877) verifies no pinned checksum, so a hostile third-party pack's member names land in tar stderr and can forge a correctly-formatted log line. I'd fix it for the mundane two-line tar case regardless.
Drop-in, mirroring QtPlatformChoice::logLines():
QString NvidiaAfxPack::forOneLogLine(const QString& text)
{
// Subprocess stderr and network error strings are multi-line: a CR or LF
// would write a second, unprefixed line into the log the Log Analyzer reads.
static const QRegularExpression newlines(QStringLiteral("[\\r\\n]+"));
QString out = withoutUrlQueries(text);
return out.replace(newlines, QStringLiteral(" "));
}then use it at :388 and in logTransferFailure() (:75-82), and keep withoutUrlQueries() alone for the emit finished() panel text if you want the panel to stay multi-line. A one-line alternative is to apply the same replace() inside withoutUrlQueries() and rename it; your call which reads better.
4. Nits (non-blocking)
src/core/backends/flex/VitaSequenceLossReport.h:106-113— the held-back notice double counts. When the budget refuses a line,allow()bumpsm_suppressedand the error stays in that stream'sm_pending, so it is reported twice: once as "N VITA-49 sequence error(s) held back" and again inside that stream's next… N in the last S s. Worth a word in the message ("…held back from an earlier line; they are counted in their own stream's next line") so nobody adds the two numbers.src/core/NvidiaAfxPack.cpp:385—if (shown == QLatin1String("cancelled"))couples the Info-vs-Warning decision to a string literal written at three other sites (:710,:736,:907). A file-localconstexpr auto kCancelled(or checkingm_cancelled) keeps a future reword from silently turning a user cancel into a warning.src/QtPlatformChoice.cpp:68-71—QT_QPA_PLATFORM=(set but empty) logsPlatform: Qt platform plugin "xcb" (QT_QPA_PLATFORM=, set by the user). Harmless, slightly odd to read;requested.isEmpty()could fall through to theUnsetwording.src/models/RadioModel.cpp:10691— for adax_rxid this is the secondrestartStreamSequence()call, sinceunregisterDaxStream()(PanadapterStream.cpp:1638) already does it. Idempotent, so purely cosmetic; the comment explains why the broader call is needed.- The four new tests are in no
ctest -Rfilter inci.yml, so they are compiled in the PR lanes (andnvidia_afx_pack_log_testonly whereENABLE_NVIDIA_AFX=ON, i.e.ci.yml:269) but executed only byfull-suite.yml. That is this repo's normal arrangement, not a defect — flagging it so "CI green" isn't read as "these tests passed".
5. Socket-test disclosure
Required record, inspected in the PR's own sources rather than the base tree: no new or modified test in this PR opens, binds, listens on or connects a socket. grep for QTcpServer|QTcpSocket|QUdpSocket|QLocalServer|bind(|listen(|connectToHost|QNetworkAccessManager|Fake across tests/vita_sequence_loss_log_test.cpp, tests/qt_platform_choice_test.cpp, tests/nvidia_afx_pack_log_test.cpp and tests/remote_audio_rx_sequence_restart_test.cpp returns nothing. The two PanadapterStream tests drive processDatagram() directly on a stream that never binds; nvidia_afx_pack_log_test imports a missing archive and never calls install(). No fake radio/amplifier/tuner peer is introduced. Nothing to act on.
6. What I tried to break (and could not)
withoutUrlQueries(). The diff rendering strips backslashes, so I read the literal in the checkout:src/core/NvidiaAfxPack.cpp:336-340is(\bhttps?://[^\s?#]+)[?#]\S*?([)\]}>"',.;:]*)(?=\s|$)replaced with\1\2— correct Qt backreference syntax (not a C++ octal escape) and correct escaping. I walked the lazy\S*?+ closing-punctuation group + lookahead againstError transferring https://h/p?X-Amz-Signature=… - server replied: 403,(https://h/x?sig=1)., a query containing quotes, an end-of-string URL, andHTTPS://: the minimal match always wins and\S*?can never cross whitespace, so the query cannot survive. No nested quantifiers, so no catastrophic backtracking.tests/nvidia_afx_pack_log_test.cpp:76-93covers these cases.- Silent failure exits.
grep "emit finished(false"returns exactly one hit, insidefail(). I also checkedfail()keeps staging (:378-380) so the new docs claim "components that finished downloading are kept, so the download resumes" is true, and that routinginstall()'s no-GPU early return throughfail()cannot mis-clear state (m_busywas never set true;m_tmpFile/m_dlFileare empty). - Lock ordering and reentrancy on the VITA path.
restartStreamSequence()takes onlym_statsMutex; the threeunregister*()hunks deliberately close them_streamMutexblock before calling it, andunregisterDaxStream()places the call after its locked block (PanadapterStream.cpp:1638). The one nested acquisition,clearRegisteredStreams()→restartAllStreamSequences(), ism_streamMutex → m_statsMutex, which already existed on that exact path viaresetAudioStreamStats()(:1422). No site takesm_statsMutexthenm_streamMutex. The log calls are made after the lock is released, as the body claims. - Counter arithmetic in the hot path.
startTotalCount = totalCount - 1is set after the increment, so "packets received" is this instance's count;stats.errorCount++precedeslossStreamErrors = errorCount - startErrorCount, so the triggering error is included.m_seqLossClockcan only be read after the first-packet branch started it (an error requireslastSeq >= 0, which requires a prior packet, which setsstarted). CumulativeerrorCount/totalCountare untouched, so the status bar and Network Diagnostics totals are unaffected. - Reused stream ids, the regression the middle commits target:
restartStreamSequence()setslastSeq = -1andstarted = false, so a re-created id's first packet cannot be measured against the old instance's count. The rate limit survives the restart (start()keepsm_lastReportMs), so a stream flapping in a loop cannot out-log the interval —vita_sequence_loss_log_test.cpp:108-118asserts exactly that. fellBack()false positives, the thing most likely to put a spurious warning in every Wayland user's log:waylandvswayland-egl(prefix match either way),xcb:arg(firstRequestedsplits on:),wayland;xcblanding onwayland, an empty request, andsource == Unset(suppressed entirely). The documented miss (minimalegl;minimal→minimal) under-reports rather than over-reports.requestPicksWayland()as a refactor. Compared against the deletedwillUseWayland()body: same first-entry-of-;-list rule, samewayland/xcbsubstring tests, same session fallback. Behaviour-preserving, and the-platformprecedence it adds matches whatmain.cpp:294-296reads beforeqputenv(sog_qpaPlatformUserSetis genuinely the user's environment, not ours).- Ordering in
main.cpp.QGuiApplication::platformName()is called at:655, well afterQApplication app(argc, argv)at:391, so it is never the empty string;logLines()always returns at least one element, solines.value(0)is safe. - Every
sourcecitation inrules.json. I checked all fourteen new and correctedfile:line/textpairs against the checkout (QtPlatformChoice.cpp:57/74/77,VitaSequenceLossReport.h:125,NvidiaAfxPack.cpp:76/81/388/716/723/775/814/825/835/856, and the four correctedPanadapterStream/RadioModel/main.cppline bumps): all match. The Stable anchor#the-download-failsthe one non-/next/rule points at exists atdocs/user/versioned_docs/version-stable/bnr-gpu-noise-removal.md:92. CI runstest_log_rules.mjsin bothstatic-checks.yml:248anddocs.yml:85and both are green at this head. - The rewritten privacy note (
LogAnalyzer/index.js:134-139), claim by claim againstsrc/core/AsyncLogWriter.cppandsrc/core/LogRedactionPolicy.h: IPv4/MAC keep the last part (:220,:236), serial keeps the last group (:247), IPv6 both forms, home directory →~, bare email, and host/name/location/grid only behind their keywords. "Tokens and keys keep their first four characters" iskeepPrefixChars = 4on the opaque-identifier rows (LogRedactionPolicy.h:28-31) while passwords/names/grids are0. "Your callsign is kept on purpose" holds — there is no callsign rule. The note is accurate. - Cross-firing of the new rules. The
bnr-download-failedregex does not match the install-failed or checksum samples and vice versa; none of the five matchesclean.log(check 6 of the rule test, green).
Could not check: whether Qt also accepts -platform=xcb. QtPlatformChoice::platformArgument() claims to mirror QGuiApplicationPrivate's loop (one leading - of -- dropped, next argument consumed, last occurrence winning) and the implementation does exactly that, but Qt's sources aren't in this tree, so if Qt supports an = form the parser would miss it — cosmetic either way, it would only mislabel who set the platform.
7. CodeGuard findings
Nearly the whole flagged list is noise for this PR: AudioEngine.cpp, ThemeManager.*, MainWindow*, WdspChannel.*, tools/flex-tailnet-shim/*, .github/workflows/*, docs/user/screens.json, .claude/skills/tag-release/* are not in this diff — they arrived on main from #6289/#6309/#6325/#6298. Of the 509 hits, only src/core/NvidiaAfxPack.cpp is a file this PR touches. The api:input_read: QNetworkReply lead is refutable from the diff: errorString() was already read on those paths; the PR only captures it into a local and routes it through withoutUrlQueries(). The CMDI/SSRF/PII hits on docs JSON, log fixtures and workflow YAML are scanner artefacts on non-executable data.
The one surviving finding is Blocker 1 above. The security audit separately cleared: no secrets in the diff (kWinTensorrtSha and the pack SHAs are integrity pins), all URLs https:// with SHA-256 and a redirect policy set on every request, QProcess used in argument-list form with no shell, no PII in the six log fixtures, and the VITA line carrying only a switch-selected category literal plus integers (so a spoofed stream id can influence only the hex number, and the 8-lines-per-10 s global budget bounds a flood). Its one extra note — that could not install into %1 (:835) now logs an app-data path — is handled: redactPii() rewrites /home/<user>, /Users/<user> and C:\Users\<user> roots to ~ before the line is written.
8. Recommendation
Approve with nits, once Blocker 1 is addressed — it is one helper function and two call sites, and the PR already contains the pattern to copy. Everything else here is unusually careful work: the category-visibility trap was checked rather than assumed, the triage's three bypass exits are all closed and provably so, the sequence limiter has a real spec test, and the eight commits read as genuine responses to review rather than churn. The one scope row worth a sentence in the PR body is snapshot_stable.py/release-prep — it is a release-process change that no part of #6285 asks for, and the release-prep owner should know it is there. Thanks for the thorough docs half; the five samples and the anchor checks are what make the rules maintainable.
🤖 aethersdr-agent · cost: $13.9237 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (518 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: $14.1003 · model: claude-opus-5 |
… and re-take the Stable snapshot. Principle VIII. (#6396) ## Summary This brings the user guide (docs.aethersdr.com) up to date with everything in v26.10.2 and re-takes the Stable snapshot so the default site shows it. Docs only, apart from the TCI page prose in `tools/docs/gen_reference.py`; no application source, test or CI changes. Every page was audited against the 106 PRs in the release. Every fact that was added or kept was checked against the source at `f753dbfc4`, the v26.10.2 release commit, not taken from the PR bodies alone. ## What changed - **Menu paths.** - AetherModem, Callsign Lookup, AetherMap and FreeDV Reporter are on **Tools**. They are created on View and then moved to Tools (`MainWindow_Menus.cpp:1519-1520`, `:1569-1575`); the style guide's example says **Tools → AetherMap...**. - **Show live CW contacts** is on **View**. - Waveforms is on **Tools**, not File. - The VUDU applet button was called TXDSP. - Labels now match the source's `...` vs `…`. - **Gaps from this cycle.** - **Windows and Network Diagnostics:** Windows Firewall rules and the Network Diagnostics Windows Firewall page (#6289); the `aether.network` and `aether.nvafx` log categories (#6289, #6308). - **Remote access:** Tailscale shim revocation semantics (#6325). - **Sleep, App Nap and power throttling (#6195):** per-OS notes. - **Hermes-Lite 2:** state restore, RX audio at unity, CW at TUNE level, multi-interface discovery and a discovery troubleshooting entry (#6259, #6254, #5975, #6009, #6293). - **ANAN-G2:** AGC-T range, a "What is remembered" section, and supply voltage and link status (#6282, #6129, #6137, #6211). - **NB2** on the HL2 and ANAN-G2 (#5966). - **DVK:** WAV import requirements, the slot menu without Delete, status text and licensing (#6247, #6262, #6251). - **Copy Assist** from unprocessed audio (#6216). - **Live CW contacts** replace the decoder's contact card (#6268). - **Status-bar indicators** work from the keyboard (#6267). - **Volume:** controllers, hotkeys and TCI `volume` follow the active output (#6129, #6260). - **Peripherals:** the KPA1500 (#5905), CTR2 link extensions and the AetherKnob (#6338), TMate 2 udev (#6250), MIDI RADE Modem and FDV modes (#6031, #6038). - **RTL-SDR:** live PPM (#6082). - **Icom:** COMP and the MK2 squelch scale (#6174, #6184). - **Known issues.** Entries whose GitHub issues are closed are removed (each checked with `gh issue view`). As decided, this includes three closed today without a demonstrated fix: #5107, #4824 and #5665. - **US spelling.** `docs-style-guide.md` asked for British spelling; it now asks for US, by the maintainer's decision, and every page is converted. The generated TCI pages are converted at their source, the prose strings in `tools/docs/gen_reference.py`. Code spans, links and URLs were left alone. UI labels were checked against the source strings, and none of them are British. Two in-page anchors and two Log Analyzer rule anchors (`#bnr-is-grayed-out-…`) follow the renamed headings. - **Stable snapshot.** `snapshot_stable.py 26.10.2` was re-run on the release tree with these corrections, before `main` was merged in, so Stable carries no post-release docs. `docs/` also has `main`'s post-release pages, in US spelling. Stable alone keeps the IC-9700 waterfall-rate entry (#5996): #6402 fixed it after the 26.10.2 tag, so 26.10.2 still has it. The PDF manual already attached to the v26.10.2 release keeps the tag's docs. ## Validation - `npm ci && npm run build` in `docs/user`: success. This is the broken-link and broken-anchor check. - `node tools/docs/test_log_rules.mjs`: 707 checks passed for 45 rules. - `python3 tools/docs/gen_reference.py --check`: up to date (8 pages). - `git diff --check`: clean. - Menu paths on a build of this branch (offscreen, isolated settings, no radio), `menu list`: `Tools → AetherModem...`, `Tools → Callsign Lookup...`, `Tools → AetherMap...`, `Tools → FreeDV Reporter...`, `Tools → Waveforms...`, `View → Show live CW contacts`. ## Screenshots now out of date (not recaptured) These need a recapture pass on the radio: - **Old panadapter title strip (#6386):** `panadapter`, `main-window`, `first-session-*`, `multi-slice-two-slices`, `split-pair`, `tnf-markers`, `offscreen-vfo-indicator`, `pan-3d-stacked-trace`, `default-light-theme`. - **Pre-canon windows (#6334, #6339, #6341, #6372):** `aetherrx*`, `aethertx*`, `aethermodem-aprs`, `bnr-panel`, `connect-dialog`, `connect-by-ip-card`, `connect-icom-rows`, `connect-smartlink-card`, `rx-agc-calibration`. - **Other:** - `title-bar` (lowercase tab states, #6281); - `menu-help` (#6294); - `display-panel` (#6371); - `support-diagnostics-dialog`; - `waveforms-dialog` and `tailnet-remote-access` (show shim 0.4.0); - `about-window` (v26.10.1); - `radio-setup-qrz`. ## Found during the audit, not fixed here - **App string bug:** `TailnetShimDialog.cpp:628` tells the user to "Connect remotely with Connect → Manual"; the real path is **File → Connect to Radio... → Connect by IP**. - **ANAN AGC-T tooltips:** the RX applet and VFO tooltips show the slider position as dB, which is wrong on ANAN. #6282 lists this as a follow-up. - **MIDI Master Volume on non-Flex radios:** it is refused while PC Audio is off, whereas hotkeys, FlexControl and HID drive the line out. The docs describe the code as it is. - **RTL mode names:** `rtl-sdr.md` writes "FM-N" and "CW-R", but the source strings are `FMN` and `CWR`. Left as is until someone confirms what the mode menu displays. - **IC-705 COMP readback:** inferred from the MK2 (#6174) and not verified on an IC-705. - **Not yet documented:** the #6230 Radio Health row "Forward responses skipped", and the Store/MSIX package. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Refs #6285 (items 1, 2 and 4). Item 3, Hermes-Lite 2 discovery, is #6293 by @on8st and is not touched here, so #6285 stays open until that lands; the analyzer rules for #6293's lines follow once its strings are on
main(the rule test checks every cited line exists).Why
The docs Log Analyzer can only flag a failure the app writes to the log. Three user-facing failures were never logged, so a support bundle could not show them either.
What it logs
aether.vita49, WarningPanadapterStream: VITA-49 sequence errors on<audio|FFT|waterfall|meter|DAX> stream 0x<8-digit id>: N in the last S s, E since the stream started (P packets received)remote_audio_rx) starts over: its next packet is not compared with the old sequence count, andE,Pand the window restart; the 10 s limit still applies across the restart. All streams together are capped at 8 lines per 10 s; a line the cap refuses keeps its errors for that stream's next line, and the next line logged is preceded byPanadapterStream: N VITA-49 sequence error(s) held back: more than 8 line(s) in 10 s across all streams.aether.nvafx, WarningNvidiaAfxPack: install failed:<the message the BNR panel shows>fail("cancelled")(during unpacking) logsNvidiaAfxPack: install cancelledat Info; cancelling an active transfer logs nothing, becausecancel()disconnects the reply before aborting it.aether.nvafx, WarningNvidiaAfxPack: transfer failed:<component>: <Qt error>orNvidiaAfxPack: transfer timed out (stall limit 30 s):<component>default, InfoPlatform: Qt platform plugin"<name>" (QT_QPA_PLATFORM=<value>, set by the user | set by AetherSDR),"<name>" (QT_QPA_PLATFORM unset, Qt default), or"<name>" (-platform <value>, on the command line)default, WarningPlatform: Qt fell back from"<first>" to "<used>"; the first plugin in <QT_QPA_PLATFORM | -platform> did not load-platform(or--platform) argument precedence overQT_QPA_PLATFORM, so when there is one, it is the request compared.Behaviour changes beside the log lines
-platformdrives AetherSDR's own platform logic. A-platform/--platformargument counts like a user-setQT_QPA_PLATFORM: AetherSDR no longer imposeswayland;xcbin a Wayland session over it, andGpuSelector's GLX/EGL (PRIME) choice reads the request Qt will actually use (GpuSelector::requestPicksWayland).RadioModelrestarts the sequence tracking of every stream the radio reports removed, in one place, which coversremote_audio_rx(it has no registration inPanadapterStream). Nothing restarts on a status for a stream still flowing, so a real gap is never hidden. A reused id no longer adds a false error to the status-bar and Network Diagnostics counts either.tools/docs/snapshot_stable.pyrewrites each Log Analyzer rule link/next/<page>to/<page>when the new Stable snapshot has that page.Design notes
qCWarning(lcVita49), notaether.protocol. The triage's correction applies:aether.vita49is declaredQtWarningMsg, so a Warning reaches a default log with noLogManagerchange, and packet loss is a warning. The limiter (VitaSequenceLossReport.h) lives inStreamStatsand runs only on the error path; a sharedVitaSequenceLossBudgetbounds all streams together, because the per-stream limiter lets each new id's first error through at once and the VITA socket does not filter by sender: a clean stream pays one clock read when it is first seen. Its values are read underm_statsMutex; the line is formatted and logged after the lock is released. Teardown (clearRegisteredStreams()on disconnect,unregister{Pan,Wf,Dax,Iq}Stream(), andRadioModelfor every stream the radio reports removed) restarts the stream's sequence tracking, because a reconnect can reuse a stream id; the cumulative per-stream totals behind the status bar and Network Diagnostics are kept, so those counters behave as before, except that a reused id no longer adds a false error on its first packet. Errors still held when a stream goes clean are not logged; the live counters (Network Diagnostics) keep them. A sequence error is any gap in the 4-bit count, so reordered packets count too (Remote RX audio dropouts: out-of-order VITA-49 packets counted as drops and concealed instead of reordered in the jitter buffer #6051); the docs say "lost or arrive out of order".lcNvAfxhad no header declaration; it is now declared inNvidiaAfxFilter.h.aether.nvafxgets aLogManagerregistry row ("BNR (NVIDIA AFX)") so it can be toggled in the Support dialog; its Warning lines reach a default log either way, since only Debug is filtered. The three exits that calledemit finished(false, …)directly (no GPU, install verification, offline archive not found) now go throughfail(), which only adds the cleanup it already does and the log line. Redaction:withoutUrlQueries()strips URL query strings and fragments (keeping closing punctuation after them) from logged text, because a redirected download URL can carry a signed query.fail()strips the message it emits as well as the one it logs, so the BNR panel'sFailed:text is redacted too. A failed download logs two lines on purpose:transfer failed:per network transfer (the same line as a failed lookup, whose panel message has no reason) andinstall failed:for the outcome. The manifest and PyPI URLs are source constants with no tokens.could not install into <path>is under the home directory, which the log redactor already collapses to~, andNvidiaAfxFilteralready logs the pack path.addr=(VITA bind) andfrom=(first packet) lines carry no host keyword, but the IPv4/IPv6 shape rules inredactPii()mask them (IPv4 keeps its last octet, IPv6 is replaced, perdocs/log-redaction.md); three negative-corpus rows inasync_log_writer_testpin that. The analyzer page's privacy note now lists exactly what is masked.main()reads the-platformargument the wayQGuiApplicationdoes (one-of--dropped, the next argument, the last one winning) beforeQApplicationconsumes it. The line lives in a small app-shell helper,src/QtPlatformChoice.{h,cpp}, not insrc/core/, somain.cppgains no engine include (the touchpoint manifest stays unchanged). A plugin that fails to load entirely aborts inside theQApplicationconstructor, before any log handler exists; that can't be logged in-process, and the docs entry says so.Docs
vita-sequence-errors,bnr-download-failed,bnr-checksum-mismatch,bnr-install-failed,qt-platform-fallback. There is also a Qt platform plugin row in the analyzer's System summary (asummaryentry plusSUMMARY_FIELDS). The plain platform line is inclean.log, andtest_log_rules.mjschecks that the summary readsxcb./next/…:bnr-download-failedlinks the Stable anchor/bnr-gpu-noise-removal#the-download-fails.snapshot_stable.pymoves/next/rule links to Stable at each snapshot;docs/user/README.mdand the/release-prepsnapshot step describe it. Tested on a scratch copy of the tree: the 4/next/links became Stable links,test_log_rules.mjspassed against the new snapshot, a second run changed nothing, and a link whose page is not in the snapshot stays/next/.test_log_rules.mjs --updatealso refreshed four existing rules whose cited lines moved inPanadapterStream.cppandmain.cpp.Proof
Agent automation bridge: an offscreen build, a fresh settings profile, and a private network namespace (
unshare -n, ascapture_screenshots.pydoes), so there is no radio and no internet.888ac58bf)18a772b75)QT_QPA_PLATFORM=offscreenINF default: Platform: Qt platform plugin "offscreen" (QT_QPA_PLATFORM=offscreen, set by the user)QT_QPA_PLATFORMINF default: Platform: Qt platform plugin "wayland" (QT_QPA_PLATFORM=wayland;xcb, set by AetherSDR)QT_QPA_PLATFORM='nosuchplugin;offscreen'WRN default: Platform: Qt fell back from "nosuchplugin" to "offscreen"; the first plugin in QT_QPA_PLATFORM did not load; Qt's ownCould not find the Qt platform plugin "nosuchplugin"went to stderr onlyQT_QPA_PLATFORM=offscreen AetherSDR -platform minimal(an intentional override)INF default: Platform: Qt platform plugin "minimal" (-platform minimal, on the command line), no warning;--platform minimallogs the sameXDG_SESSION_TYPE=wayland,WAYLAND_DISPLAYset, noQT_QPA_PLATFORM) with-platform offscreen/--platform offscreenPlatform: Wayland session … -> QT_QPA_PLATFORM wayland;xcbline (AetherSDR no longer imposes it);INF default: Platform: Qt platform plugin "offscreen" (-platform offscreen, on the command line)AetherSDR -platform 'nosuchplugin;offscreen'INF default: Platform: Qt platform plugin "offscreen" (-platform nosuchplugin;offscreen, on the command line), thenWRN default: Platform: Qt fell back from "nosuchplugin" to "offscreen"; the first plugin in -platform did not loadinvoke "AetherRX..." trigger→invoke dspMethodBtnBNR click→invoke "Download BNR runtime" click→invoke Accept click(license prompt, throwaway profile), thenlog mark/log tailW aether.nvafx: NvidiaAfxPack: transfer failed: AFX runtime: Network unreachable, thenW aether.nvafx: NvidiaAfxPack: install failed: download failed (AFX runtime): Network unreachableanalyze.mjs+rules.json) run on those real log filesqrhi-failure,bnr-pack-missing; summary has no platformbnr-download-failed(BNR run) andqt-platform-fallback(fallback run); summaryQt platform plugin: offscreen/wayland(
qrhi-failureis expected underoffscreen, as its rule says.)One thing the bridge showed along the way, not changed here: after the failed download,
dumpTreereads the BNR status label asNot installed.updateBnrStatus()runs straight after theFailed: <msg>text is set and overwrites it, so before this PR the reason for a failed download appeared nowhere.Unit tests (socket-free,
QT_QPA_PLATFORM=offscreen,ctest --no-tests=error): all pass.vita_sequence_loss_log_test(45 checks): the limiter (first error at once, none for 10 s, held count carried, 7 lines in a minute of continuous errors, the limit held across a restart) and the exact text. It also feeds meter packets toPanadapterStream::processDatagram()on a stream that never binds, through a test-access friend likePcmCompatibilityTestAccess, under the product'saether.*.debug=falserule. In-order packets log nothing, one gap logs the exact warning, and 50 more gaps within 10 s add no line. AfterclearRegisteredStreams(), a reused stream id's first packet logs and counts nothing, the cumulative total is kept, and the next real gap logs at once with restarted counts; after each ofunregister{Pan,Wf,Dax,Iq}Stream()a re-created stream counts no error.remote_audio_rx_sequence_restart_test(4): throughRadioModel's status router, a gap across an adoption status still counts, a removedremote_audio_rxid's next packet counts no error, and a later real gap counts.async_log_writer_test: negative-corpus rows for theaddr=/from=VITA lines (IPv4 and IPv6).qt_platform_choice_test(31): list parsing,waylandvswayland-egl, fallback detection, the exact text for each source including-platform, and reading-platform/--platform(last wins, a trailing-platformand-platformthemeignored), andGpuSelector::requestPicksWaylandon the-platformrequest.nvidia_afx_pack_log_test(11): the offline import of a missing archive logsW aether.nvafx: NvidiaAfxPack: install failed: archive not foundwithout changing thefinished()message; it never callsinstall(). URL query and fragment stripping, closing punctuation kept, any scheme case.Mutation checks, each restored and confirmed by sha256 against the saved original:
qCInfo→ the integration checks fail (Info onaether.vita49is dropped by a default log).fellBack()always false → 4 fail.emit finished(…)→ the log check fails.withoutUrlQueries()returns its input unchanged → 2 fail; the old greedy tail → the punctuation check fails.start()resetting the last report again → the 4 restart checks fail.clearRegisteredStreams()without the sequence restart → 3 fail; eachunregister*Stream()without it → its check fails.platformArgument()never returning a value → 3 fail; without the--handling → 1 fails;requestPicksWaylandnot recognisingxcb→ 2 fail.RadioModelandunregisterDaxStream()) → 2 fail; stream id unpadded → 1 fails;fellBack()prefix one-way → 1 fails.Also run:
pcm_compatibility_test,nvidia_afx_filter_test,nr_rate_domain_testandflex_backend_lifecycle_testpass. Every Static checks script passes locally, includinggen_reference.py --check,test_log_rules.mjs(707 checks, 45 rules) andgen_touchpoint_manifest.py --check. The colour and a11y steps have nosrc/gui/files to scan.cd docs/user && npm ci && npm run buildpasses (links and anchors).Build: Linux, Qt 6.12.0, lean review configure (
AETHER_SHARED_CORE=ON,-O2 -g1); targetsAetherSDRplus the tests above.Not verified
PanadapterStreampath is exercised by feeding synthetic packets toprocessDatagram(), not by a lossy network.transfer timed out). I checked with a standalone Qt 6.12 probe that a transfer timeout reportsTimeoutError("Operation timed out"). The app's own stall path was not triggered.Agent automation bridge — gaps found
PanadapterStream. Proposed: a TX-freesim vita drop <stream> <every-N>fault-injection verb (or a pcap replay) on the Flex receive path.log tailstarts at bridge start-up, so thePlatform:lines (written earlier inmain()) are only in the log file. Proposed:log file [grep], which returns lines of the current log file.🤖 Generated with Claude Code