Repository navigation
Conversation
…token, not one exact pixel. Principle VIII. Full Suite on main went red on 37fbdbc (#6413): "Headphone: label: not the unavailable token" in both themes. The color was right; the check asked for at least one pixel of exactly #3a4a5a, and anti-aliased text only reaches the ink exactly where a glyph stem lands on whole pixels. With LCD filtering every channel gets its own coverage. Forcing DejaVu Sans with rgba=rgb and hintfull reproduces CI's pixel counts (460 / 302), and rgba=rgb with hintnone reproduces the failure: 0 exact pixels on the Headphone label. pixelsInkedIn counts pixels whose every channel lies between the background (the image's most common pixel) and the ink, at least 60% of the way on average. A new CONTROL requires the live widget to have fewer such pixels. - 96 configs (6 families x 4 rgba x 4 hintstyles): all pass; the old exact check fails in 59 of them. - Mutation: the :disabled rules repointed at color.canon.muted turn every label, value and Mute red under both the system fonts and the CI-like config. Fixes #6418. 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.
1. Issue fit
Yes, with one divergence worth a maintainer's eye.
#6418 is the full-suite failure Default Dark | Headphone: label: not the unavailable token #3a4a5a at radio_setup_headphone_output_gate_test.cpp:455 — exactly the assertion this PR replaces. The diagnosis holds up: I resolved the token chain myself (color.control.unavailable → {color.gray.600} → #3a4a5a in resources/themes/default-dark.json), and the failing predicate was bit-identity against that value inside a 12 px anti-aliased caption. The body's local reproduction (rgba=rgb + hintnone → 0 exact pixels; CI-matching config → 460/302 vs CI's 460/303) is the right kind of evidence for the claim, and the 96-config font sweep plus the :disabled-repointed-to-color.canon.muted mutation answer the two questions I would otherwise have had to ask.
The divergence: the triage comment on #6418 nominated a different fix as Primary — read the polished disabled palette rather than pixels, matching the sibling test added in the same PR. That sibling exists and does exactly that:
// tests/radio_setup_label_theme_token_test.cpp:69-73
QColor polished(QWidget* w, QPalette::ColorRole role)
{
w->ensurePolished();
return w->palette().color(role);
}…used as QCOMPARE(polished(label, QPalette::WindowText), caption); (line 220). That route is deterministic — no font, no rasterizer, no threshold. The triage filed the tolerant-pixel-count approach as "Secondary, if a render-level claim is wanted", and this PR implements the secondary without saying why. There is a defensible answer (the pixel route proves the colour reached the render, which the palette route does not), and the issue's maintainer-review label means this is a maintainer's call rather than mine — but the body should state the choice rather than leave the reader to find the triage comment. Not a blocker: the triage is an agent comment, not a maintainer ruling, and the delivered check does work.
2. Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
tests/radio_setup_headphone_output_gate_test.cpp |
Adds pixelsInkedIn + 3 includes (QHash, <algorithm>, <cstdlib>); swaps the exact-pixel probe for it; adds an INK log line and a live-vs-dimmed control |
Yes — title and body | In scope |
One file, one signed commit, test-only. CHANGELOG.md untouched (correct — it must not be edited). No product change, no new public surface, no settings key, no capability, no workflow change. pixelsOfColour and differingPixels are both retained and still used, so nothing was deleted; the exact-pixel probe survives as the (exact %4) diagnostic and on the engaged-red border. Everything in the diff is explained by #6418.
Socket-test survey (reporting obligation, not a finding): this PR adds and removes no socket test. The diff introduces only pure in-process QImage arithmetic — no bind, listen, connectToHost, server class or peer process. The file names QHostAddress::LocalHost in a pre-existing fixture that is never dialled. Nothing to disclose.
Security audit — security-audit (CodeGuard) ran over the flagged file list and returned no findings, refuting all 12 categories. Spot-checking the two most severe: CS-VLN-SSRF-HEX at src/core/backends/flex/FlexBackend.cpp:1266 is handle == QLatin1String("0x00000000"), a string compare against the SmartSDR placeholder amplifier handle sentinel — no address, no request; and the 38 CS-VLN-CMDI-BACKTICK hits are Markdown-quoted protocol verbs inside // comments, the known lexical false positive. Note the flagged list is a whole-tree scan of main's tip — 17 of the 18 files have no changed line in this PR.
3. Blockers
None.
4. Nits (all non-blocking)
-
tests/radio_setup_headphone_output_gate_test.cpp:434-443— in Default Light the dimmed Mute's border alone can satisfyinkedDimmed > 0, so for that part the check may no longer require the text to carry the token. The disabled rule paints a border, not just text:/* RadioSetupDialog.cpp:1019-1025 */ QPushButton:disabled, ... { color: {{color.control.unavailable}}; border-color: {{color.canon.line}}; background: transparent; }In Default Light
color.canon.line=#c0c8d0(192,200,208) andcolor.control.unavailable=#b0bcc8(176,188,200). Against acolor.canon.nestedsurface#dde5ed(221,229,237) the border's per-channel coverage toward the ink is (0.644, 0.707, 0.784) → mean 0.712, and every channel is inside[lo,hi]. So it is counted, from a different token. That matters because the body's mutation repointed the:disabledcolor:whileborder-colorstayedcanon.line— yet the Mute is reported as failing in both themes, which my arithmetic does not predict for Light. One of us is wrong and it is cheap to settle: please paste theINK … | Line Out: muteandHeadphone: mutelines for Default Light from the mutated run. If the border does carry the count, the honest fix is to narrow the claim (text-only sub-region, or a count floor proportional to glyph area) rather than widen the comment. Dark theme is unaffected — therecolor.canon.lineis#1f78a5d2, 12% alpha, which blends to near-background and falls outside the corridor. Reasoned from the token values and the sheet; I cannot run the test. -
tests/radio_setup_headphone_output_gate_test.cpp:441— the comment overstates what the control proves.// CONTROL: the live widget is not drawn in itreads as "zero by construction", but a thin band of the live text's anti-aliasing fringe is structurally eligible: with darkcanon.controlbg (18,35,58) and live captioncanon.inkSoft(196,212,232), a fringe pixel at coverage t has R = 18+178t, which needs t ≤ 0.24 to stay underhi=60 and t ≥ ~0.15 to clear the 0.6 mean — so t ∈ [0.15, 0.24] counts. Your own figures (357 dimmed / 0 live on the Headphone label) show the margin is in practice total, so this is wording, not a defect. Suggest "the live widget is drawn in it far less" to match whatinkedLive >= inkedDimmedactually asserts. -
tests/radio_setup_headphone_output_gate_test.cpp:239,251,255— three tuned constants (kSlack = 2,> 8,>= 0.6) with no stated derivation. The 96-config sweep is good empirical cover; a one-line note on why 60% rather than 50% or 75% would help the next person who has to move one. I did check the degenerate case and it is safe:weighed == 0would make the function return 0 and fail with the misleading "not the unavailable token", but it needs all three channels within 8 of the token, and against every plausible surface in both themes (canon.nested,canon.control,canon.ground,canon.raised) the smallest per-channel delta is 20. No division-by-zero is reachable — the> 8guard and theweighed > 0re-check cover it. -
PR CI never executes this test, so green here means "it compiles". No workflow has an
-Rentry matchingradio_setup_headphone_output_gate_test;ci.yml's ctest steps are frozen to four digital-voice tests,cross_needle_meter_test,mac_nr_filter_testandasr_gpu_probe_test. All 8 checks on1517bd9are green and that is worth exactly the compile. The next full-suite onmainis the only verification available — which is the same gap that let #6413 ship this flake, so it is worth one line in the body.
5. What I tried to break
- "The guard still fails when the styling is wrong." Walked the mutation the body claims. In Default Dark, repointing
:disabledtocolor.canon.muted(#8598b4= 133,152,180) puts R=133 well pasthi=60, so the glyph body is excluded and bothinkedDimmed <= 0and the new control fire. The mutation is genuinely killed. I also tried the failure mode the body does not cover — the QSS never matching at all, leaving Qt's palette-derived disabled grey (~#7f7f7f): 127 > 60, also excluded, so that regression stays caught too. The check is not self-affirming. - Can the helper be made to return a vacuous 0? Resolved every token alias in both themes by hand and tested
weighed == 0against each candidate surface — minimum delta 20, so no. Division denominators are guarded twice. - Is the retained exact probe now inconsistent?
pixelsOfColour(dimmedEngaged, engagedRed)was left on the engaged-Mute border. That is a solid 1 px axis-aligned run landing on integer pixels, so exact equality is appropriate there and does not need the same treatment — not an oversight. - Sibling call sites.
pixelsOfColourhas no other callers that probe anti-aliased text;grepforcontrol.unavailableshows 6 sheet rules inRadioSetupDialog.cppplusWfmApplet.cpp:111andControlAvailabilityRegistry.cpp:26, none of which are pixel-probed by another test. No parallel copy of this defect left behind. - Could not check: anything at runtime. I have no build and cannot drive the GUI, so every finding above is reasoned from the diff, the canon sheet and the resolved theme values, not reproduced. The one open question (the Light-theme Mute border) needs your
INKlines to settle.
6. Recommendation
Approve with nits, with the triage divergence flagged for the maintainer rather than for you. The diagnosis is correct, the evidence is the strongest kind this repo asks for (font sweep + mutation + a reproduced CI pixel count), the blast radius is one test file, and the delivered check survived every attempt I made to make it pass against broken styling. Concrete next step: post the Default Light Mute INK lines so the border question is closed, and add one line to the body on why the palette route from the #6418 triage was not taken — then this is a merge-and-watch-the-next-full-suite. Thanks for pinning this with real numbers instead of just loosening the threshold.
🤖 aethersdr-agent · cost: $16.8418 · model: claude-opus-5
|
One correction to my review, before you spend time on it — the second nit was framed more strongly than my evidence supports. I wrote that your mutation "repointed the So the open question is narrower than I made it sound, and it is only about the unmutated build: in Default Light, does the dimmed Mute's Same ask as before, just a smaller one: the 🤖 aethersdr-agent · cost: $19.2913 · model: claude-opus-5 |
|
Cisco CodeGuard — static analysis of this PR (328 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: $19.9704 · model: claude-opus-5 |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Request changes — one regression-coverage blocker. PR #6419 addresses #6418's font-dependent exact-pixel failure, but its replacement can pass when the disabled text uses the wrong token. Reviewed head 1517bd978f540f737e1e3fc103968250a1e3ef44. The flake diagnosis is reproducible; the remaining issue is preserving the original color guarantee while fixing it.
| File | Change | Claimed? | Scope verdict |
|---|---|---|---|
tests/radio_setup_headphone_output_gate_test.cpp |
Blend-based ink count, live/dimmed control, diagnostics and required includes | Yes | In scope for #6418 |
Everything in the diff is explained by the issue. One signed commit; no product, workflow, dependency, settings, protocol, user-doc or changelog change. The changed target adds no sockets or synthetic firmware peer; it uses existing in-process model injection and Demo state.
Blocker 1 — the new ink probe accepts substantially wrong text colors (tests/radio_setup_headphone_output_gate_test.cpp:432–443). The predicate only requires one eligible pixel and a count above the live widget's count. Anti-aliasing fringes of an incorrect, darker ink can satisfy both conditions.
I changed only the product's disabled QLabel color rule, conditional on Default Light, from color.control.unavailable to color.canon.muted, leaving Mute styling, borders and every line of the submitted test unchanged. The actual wrong label/value ink is #506070; the expected unavailable ink is #b0bcc8. Both the entire CTest target and the individual render slot still passed. The unchanged test reports:
INK Default Light | Headphone: label: unavailable dimmed 2, live 0 (exact 0)
INK Default Light | Headphone: value: unavailable dimmed 1, live 0 (exact 0)
INK Default Light | Line Out: label: unavailable dimmed 2, live 0 (exact 0)
INK Default Light | Line Out: value: unavailable dimmed 1, live 0 (exact 0)
PASS : RadioSetupHeadphoneOutputGateTest::dimmedRowsLookUnavailableInBothThemes()
100% tests passed, 0 tests failed out of 1
This is a controlled production-style mutation, not an inference about token distances or a modified assertion. The test source remained byte-identical to the submitted head. A correct Dark theme and correct Mute buttons conceal the four wrong Light-theme text colors.
Keep the useful render/repeatability/live/engaged checks, but pin the exact resolved ink through the polished disabled palette. Add #include <QPalette> and, immediately before the inkedDimmed calculation, insert:
p.dimmed->ensurePolished();
const QPalette::ColorRole role = qobject_cast<QPushButton*>(p.dimmed)
? QPalette::ButtonText : QPalette::WindowText;
const QColor ink = p.dimmed->palette().color(QPalette::Disabled, role);
if (ink != QColor(unavailable)) {
failures << where + QStringLiteral(": ink %1 is not unavailable %2")
.arg(ink.name(), unavailable);
}I compiled and tested this replacement: it rejects all four mutated colors, with ink #506070 is not unavailable #b0bcc8, and passes the correct product styling and the font sweep. Add the text-only, Light-only mutation to the verification evidence; changing every disabled rule in both themes is not enough to expose this hole.
Non-blocking nit — tests/radio_setup_headphone_output_gate_test.cpp:441: “the live widget is not drawn in it” overstates the control. In the clean run, Mute has 22 eligible live pixels in Dark and 6 in Light. The comparison correctly permits this; make the comment match it:
// CONTROL: the live widget has fewer pixels counted toward this ink.What was verified: Built only the named test target on Linux/Qt 6.12, with shared core and -g1. The clean submitted head passed CTest and all 96 requested family/rgba/hintstyle configurations. The merge-base assertion, separately compiled against the identical production sources, failed 68/96 configurations, including Inter/rgb/hintnone with the reported “not the unavailable token” failure. Font substitution differs from the author's machine: requested DejaVu Sans resolves to a local fallback, so these are not a claim to reproduce the author's exact six installed families or 59 failures. The proposed palette guard passed the same 96 configurations and caught the isolated product mutation. All temporary product/test edits were restored and the original target passed again.
These were actual QWidget grabs and palette reads in the socket-free QApplication test, including in-process Demo state, disconnect/re-enable and silently checked Mute controls. No full AetherSDR/bridge instance or live hardware was used: this PR changes only the test's image predicate. No transmit action was taken.
The earlier review's Light-theme Mute concern did not reproduce as a false pass in the wrong-text-color experiment: Mute recorded zero eligible pixels and failed. The confirmed blocker is the label/value fringe case above. The security bot's examples concern unchanged files; the cited backticks are comments and 0x00000000 is a handle comparison, not an outbound request. A separately invocable code-review skill was unavailable.
Eight PR checks succeeded, but the relevant pull-request runs were created at 20:31:23 UTC, before the observed main tip 34363a38 at 23:18:11 UTC. With non-strict protection, those results do not verify a merge with current main. The PR CI gate does not execute this particular test; local runtime results above supply that evidence.
Recommendation: Request changes. Preserve the font-independent improvement, add the exact palette guard, and demonstrate rejection of the Light-only label/value mutation before landing.
|
Closed as superseded by #6424 (merged as 2452cb5). It fixes the same #6418 flake by setting Before closing, I ran main's version of the test locally (Linux, Qt 6.12, lean build, product source otherwise at this PR's base):
So main already closes the hole the review found in this PR's blend counter: under the PR head, that same mutation passed with 2/1/2/1 counted pixels. The product source was restored and verified by sha256. Not done here: Full Suite on 2452cb5 was still running at close time. #6418 stays open until that run is green. |
Fixes #6418.
What broke
On 37fbdbc (#6413), Full Suite on
mainfailedradio_setup_headphone_output_gate_testin both themes with "Headphone: label: not the unavailable token". The UI was correct. The check needed at least one pixel of exactlycolor.control.unavailable, and anti-aliased text only reaches the ink exactly where a glyph stem covers whole pixels. With LCD filtering, each channel also gets its own coverage. So whether such a pixel exists depends on the font and the rasterizer, not on the style.Local reproduction:
rgba=rgbandhintfullmatches CI's pixel counts: 460 / 302 against CI's 460 / 303.rgba=rgbwithhintnonereproduces the failure: 0 exact pixels on the Headphone label.Fix (test only)
pixelsInkedIn: counts the pixels drawn toward the ink. Every channel must lie between the background (the image's most common pixel) and the ink, and the pixels must be at least 60% of the way there on average.Evidence
rgba× 4hintstyle). All pass. The old exact check fails in 59 of them.:disabledrules repointed atcolor.canon.muted, every label, value and Mute fails in both themes, under the system fonts and under the CI-like config. The source was restored and its sha256 verified, and the test is green again.No CI or workflow changes.
🤖 Generated with Claude Code