Skip to content

feat(gui): NR2's FFTW Wisdom window is a canon tool window with a brand-gradient progress bar. Principle VIII. - #6776

Merged
ten9876 merged 2 commits into
mainfrom
feat/fft-wisdom-canon
Oct 25, 2026
Merged

ten9876 merged 2 commits into
mainfrom
feat/fft-wisdom-canon

Conversation

@ten9876

@ten9876 ten9876 commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Part of the style guide follow-ups (RFC #6226). The FFTW Wisdom window that NR2 shows the first time it runs moves from an inline QDialog with FramelessWindowTitleBar to a CanonWindow dialog. It is built in MainWindow::enableNr2WithWisdom() today; it becomes its own class, Nr2WisdomDialog (src/gui/Nr2WisdomDialog.{h,cpp}). The maintainer chose how it looks and behaves:

  • Own class. The window owns its labels, progress bar, activity timer and Cancel. MainWindow keeps the worker thread, the shared cancel flag (set by the window's cancelRequested() and by shutdown/disconnect, as before) and the NR2 enable. MainWindow.cpp loses about 200 lines.
  • Still a tool window. Modeless, Qt::Tool (no taskbar entry), WA_ShowWithoutActivating, and WA_MacAlwaysShowToolWindow on macOS, set ahead of the first show, as before. It no longer follows View → Frameless Window: canon windows are always frameless.
  • Title "FFTW Wisdom" in the canon header and for the taskbar and screen readers (was "AetherSDR — FFTW Wisdom").
  • Canon sheet under canon/nr2Wisdom. Canon ink and soft ink for the text, the canon button style, and a progress bar on color.canon.nested whose chunk is color.brand.gradient, like the Updates window's download bar. The hard-coded #050710 ground is gone; the canon ground replaces it.
  • Cancel. Cancel, the corner ✕, Esc and Ctrl+W (⌘W) each request cancellation once, as before. Cancel keeps the window up showing "Canceling…"; the other three also hide it, as QDialog::reject() did.

Small changes beyond the move:

  • Progress reports the planner queued before a cancel are now ignored once the cancel is requested, so a late report can no longer replace the "Canceling…" text or hide the cancel notice.
  • Cancel is disabled while the outcome shows (for the 800 ms before the window closes).
  • The progress bar has an accessible name, "FFTW wisdom generation progress".

Object names (nr2WisdomDialog, nr2WisdomStatusLabel, nr2WisdomProgressBar, …) are unchanged, so the fftw-wisdom-window recipe in screens.json and bridge automation still find it.

Docs:

  • nr2-noise-reduction.md calls it the FFTW Wisdom window and says ✕ and Esc cancel too; the alt text in the page and in screens.json drops "AetherSDR".
  • docs/style/* list it among the canon windows and canon/nr2Wisdom among the scopes. It saves no geometry and installs no resizer (a short-lived window), so it is not in those lists.
  • The touchpoint manifest counts one more core/ThemeManager.h includer (ui-support, no radio state).

Intent

After this merges, NR2's FFTW Wisdom window is a canon tool window that follows the theme, like the other canon windows. Its text, progress, cancel behavior and lifetime are unchanged. Out of scope: the planner, the wisdom file, and the other inline dialogs in MainWindow.

Stale screenshot: docs/user/static/img/screens/fftw-wisdom-window.png still shows the old window. That shot belongs to the screens harness's real-radio on-screen pass, so it is not recaptured here.

Constitution principle honored

Principle VIII (Evidence Over Assertion) covers the checks below. The visual direction is RFC #6226's.

Test plan

  • Local build passes (Linux, Qt 6.12, cmake --build build)
  • New nr2_wisdom_window_test (6 slots, socket-free, no planner) passes offscreen. It covers:
    • a non-modal CanonWindow tool window titled FFTW Wisdom, shown without activating, under canon/nr2Wisdom, with the canon header and every named child;
    • the sheet in Default Dark and Default Light: the progress chunk resolves to the brand gradient, every token resolves, and the old literal is gone;
    • Cancel requests once, keeps the window up and shows the cancel state; Escape requests too; reports after a cancel change nothing;
    • planner step text and progress reach the window, and the activity line ticks;
    • each outcome (complete, canceled, failed) sets the bar, activity and status lines, the status line's accessible name, and stops the ticking.
    • Mutations: removing the Qt::Tool flag, WA_ShowWithoutActivating, the gradient, the cancel-once guard, either after-cancel guard, the rejected → cancel wiring, the timer stop or the outcome announcement each fails a check.
    • CI does not run it before merge: it is not in ci.yml's per-PR ctest -R steps, so green checks prove it compiles; it runs in full-suite.yml.
  • canon_window_test passes.
  • Driven through the automation bridge, offscreen, with scratch settings and a scratch HOME (the wisdom file lives under it), never connected to a radio:
    • AetherRX... → NR2 opened the window: canon header, corner ✕, gradient bar, live step text and elapsed time, in Default Dark; switching to Default Light re-themed it live;
    • Cancel showed "Canceling…" with the busy bar and the cancel notice; the window closed when the plan in flight finished, NR2 stayed off, and no wisdom file was written;
    • a second run went to completion in 4:46: "Complete" at 100%, the window closed itself, NR2 turned on, and the wisdom file was written.
  • check_a11y (0 findings), audit_colours, check_theme_seed, check_test_registration, check_qsettings, check_engine_boundary --strict, check_capability_records --strict and check_command_plane --strict pass; gen_touchpoint_manifest.py --check is up to date; embed_screenshots.py --check reports the same pages out of step as on main (not the NR2 page).
  • Not exercised: macOS and Windows (the tool-window attributes are carried over unchanged), and the docs site build (CI's Static checks gate it).

Checklist

  • Commits are signed
  • No new flat-key AppSettings calls (the window saves no geometry)
  • Code is clean-room
  • All meter UI uses MeterSmoother (n/a)
  • Documentation updated: docs/user/docs/nr2-noise-reduction.md, docs/user/screens.json, docs/style/*, docs/architecture/aetherd-touchpoints.md
  • Security-sensitive changes: n/a

🤖 Generated with Claude Code

…nd-gradient progress bar. Principle VIII.

The window NR2 shows while it makes its first-use FFT plans moves out of
MainWindow::enableNr2WithWisdom() into Nr2WisdomDialog, a CanonWindow
dialog under canon/nr2Wisdom titled "FFTW Wisdom". It keeps the tool-window
behavior: modeless, no taskbar entry, shown without taking the focus, and
on macOS still visible when the app deactivates. Cancel, the corner close
button, Escape and Ctrl+W request cancellation once; reports that arrive
after a cancel are ignored. MainWindow keeps the worker thread, the cancel
flag and the NR2 enable.

nr2_wisdom_window_test pins the chrome, the canon sheet in Default Dark
and Default Light, cancel, planner reports and the three outcomes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@ten9876
ten9876 requested review from a team as code owners October 25, 2026 04:21

@ten9876 ten9876 left a comment •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 4e1542b2bb29f057ce8457373807c8948a26057d against actual merge base 4ef64e215ad00b2fe907bce76f834a53c07a627c.

Issue fit: yes. No closing issue is linked; the explicit intent is the RFC #6226 canon migration of NR2's first-use wisdom window, with the planner and wisdom storage out of scope. The diff extracts presentation into Nr2WisdomDialog, keeps worker/cancel/NR2 ownership in MainWindow, and adds the disclosed post-cancel report guards and outcome-button disable. The visual and interaction choices are explicitly attributed to the maintainer and fit the approved per-surface canon migration. The tests and actual running application survived the checks below.

Scope

Files Change / disclosed? Verdict
src/gui/Nr2WisdomDialog.{cpp,h} Own the canon tool window, progress, activity, cancellation and outcome display; yes In scope
src/gui/MainWindow.{cpp,h} Replace inline widgets with the class and route existing worker callbacks; yes In scope
CMakeLists.txt, tests/tests.cmake Build the window and register its isolated test/settings consumer; yes In scope
tests/nr2_wisdom_window_test.cpp Socket-free presentation, cancel, progress and outcome coverage; yes In scope
docs/style/{aethersdr-style-guide,dialog-patterns,theme-style-guide}.md Canon adoption and theme-scope lists; yes In scope
docs/user/docs/nr2-noise-reduction.md, docs/user/screens.json Title, cancellation instructions and alt text; yes In scope; old screenshot explicitly disclosed
docs/architecture/aetherd-touchpoints.md ThemeManager includers 184 -> 185 for the extracted class; yes Justified UI-support growth, no vendor retagging

Everything in the diff is explained by the stated migration. One commit authored October 25 UTC; no planner/storage changes, new radio-facing surface, dependencies, settings keys, frozen test-gate additions, or enforcement-baseline changes. The worker/shutdown/disconnect paths retain the existing ownership and cancellation flow. The stale screenshot is named in the body, satisfying the documented alternative to recapture in this pass.

Blockers

None found.

Non-blocking nits

None.

What I tried to break

  • Built the exact head desktop app, nr2_wisdom_window_test and canon_window_test in an isolated Linux scratch worktree using the lean shared-core review configuration. Both registered CTests passed: 2/2, 2.89 seconds. The new test runs no planner, socket or external process and isolates AppSettings before QApplication. Its target is registered and present in AETHER_SETTINGS_CONSUMERS.

  • Independently removed the post-cancel step-description guard in a temporary source copy and linked it into the same test. cancelRequestsOnceAndShowsTheCancelState then failed at its late-report assertion: the cancel status was overwritten. This checks one of the claimed mutations; I did not independently rerun every listed mutation. Additional socket-free window probes exercised the corner close button and Ctrl+W: each emitted one cancellation, hid the window, and retained cancellation text after late step/progress reports. The delivered tests cover Escape, repeated cancellation, outcome text/accessibility and stopped outcome timers.

  • Drove the real head app through explicit socket /tmp/aethersdr-pr6776, identity pr6776-review, offscreen, with fresh settings/cache and TX disabled. A review-only Qt home-path shim routed the real planner's wisdom file to /tmp/pr6776-runtime/home; it changed the file location, not the planner or window logic. Discovery was inspected and connection was explicitly limited to DEMO-0001; get radio confirmed AetherSDR Demo. Opened AetherRX... -> NR2, observed live step/progress/elapsed updates, switched Dark -> Light while open, and inspected both captures plus the cancel capture. No clipped labels or broken theme state appeared.

  • Exercised both real worker outcomes. Cancel left the visible window showing its notice with Cancel disabled; after the in-flight plan finished, the window closed, NR2 stayed off and the scratch directory contained no wisdom file. A second run completed normally in 318.8 seconds: Wisdom generation complete!, progress 100, automatic closure, NR2 checked, and a 31,035-byte wisdom file under the scratch path. Selected bridge evidence:

    {"cancel":{"windowRemainedVisible":true,"cancelButtonEnabled":false,
               "noticeVisible":true,"eventuallyClosed":true,"nr2Checked":false,
               "wisdomFiles":[]},
     "complete":{"elapsedSeconds":318.8,"progress":100,"closed":true,
                 "nr2Checked":true,"wisdomBytes":31035}}

    Confirmed the instance was still on DEMO-0001 with TX inactive, disconnected, and closed it; the process exited successfully. No live radio or transmit action was used. No base GUI was launched; the added report guard was checked through the mutation above, and the existing worker path was read against the removed implementation. Native Windows/macOS focus/tool-window behavior and assistive-technology announcements were not exercised locally.

  • All four engine gates pass on actual merge base and head. Their per-file findings are unchanged after line-number normalization; source counts grow by the two new GUI files. Manifest checks validate the disclosed ThemeManager includer change; capability/command-plane output is unchanged. Accessibility scanning of the four touched GUI files reports zero findings; theme seed, test registration, QSettings ratchet and generated reference checks pass. The dedicated automated code-review skill was unavailable; this was a manual audit with executed tests and bridge runs.

CI freshness: all reported non-skipped checks now pass, including Linux, Windows, macOS, Static checks and the docs-site build. The new window test is not executed by the frozen per-PR test gate; its runtime result above is local. Pull-request CI/Static/Docs runs were created at 04:21:24 UTC. Current main remains eef08ff37237ee30728a4fb5d0248cc3bfe97dfe from 04:07:11 UTC, so there is no newer-main gap. Local verification used the branch head; CI built its merge with main.

Recommendation: Approve. The intended migration, late-report protection, close routes, real cancellation and real successful completion are verified. Native platform behavior and refreshing the disclosed screenshot remain the stated validation limits. Posted as COMMENT under the review workflow; approval remains the operator's call.

Recheck of the unchanged head

The repeated review request still names 4e1542b2bb29f057ce8457373807c8948a26057d. No new blocker or nit was found, so this existing review is refreshed rather than posting a duplicate.

Fresh verification rebuilt the exact head's window, CanonWindow, ThemeManager and theme-seed sources/resources from a private snapshot, with its original test sources. The narrow harness links the existing review core for AppSettings/logging; the relevant core support sources were checked unchanged. The NR2 test passes all eight QtTest checks (six slots plus setup/cleanup), and the CanonWindow test passes. Removing the post-cancel step-description guard in this private build makes the late-report assertion fail; restoring it returns all eight checks to green. Corner-close and Ctrl+W probes each request cancellation once, hide the window, and preserve the cancel text after late reports.

The four architecture gates were rerun against private snapshots of the actual merge base and head and pass with unchanged per-file findings; the disclosed ThemeManager includer growth remains UI-support only. The recorded Dark/Light captures above were re-inspected. The full-app demo cancellation and 318.8-second successful planner run documented above are retained evidence from the earlier pass on this exact head, not claimed as newly repeated. Native Windows/macOS runtime behavior and the disclosed screenshot refresh remain unverified.

Current main is still eef08ff3 (04:07:11 UTC), before this head's CI creation. Linux, Windows, macOS, Static checks and the docs-site build pass. The private sources were restored and hash-checked against the head after mutation; no invoked-checkout changes or new full-app/planner session were made. Recommendation remains Approve, with no findings.

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue fit

No Closes #N; the body frames this as an RFC #6226 style-guide follow-up and states "The maintainer chose how it looks and behaves." Reviewed against that stated intent, it delivers it: Nr2WisdomDialog is a non-modal Qt::Tool CanonWindow under canon/nr2Wisdom, MainWindow::enableNr2WithWisdom() drops ~185 lines to worker + cancel-flag + enable, and the three disclosed behaviour deltas (late planner reports dropped after cancel, Cancel disabled during the outcome, progress bar accessible name) are all present in the code. GOVERNANCE.md wants an RFC for architectural change and this rides an approved one (#6226), so no issue was owed.

On the #6239 pick: I did not file this as needs-maintainer-decision. @ten9876 is the Project Maintainer (GOVERNANCE.md:82, "Final authority on direction"), so a PR of his that adds "FFTW Wisdom" to the canon lists is the pick — unlike the sibling w5jwp conversions (#6749/#6751/#6753/#6757/#6759/#6762), which each carry a "don't merge before the pick" notice precisely because they can't make it themselves.

Scope

File / group What it changes Claimed? Verdict
src/gui/Nr2WisdomDialog.{h,cpp} New canon dialog class (the move) Yes In scope
src/gui/MainWindow.{cpp,h} Inline dialog deleted; include, QPointer type, two dropped includes Yes In scope
CMakeLists.txt Nr2WisdomDialog.cpp → GUI_SOURCES Implied In scope
tests/nr2_wisdom_window_test.cpp, tests/tests.cmake New socket-free test + registration + AETHER_SETTINGS_CONSUMERS Yes In scope
docs/style/{aethersdr-style-guide,dialog-patterns,theme-style-guide}.md Window + canon/nr2Wisdom scope added to the three lists Yes In scope
docs/user/docs/nr2-noise-reduction.md, docs/user/screens.json Title drops "AetherSDR —"; alt text follows; ✕/Esc documented Yes In scope
docs/architecture/aetherd-touchpoints.md core/ThemeManager.h 184 → 185 Yes In scope — regenerated, matches the one new includer

Everything in the diff is explained by the stated intent. No CHANGELOG.md entry (correct — it's a release-prep file). No new settings key, protocol verb, capability field, or public surface. Every object name is preserved (nr2WisdomDialog, nr2WisdomStatusLabel, nr2WisdomProgressBar, nr2WisdomActivityLabel, nr2WisdomElapsedTimeLabel, nr2WisdomReassuranceLabel, nr2WisdomCancelButton), so the fftw-wisdom-window recipe in screens.json still resolves — I checked it, and it targets nr2WisdomDialog by objectName, not a QDialog/... class path, so it does not hit the breakage #6762 had to fix for Net Scheduler. Deleted - lines: the two removed includes (QAccessible, QElapsedTimer) both moved into the new TU; no guard, early return, or symptom-naming comment was dropped.

Blockers

None.

Nits (all non-blocking)

1. tests/nr2_wisdom_window_test.cpp:100 — this assertion cannot fail, and both the test and the body claim it can.

            QVERIFY2(!sheet.contains(QLatin1String("{{")), "every token resolves");

ThemeManager::resolveFor() (src/core/ThemeManager.cpp:1563-1577) replaces every {{...}} match unconditionally, and cssFragment() returns an empty QString for an unknown token (ThemeManager.cpp:1428-1437, with a qCWarning). So a sheet full of typos ({{color.acent}}, {{color.canon.inkSft}}) resolves to color: ; declarations that Qt silently drops — and still contains no {{. The label "every token resolves" is structurally unfalsifiable, and the test plan repeats the claim. The sibling assertions are fine: the qlineargradient check at :104 genuinely proves color.brand.gradient resolved, and !sheet.contains("#050710") is real. A drop-in that does fail on a typo:

            QVERIFY2(!sheet.contains(QLatin1String(": ;")), "no token resolved to an empty value");

Stronger still (and the established seam — tests/theme_manager_test.cpp:1398-1453 already asserts resolved values this way): assert the expected hex is present for one or two tokens per theme.

For the record, I did verify by hand that all ten tokens the sheet references exist in both bundled themes: canon.{ink,inkSoft,nested,line,lineHi,control,cyan,aqua,muted} and color.brand.gradient (a {type: linear-gradient} object in each, which ThemeManager.cpp:204 emits as qlineargradient(...)). So the sheet is correct today; it's the guard that wouldn't catch it breaking.

2. tests/nr2_wisdom_window_test.cpp:170-171 — timing-coupled, will flake on a loaded runner.

        QTRY_VERIFY_WITH_TIMEOUT(activity->text().startsWith(QLatin1String("Working.")), 2000);
        QCOMPARE(elapsed->text(), QStringLiteral("Elapsed: 0:00"));

dotCount = (elapsedMs / 500) % 4, so "Working." (one dot) first appears at the ~500 ms tick and the exact-match Elapsed: 0:00 only holds below 1000 ms. If the event loop stalls past a second — plausible in full-suite.yml under parallel ctest — the first tick that satisfies the QTRY predicate is "Working.." at 1000 ms+, where the label reads Elapsed: 0:01 and the QCOMPARE fails. Assert the format instead of the value:

        QTRY_VERIFY_WITH_TIMEOUT(activity->text().startsWith(QLatin1String("Working.")), 2000);
        QVERIFY2(QRegularExpression(QStringLiteral("^Elapsed: \\d+:[0-5]\\d$"))
                     .match(elapsed->text()).hasMatch(),
                 qPrintable(elapsed->text()));

(needs #include <QRegularExpression>). This still fails if the elapsed label stops updating or loses its format, which is what the case is for.

3. tests/nr2_wisdom_window_test.cpp:138-142 — the comment reads as the opposite of the assertion under it.

        // A report counts as a sign of life and hides the reassurance line,
        // which here carries the cancel notice.
        QVERIFY(!reassurance->isHidden());

The behaviour is right — setStepDescription()/setProgress() return before noteActivity() once m_cancelRequested is set — but the comment describes the hazard rather than the outcome, so it looks like a stale comment left over from the pre-guard version. Suggest: "A report would normally hide the reassurance line; after a cancel the guard drops the report, so the cancel notice stays."

4. docs/style/dialog-patterns.md:274-276 — the resizer sentence now has a second silent exception.

A tool window on it also
installs `FramelessResizer`, since `CanonWindow` itself only moves.

The PR body gives the reason ("It saves no geometry and installs no resizer (a short-lived window), so it is not in those lists"), and the window is genuinely transient, so I'm not asking for the resizer — but that reasoning lives only in the PR body, while the doc still states the rule flatly. #6749 was asked for exactly this and recorded its fixed-size exception. Worth one clause here ("a short-lived window on it needs neither"), especially since this PR does add the window to the sentence's own paragraph two lines down. Minor and related: line 277's "tool windows the maintainer has moved to it" parenthetical doesn't gain FFTW Wisdom the way aethersdr-style-guide.md:118 does — that list already lags (no AGC-T Calibration either), so this is drift, not something the PR introduced.

5. docs/user/docs/nr2-noise-reduction.md:56 — Ctrl+W / ⌘W is missing from the user-facing list.

- **Cancel** (or the ✕ in the corner, or Esc) stops after the FFT plan currently being computed.

CanonWindow binds QKeySequence::Close to reject() (CanonWindow.cpp:78-79), so ⌘W/Ctrl+W cancels too — the PR body and the header comment at Nr2WisdomDialog.h:19 both say all four do, and this is a new way out versus the old inline dialog. Add it to the sentence.

Other tools' findings

security-audit (CodeGuard): no security defect in this PR. I had the audit reconcile the 66 findings against the diff, and every one is pre-existing or co-located rather than introduced:

  • Five of the eight files in the audit list — .github/workflows/codeql.yml, .github/workflows/windows-installer.yml, src/core/UpdateStager.h, src/main.cpp, tests/update_stager_test.cpp — are not in this diff at all. They are main's tip (the updater work in be216ed32/9d1791f44/eef08ff37), attached to this PR only through the merge base.
  • Every MainWindow.cpp line cited (518, 1227-3618, 6229, 6308, 9787-9789) sits outside the changed hunk, which is new-file 9661-9755. The CG-PATH-001 trio at 9787-9789 is untouched code that the ~185-line deletion shifted upward. CS-VLN-PATH-PROC at 6229/6308 is QFile statusFile("/proc/self/status") — a constant path read for the process's own RSS, identical on main.
  • docs/user/screens.json: the PR edits exactly one line (3776, dropping "AetherSDR " from the alt text). Every flagged IP/localhost/email hit (1124, 2543, 2589, 2623, 2720, 2760, 3841, 4183) is untouched, and all are RFC 5737/2606 documentation placeholders inside screenshot alt text.
  • The one PR-added line any rule touches is src/gui/MainWindow.cpp:9680, flagged CS-VLN-CMDI-BACKTICK: // \cancelled`, which the window's cancel and shutdown both set.` That rule is lexical and fires on backticks in C++ comments; there are 28 others in the same file. False positive.
  • Nr2WisdomDialog.{h,cpp} and the new test are not in the flagged list; read directly, they contain no file, process, network, or deserialization call, and the only non-literal string is FFTW's own plan label going into QLabel::setText().

No Copilot or other reviewer comments were attached.

What I tried to break

Findings below are reasoned from reading the code at /tmp/aetherclaude/pr-6776; I have no build, no test run, and no way to drive the GUI, so nothing here was reproduced at runtime.

  • Use-after-free on Escape. This was my main hypothesis: the thread-finished lambda captures dlg as a raw pointer (MainWindow.cpp:9716) and calls dlg->showResult(...), so if Escape destroyed the window the worker's completion would write to freed memory. It doesn't. CanonWindow sets WA_DeleteOnClose nowhere, MainWindow doesn't set it either, and Escape/⌘W go through QDialog::reject(), which hides (CanonWindow.cpp:126-135 says so explicitly). The window survives hidden, showResult() lands on it, and the 800 ms singleShot does accept() + deleteLater(). The QPointer<Nr2WisdomDialog> m_nr2WisdomDialog single-session guard also still re-show()s the hidden window, so the doc's "clicking NR2 again brings the same window forward" holds after a cancel.
  • Cancel losing the race, or firing twice. requestCancel() guards on m_cancelRequested first, so cancelRequested() is emitted at most once; the receiver is this (MainWindow) on the same thread, so the connection is Direct and cancelled->store(true) happens synchronously inside the click — no window where the worker misses it. Shutdown's independent path (m_nr2WisdomCancel, #6287) sets the same atomic and is untouched. m_cancelRequested is GUI-thread-only; the worker reads only the shared_ptr<atomic_bool>.
  • "Appears without taking the focus." I read CanonWindow::showEvent() in full looking for an activateWindow()/setFocus() that would defeat WA_ShowWithoutActivating. It only does restoreGeometryFromSettings(), resize() and move() — the claim holds.
  • The Qt::Tool flag composition. The base ctor sets Qt::Dialog | Qt::FramelessWindowHint, then the subclass ORs in Qt::Tool. Since Qt::Tool == Popup|Dialog, windowType() resolves to Qt::Tool (so the test at :61-62 is checking something real) and FramelessWindowHint survives. WA_MacAlwaysShowToolWindow is set in the constructor, correctly ahead of the first show()/winId().
  • Theme-scope ordering. theme::setContainer(this, "canon/nr2Wisdom") at Nr2WisdomDialog.cpp:74 precedes applyCanonSheet(bodyWidget(), ...) at :90. Reversed, the sheet would resolve at root and the canon re-palette would silently vanish. Also confirmed the theme files declare only the canon parent scope with no per-window children, so canon/nr2Wisdom resolving by fallback is exactly how the other 13 canon/* containers work — not a missing token definition.
  • Link completeness of the new test target. nr2_wisdom_window_test compiles only Nr2WisdomDialog.cpp + CanonWindow.cpp; applyCanonSheet is inline in CanonIndicators.h:150 and makeCanonHeader lives in CanonWindow.cpp, and CanonWindow.cpp references neither FramelessResizer nor CanonIndicators — so omitting FramelessResizer.cpp (which the sibling multiflex_dashboard_window_test does include) is correct here, not an oversight.
  • The body's factual claims. "Like the Updates window's download bar" checks out byte-for-byte: UpdatesWindow.cpp:200 is QProgressBar#firmwareProgress::chunk { background: {{color.brand.gradient}}; border-radius: 3px; }. The CI disclosure is accurate — I grepped ci.yml and nr2_wisdom_window_test is genuinely absent from every per-PR ctest -R step, so the 14 green checks here prove it compiles, not that it passes. The touchpoint 184 → 185 bump matches the one new core/ThemeManager.h includer. I spot-checked eight of the nine claimed mutations against the assertions and each would in fact fail the named case.
  • Mutation-resistance of the outcome case. outcomesSetTheWindow ends with qWait(600) and re-compares the activity text, so deleting m_activityTimer->stop() from showResult() fails it — that one isn't self-confirming.
  • Could not check: macOS/Windows behaviour of the tool-window attributes (carried over unchanged, as the body says), and whether the stale fftw-wisdom-window.png recapture matters for the docs gate — the body discloses the screenshot as stale and embed_screenshots.py --check reportedly flags the same pages as main, which I did not independently run.

Recommendation

Approve with nits. The refactor is a clean extraction: lifetime, cancel idempotency, scope ordering and tool-window attributes all survive the move, the two real behaviour improvements (dropping post-cancel reports, and isHidden() instead of isVisible() at Nr2WisdomDialog.cpp:239 — which stops the reassurance line re-announcing itself to screen readers every 500 ms while the window is off-screen) are genuine fixes rather than incidental churn, and the body is unusually honest about what CI does and doesn't run. Nothing here blocks.

Next step: the one worth acting on before merge is nit 1 — !sheet.contains("{{") is the only assertion in the new test that can't fail, and the test plan leans on it. Nits 2-5 are small enough to fold into this PR or leave. Thanks for the disclosure that the new test isn't in the per-PR gate; it meant I checked ci.yml to confirm rather than reading green as coverage.


🤖 aethersdr-agent · cost: $19.6531 · model: claude-opus-5

@aethersdr-agent

Copy link
Copy Markdown
Contributor

Cisco CodeGuard — static analysis of this PR (74 finding(s))

  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/codeql.yml /tmp/aetherclaude/pr-6776/.github/workflows/codeql.yml:22
  • [CRITICAL] CS-VLN-CMDI-SUBSHELL — Vulnerability: CS-VLN-CMDI-SUBSHELL in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:471
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:77
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:281
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:291
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:363
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:388
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:955
  • [HIGH] CS-PII-BANK — PII detected: bank_account in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:224
  • [HIGH] CS-PII-BANK — PII detected: bank_account in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:358
  • [HIGH] CS-PII-BANK — PII detected: bank_account in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:359
  • [HIGH] CS-PII-BANK — PII detected: bank_account in .github/workflows/windows-installer.yml /tmp/aetherclaude/pr-6776/.github/workflows/windows-installer.yml:360
  • [HIGH] CS-VLN-SSRF-LOCALHOST — Vulnerability: CS-VLN-SSRF-LOCALHOST in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:2543
  • [HIGH] CS-VLN-SSRF-LOCALHOST — Vulnerability: CS-VLN-SSRF-LOCALHOST in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:4183
  • [MEDIUM] CS-PII-EMAIL — PII detected: email in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:3841
  • [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:1124
  • [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:2543
  • [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:2589
  • [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:2623
  • [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:2720
  • [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:2760
  • [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6776/docs/user/screens.json:4183
  • [HIGH] CS-VLN-SSRF-FILE — Vulnerability: CS-VLN-SSRF-FILE in src/core/UpdateStager.h /tmp/aetherclaude/pr-6776/src/core/UpdateStager.h:122
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:9787
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:9788
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:9789
  • [HIGH] CS-VLN-PATH-PROC — Vulnerability: CS-VLN-PATH-PROC in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:6229
  • [HIGH] CS-VLN-PATH-PROC — Vulnerability: CS-VLN-PATH-PROC in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:6308
  • [CRITICAL] CS-VLN-CMDI-AMP — Vulnerability: CS-VLN-CMDI-AMP in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:520
  • [CRITICAL] CS-VLN-CMDI-AMP — Vulnerability: CS-VLN-CMDI-AMP in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:3585
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:518
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:1227
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:1710
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:1798
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:1875
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:1917
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:1952
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:2248
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:3518
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6776/src/gui/MainWindow.cpp:3618

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: $20.5097 · model: claude-opus-5

…check is timing-safe; docs name Ctrl+W and the resizer exception. Principle VIII.

- The sheet test's "{{" check could not fail: an unknown token resolves to
  "" and leaves `color: ;`. It now checks every token the sheet uses
  resolves in the window's scope and appears in the sheet, and that no
  declaration is empty. A typo'd token (color.canon.cyn) turns it red.
- The elapsed-label check matches the format, not "0:00", so a stalled
  runner past one second no longer fails it.
- The post-cancel comment describes the outcome, not the hazard.
- dialog-patterns.md lists FFTW Wisdom among canon tool windows and says a
  short-lived one needs no resizer or saved geometry.
- nr2-noise-reduction.md names Ctrl+W (⌘W on macOS) as a way to cancel.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@ten9876

ten9876 commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

All five nits from the agent review are fixed in 00e999a:

  1. Unfalsifiable {{ check (tests/nr2_wisdom_window_test.cpp): replaced. The test now checks that each of the ten tokens the sheet uses resolves to a non-empty value through ThemeManager::cssFragment(bodyWidget(), token), in the window's canon/nr2Wisdom scope, and that the value appears in the sheet. It also checks that no declaration is left as : ;. To prove it can fail, I changed color.canon.cyan to color.canon.cyn: sheetIsCanonInBothThemes went red ("no token resolved to an empty value"). After restoring the file (sha256 matches HEAD) all 8 checks pass again.
  2. Timing-coupled elapsed check: it now matches the ^Elapsed: \d+:[0-5]\d$ format instead of 0:00.
  3. Post-cancel comment: reworded as suggested.
  4. dialog-patterns.md: FFTW Wisdom is added to the tool-window list, plus one clause saying a short-lived canon window needs neither the resizer nor saved geometry.
  5. nr2-noise-reduction.md: Cancel now also lists Ctrl+W (⌘W on macOS), worded the way the other canon pages put it.

CodeGuard: as the review reconciled, none of its findings are in lines this PR adds. The one PR-added hit is a lexical match on backticks in a C++ comment. No code change for it.

@ten9876
ten9876 merged commit 85f6bfd into main Oct 25, 2026
14 checks passed
@ten9876
ten9876 deleted the feat/fft-wisdom-canon branch October 25, 2026 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant