[ten9876]

Intent

Make the CTR2 window's popups match the window they come from. The CTR2 window is a CanonWindow, but every question and notice it raised was a plain QMessageBox: an OS icon and native buttons over the canon ground.

Also: a CTR2 that leaves the bus right after being asked to restart into its other firmware (or download mode) is not reported as a lost device.

Non-goals: no change to what any popup says or when it appears, and no change to other windows' message boxes. The file picker stays the platform's own.

What

• CanonMessageBox (src/gui/CanonMessageBox.{h,cpp}): a CanonWindow dialog in place of QMessageBox.
  • The title is a canon header, the text is in canon ink, and optional further text is in the softer ink. Canon buttons sit at the bottom right.
  • The default button is outlined in cyan; a destructive button (Restore) is drawn in gold. Buttons are auto-default, as in QMessageBox, so Return on a focused button clicks that button.
  • The texts are 420 px wide and may grow to 720, so a long path in a command to copy is never clipped.
  • Opening it sends screen readers an Alert, as QMessageBox does, and its accessible description carries both texts.
  • Its title and texts are plain text: they carry firmware-manifest and device strings, and markup in them is shown, never rendered.
  • clickedButton() names the button clicked, and stays null when the box is closed any other way (corner button, Escape, Ctrl+W). The callers' clickedButton() != go checks are unchanged.
• All eleven CTR2 popups move to it:
  • FirmwareInstallDialog: install, install over the USB cable, update the stock firmware, restore a backup, and put the CTR2-Max in download mode.
  • Ctr2ProxyPanel: the Install rule question and the five Linux USB-access notices.
• One behavior change: Restore a backup now opens with Cancel as the default, so Return never overwrites the device; restoring takes a click.
• docs/style/dialog-patterns.md says when to use it, and the user guide's Backups section notes the Cancel default.

Restart requests that beat their answer

Restart into (SWITCH) and download mode (BOOTLOADER) waited for the device's STATUS. A port loss before it arrived read as "Lost the device: … The device keeps the firmware it had."

Lynovation's CTR2-Max v1.02.00 betas answer SWITCH naming the op as a character ('5', 0x35, instead of 0x05). AetherSDR ignores that answer, and the unit leaves the bus two seconds later, so a switch that worked was reported as a failure (Lynn saw it on Windows).

• Ctr2FirmwareUpdater: a port loss while the answer is outstanding, once the port has written the request (reportsSent), now finishes the request as the OK answer does: "The device is restarting into its other firmware" (or download mode). A port that fails before writing it (a write error) still reports the device lost, and an answer that refuses the request still fails it.
• Ctr2HidThreadPort: writes that went out before a failing one in the same tick are now reported sent, before the failure, so the written count never misses a request the device received. A restart request that could not be queued at all fails at once.
• docs/ctr2-usb-relay-design.md states the rule.

Testing

• New canon_message_box_test:
  • a click on an accepting, a rejecting and a destructive button each name that button;
  • Escape and the close button leave no button clicked;
  • Return clicks the default button, which opens with the focus;
  • Return on a focused Cancel clicks Cancel; the real corner close button leaves no button clicked;
  • the further text shows only when set, and the accessible description holds both texts.
• New ctr2_hid_thread_port_test case: writes before a failing one in the same tick are reported sent before the failure. It fails with that acknowledgement removed.
• ctr2_firmware_updater_test: a simulated device that answers naming the wrong op, and one that leaves for download mode without answering, both finish as restarts; a port whose write of the request fails is reported lost. Forcing the written-check true fails the write case; forcing it false fails both departure cases.
• All nine ctr2_* tests and the nine firmware-installer tests pass.
• Mutations: not recording the click fails four checks; not focusing the default button fails three. Both pass again once restored.
• The app builds, and the four static gates pass (check_engine_boundary --strict, gen_touchpoint_manifest --check, check_capability_records --strict, check_command_plane --strict).
• The boxes were rendered offscreen from the real class (install over the cable, restore, a notice) and reviewed.
• Not tested: the popups inside the running app, and Windows and macOS.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WvUonxmfcg6yopRJnXx9VG

[ten9876]

Reviewed head 7ef5ba430d70f56e263de590dd3e1c83648d4a4e.

Issue fit: partially meets the stated intent; request changes. There is no linked closing issue. The eleven popup migrations and Cancel-default restore implement the stated CTR2 scope. The restart workaround fixes the wrong-op/silent-departure cases, but also turns a failed command write into a successful restart.

Scope audit

File/group	Claimed?	Verdict
CanonMessageBox.{h,cpp}	Yes	Internal GUI helper for canon questions/notices
Ctr2ProxyPanel.cpp, FirmwareInstallDialog.cpp	Yes	Eleven callers migrated; Restore default explicitly disclosed
Ctr2FirmwareUpdater.{h,cpp}	Yes	Restart outcome change explicitly disclosed; correctness blocker below
Two test sources	Yes	Dialog behavior and injected restart coverage
CMakeLists.txt, tests/tests.cmake	Yes	Production registration and socket-free test registration
docs/style/dialog-patterns.md, docs/ctr2-usb-relay-design.md	Yes	Contributor documentation for those changes

Everything in the diff is explained by the body. The restart commit's authored time predates the first style commit, but its content is explicitly included in the title and intent; it is not undisclosed scope. No new external protocol/config surface, dependency, thread, or enforcement-baseline growth. Both commits have verified signatures. No CHANGELOG entry is needed.

Blocker

1. [P2] A failed restart write is reported as a successful restart — src/core/Ctr2FirmwareUpdater.cpp:460–464.

   Ctr2HidPort::failed means “gone or unusable,” not confirmed USB departure. send() queues reports, and the real worker emits this same failure signal when writeReports() returns negative (src/core/Ctr2HidThreadPort.cpp:256–259), as well as for read/framing errors. Phase::Commanding is entered before the restart reports are sent. Consequently an EIO while writing SWITCH/BOOTLOADER reaches restarted() even if the device never received or executed the command.

   Reproduced with the existing socket-free injected port: fail the restart write before processing it, emit failed("CTR2 USB write failed: injected EIO"), and leave restartedInto empty. Head returns:

   WRITE_FAILURE_RESULT ok=1 restarted= message=The device is restarting into its other firmware.
   FAIL: a restart write failure must not claim success
   

   Substituting the merge-base updater implementation in the same harness returns:

   WRITE_FAILURE_RESULT ok=0 restarted= message=Lost the device: CTR2 USB write failed: injected EIO. The device keeps the firmware it had.
   

   Preserve failure/uncertainty when transmission fails; distinguish that from an expected restart departure, or verify the resulting device state before asserting success. Add this negative injection beside the two new positive cases. This finding is reproduced at the updater's transport seam, not against a physical USB device; no firmware write or restart was attempted.

Non-blocking nit

• User guide missing the new Restore default — src/gui/FirmwareInstallDialog.cpp:710; docs/user/docs/ctr2-proxy.md:259. The contributor docs change, but the user guide's Backups section does not explain that Cancel is the default and Return cancels. Add that small user-facing update in this PR, per AGENTS.md's same-PR user-documentation requirement.

Verification and attacks

• Built the app and named tests in a scratch export, using the skill's Linux shared-core configuration. Original canon_message_box_test, ctr2_firmware_updater_test, esp_firmware_installer_test, esp_firmware_job_test, and firmware_install_model_test all pass. I did not independently rerun the entire claimed nine-plus-nine set.
• Review-only dialog checks pass for Ctrl+W dismissal and Return after focusing Restore while Cancel is default. Removing click recording kills four checks; removing explicit default focus kills one. Replacing the head updater with its base implementation kills both new restart assertions, while the injected write-failure assertion passes on base and fails on head. Explicit device refusal still fails as intended.
• All four architecture gates pass on base and head. Per-file engine findings are identical (101 tracked), the touchpoint manifest is unchanged/current, capability booleans remain 70 and above-seam commands remain 125. No socket-owning added/modified test was executed.
• Opened the actual CTR2 window via the automation bridge in an isolated, offscreen, NO_TX instance explicitly connected to DEMO-0001. Bridge reported model AetherSDR Demo, transmitting: false; install confirmation remained disabled, so popup appearance inside the running workflow remains unverified. Closed the review instance. No live radio connection, install, restore, or restart was requested.
• Sources were restored and checked against the exact head after mutations. The invoked checkout was untouched. No separate code-review skill was available; the adversarial code/governance pass was manual. Windows/macOS behavior remains unverified. Latest main predates the head's CI run creation; Static checks pass and platform builds were still running at the final check.

Recommendation: Request changes for blocker 1. The dialog migration and disclosed default change survived the focused behavioral checks; narrow the restart success condition and retain the negative write-failure proof before landing.

Posted as COMMENT because the authenticated reviewer account is also the PR author and GitHub disallows self REQUEST_CHANGES reviews; the recommendation above is Request changes.

[ten9876]

Both findings are fixed in a284c3192.

Blocker: a failed restart write was reported as a restart. Fixed in src/core/Ctr2FirmwareUpdater.cpp (onPortFailed, Phase::Commanding).

• The updater now counts the reports it queues on each port, and the ones the port reports written through reportsSent. The USB worker delivers reportsSent before any later failed.
• The point where the restart request was queued is recorded. A port loss counts as the restart only once the written count has reached it.
• Before that, for example on the worker's CTR2 USB write failed, the result is "Lost the device: … The device keeps the firmware it had.", as on the merge base.

Proof is in ctr2_firmware_updater_test:

• The simulated port now acknowledges writes the way the worker does. It can also fail the write of a FIRMWARE message, which is your injected EIO: the port closes and failed("CTR2 USB write failed: injected EIO") fires before the device sees the request.
• New check: "a restart request whose write fails is reported as lost, not as a restart". It expects ok=false, restartedInto empty, and a message containing "write failed" and "keeps the firmware".
• Mutations:
  • with the written-check forced true (the old behavior), only the new check fails;
  • forced false, only the two departure checks fail (wrong op, silent leave);
  • restored, all pass.
• All nine ctr2_* tests, the nine firmware-installer tests and canon_message_box_test pass, and so do the four static gates.
• docs/ctr2-usb-relay-design.md now states the written condition.

Nit: the user guide. docs/user/docs/ctr2-proxy.md § Backups now says the restore question has Cancel as its default, so Return keeps what is on the unit.

[aethersdr-agent]

Issue fit

No linked issue, and none is required: the canon restyle is approved RFC #6226 work, and the second commit is a bug fix with a named root cause (the v1.02.00 beta answering SWITCH as '5'), which GOVERNANCE.md explicitly exempts from the RFC path. Both halves of the title are delivered by the diff, and the body's claims held up everywhere I could test them. I reviewed against the PR's own stated intent.

One coordination note, not a problem with this PR: #6733 hand-rolls ResetSettingsDialog as a CanonWindow for a confirmation plus a failure notice — the job CanonMessageBox now does, minus #6733's file-list widgets — and both PRs add a paragraph to docs/style/dialog-patterns.md. #6735 is parked on the same "is this a CanonWindow pick" question. Whichever lands second will need a rebase, and #6733's confirm half could ride this primitive instead of a one-off class. Worth deciding the order rather than discovering it in a conflict.

Scope

Everything in the diff is explained by the stated intent. No unrelated files, no formatting churn, no new settings keys, no protocol or capability surface, and correctly no CHANGELOG.md entry.

File(s)	Change	Claimed?	Verdict
src/gui/CanonMessageBox.{h,cpp}	new QMessageBox replacement on CanonWindow	yes	In scope
CMakeLists.txt	+1 line, the new source into GUI_SOURCES	implied	In scope
src/gui/Ctr2ProxyPanel.cpp	6 popups converted	yes	In scope
src/gui/FirmwareInstallDialog.cpp	5 popups converted; !ask null guards added	yes	In scope — the guards are a small hardening, matching Ctr2ProxyPanel's existing pattern
src/core/Ctr2FirmwareUpdater.{cpp,h}	restarted() extracted; Phase::Commanding port-loss branch	yes (2nd commit)	In scope
docs/style/dialog-patterns.md, docs/ctr2-usb-relay-design.md	state the new rules	yes	In scope
tests/canon_message_box_test.cpp, tests/ctr2_firmware_updater_test.cpp, tests/tests.cmake	new target + 2 cases	yes	In scope

Deleted lines. The only removals are the five lines restarted() absorbs and the QMessageBox includes. No guard, early return, or symptom-naming comment was dropped.

Preference check (§6). There is one user-visible behavior change — Cancel becomes the default on "Restore the backup?" — and it is disclosed in the body, it moves a destructive action toward safety, and it is authored by the Project Maintainer, so the visual direction and this default are ratified by the person who ratifies them. (The task prompt labels the author a community contributor; that is wrong.) Nothing here reads as smuggled preference.

Socket tests: none. Neither added/modified test file contains QTcpServer, QTcpSocket, QUdpSocket, QLocalServer, QWebSocketServer, listen(, bind( or connectToHost(. SimPort in ctr2_firmware_updater_test.cpp is a pre-existing in-process Ctr2HidPort subclass, not a network peer, and this PR only adds two fields to its device struct.

Blockers

None.

Nits (all non-blocking)

1. src/gui/CanonMessageBox.cpp:28-45 — the new message box renders its text as Qt::AutoText, and it is the one surface in these two files that doesn't opt out. Credit: security-audit (CodeGuard), verified against the diff.

Neither label sets a text format, so both default to Qt::AutoText, and Qt::mightBeRichText() promotes the whole label to rich text as soon as markup appears in the string:

    m_text = new QLabel(text);
    m_text->setObjectName(QStringLiteral("canonMessageText"));
    m_text->setWordWrap(true);

grep -c setTextFormat src/gui/CanonMessageBox.cpp → 0. Meanwhile both files being converted already make the opposite choice explicitly, with a comment saying why:

• src/gui/Ctr2ProxyPanel.cpp:154 — // Carries a device's own product string; never render it as rich text.
• src/gui/Ctr2ProxyPanel.cpp:206 — same comment on the error label
• src/gui/FirmwareInstallDialog.cpp:46 — l->setTextFormat(Qt::PlainText); in the dialog's own label helper

And the converted call sites do interpolate non-constant data: FirmwareInstallDialog.cpp:742 tr("Install %1?").arg(product), :743 .arg(product, version), :837 stockFirmwareName(), :705 backupText(path), plus the free-form detail/why strings at Ctr2ProxyPanel.cpp:348 and :382. product and version are free-form JSON from the firmware manifest with only a non-empty check (src/core/EspFirmwareManifest.cpp:120-121, :128), i.e. they come from whoever supplies the package. A crafted manifest could restyle the text of a confirm-before-flash prompt. (I checked linkVersion() separately — it is built from three integers in Ctr2HidFraming.h:183-201, so the device itself cannot inject anything.)

This is a nit and not a blocker because QMessageBox also defaults to AutoText, so it is not a regression — but it is new shared code whose whole job is to be the house message box, with exactly one consumer today, so it is the cheapest it will ever be to make safe by default. The one I'd fix before merge:

    m_text = new QLabel(text);
    m_text->setObjectName(QStringLiteral("canonMessageText"));
    m_text->setTextFormat(Qt::PlainText);
    m_text->setWordWrap(true);

…and the same line for m_info. Worth a thought for the title too: makeCanonHeader() (src/gui/CanonWindow.cpp:540) is shared code that has only ever been handed constant titles, and CanonMessageBox is the first caller to pass an interpolated one. That one is a judgment call — Qt::PlainText inside makeCanonHeader() touches every canon window, so it may belong in its own change.

2. src/core/Ctr2FirmwareUpdater.cpp:460-464 — the Commanding branch cannot tell a restart from a device that never got the command, and the 5 s timeout no longer covers that window.

    case Phase::Commanding:
        // Leaving the bus is what the request asks for. A device that does it
        // before its STATUS arrives, or whose STATUS names another op, has
        // still restarted.
        restarted();
        break;

The message argument is dropped, and every port failure in Commanding now reports success. Three cases reach it: the real one this PR fixes; a cable pulled between Ready and the SWITCH landing; and a transient HID error while the device sits on the bus still running its old firmware. send() is fully asynchronous (Ctr2HidThreadPort.cpp, queued to the worker), so a failed write of the SWITCH report also arrives here as a plain failure with the phase still Commanding.

I initially wrote this off as a wrong message, but the consequence is a bit larger for Restart::Bootloader, which is step one of a chain — FirmwareInstallModel.cpp:150-156:

                    if (m_waiting && ok) {
                        // Restarted into download mode: the install starts
                        // once it is found there.
                        m_poll->start();

So on a false success the installer starts polling for a download-mode device that never appears, and the operator learns of the loss later from that wait's own outcome rather than from "Lost the device" at once. Nothing is written to the device and nothing is automated into a keyed transmitter, which is why this stays a nit: the firmware is untouched either way, and the fix is a real improvement over failing a switch that actually worked.

If you want to narrow it, Ctr2HidThreadPort already emits reportsSent(count) and the updater never connects to it. Gating the branch on "the SWITCH/BOOTLOADER report was confirmed sent" would separate "it got the command and left" from "it never got it", without giving up the fix. Offered as a suggestion, not a request.

3. src/gui/CanonMessageBox.cpp:66 / src/gui/CanonMessageBox.h:40 — m_clicked is never cleared, so a box shown twice reports a stale button. exec() on a box that was previously dismissed with Escape would still return the button clicked the first time. Unreachable today — every call site is a single-exec() scoped local — but it is a footgun on a new public API whose contract is "null when the box is closed any other way". Clearing m_clicked in showEvent() would close it.

4. src/gui/CanonMessageBox.cpp:59 — with no default button set, Return does nothing, because addButton() sets setAutoDefault(false) on every button and QDialog then finds no default to click. All eleven current call sites do set one, so this is latent. A comment on setDefaultButton() saying it is effectively required, or falling back to the first accepting button, would stop the next caller finding out the hard way.

5. Windows does not compile the new test target in this PR. check-windows builds --target AetherSDR aether-dv-waveform thumbdv_queue_test dstar_model_test digital_voice_waveform_process_test cross_needle_meter_test (.github/workflows/ci.yml:796), so canon_message_box_test.cpp is only compiled by the Linux build and check-macos jobs, which do build the full tree. An MSVC-only break in the new test would surface on main's full-suite, not here. Nothing to change — just don't read the three green builds as three-platform cover for the test itself.

What I tried to break

Findings above are reasoned from the code and from the repo's own static gates; I have no build and no way to drive the GUI, so nothing here was reproduced at runtime.

• The false-success hunt that motivated the review. My main suspicion was that Phase::Commanding covered commands other than SWITCH/BOOTLOADER, which would make restarted()'s m_restart && … guard fall through to "restarting into its other firmware" for an unrelated command. It doesn't: Commanding is set at exactly one site (:317), inside if (m_phase == Phase::Calling && m_restart), so the phase implies the request. Survived.
• A refusal laundered into a success. A device answering with the wrong op and a refusing status is ignored by the reply.op != check, so the refusal is never seen — but it then stays on the bus, and onTimeout() still fails Commanding honestly (:505, "The device did not answer the restart request."). The PR's "an answer that refuses the request still fails it" holds. Survived.
• Re-entrancy and leaks around the new helper. restarted() has no if (!isRunning()) return guard, unlike fail(), so I checked whether it can run twice: closePort() disconnects every signal before shutdown(), and onPortFailed() clears m_port before switching on the phase, so neither path can re-enter. It also skips m_retry->stop(), which fail() and succeed() both do — harmless, because the retry lambda is phase-guarded to Calling/Confirming/Returning/Verifying and is already stopped at :314 on entering Commanding. It leaves m_port unclosed on the port-failure path, but that path already shut the port down, and openPort() begins with closePort() anyway. Survived.
• Behavior preservation in the extraction. The refactor moves m_timeout->stop() from before closePort() to after it. Checked that closePort() cannot make the timer fire in between (QTimer only fires from the event loop); the two orderings are equivalent.
• Button semantics against the old QMessageBox. Walked each converted site for dismissal parity: QMessageBox returns Cancel on Escape and on the window close button, so the old exec() == QMessageBox::Ok was false in both cases; the new clickedButton() == ok is false in both too. askDownloadMode, onInstall, installOverLink, installStockFirmware and confirmRestore all preserve "anything but the affirmative button means don't". Survived.
• The test that claims the corner close button. canon_message_box_test.cpp:117 is titled "the corner close button" but calls box.close(). Not a cheat: CanonWindow.cpp:74 wires m_close straight to QDialog::close, so the test exercises the same slot the button does. Escape and Ctrl+W reach QDialog::reject() (CanonWindow.cpp:129), matching the Rejected assertions.
• Whether the new test can link. Its CMake block mirrors canon_window_test and adds CanonMessageBox.cpp + ModemChrome.cpp; makeCanonHeader is defined in the already-linked CanonWindow.cpp:532, and applyCanonSheet is inline in CanonIndicators.h:150, so no missing object. Confirmed empirically by the three green builds on the two full-tree jobs.
• Theme tokens. color.canon.{ink,inkSoft,cyan,sparkGold} are all in established {{…}} use (AgcCalibrationDialog.cpp, Ax25HfPacketDecodeDialog.cpp), so the cyan and gold resolve rather than leaving literal braces. The :default border-color override works because ModemChrome.cpp:190-197 gives QPushButton a 1px solid border to recolour.
• Repo gates, run locally against the PR head: check_test_registration OK, check_ci_test_gate OK (and neither new test is in the frozen 7-name per-PR gate, which is the documented norm), check_a11y.py src/gui/CanonMessageBox.cpp → no findings.
• Sibling call sites. grep for QMessageBox and FramelessMessageBox in both converted files returns nothing, so the "all eleven" claim is complete for these two files, with the platform file picker left alone as the body says.
• CodeGuard's other 100-odd hits — CS-VLN-CMDI-*, CS-VLN-SSRF-*, CS-VLN-PATH-PROC, CS-PII-*, CS-INJ-*, CS-MAL-SCRIPT — I refute. Most are on docs/user/screens.json and src/gui/MainWindow*, which this PR does not touch (the harness file list looks like main's tip, not the diff). In the files it does touch, every & is a connect/address-of or a lambda capture and every | is a bit-or; there are no backticks and no shell. The only real shell nearby is Ctr2ProxyPanel.cpp:397-439, untouched here and already safe — a constant script under pkexec /bin/sh -c with the rule path and the constant kRuleTarget passed as quoted $1/$2. The sudo cp … | udevadm … text at :361 is display-only copy-paste instructions. This matches the known lexical-false-positive pattern for CS-VLN-CMDI-BACKTICK on comments.
• Could not check: the popups in the running app, and Windows/macOS rendering — the body already says so. Contrast figures for the gold destructive button and the cyan default against the canon button gradient are not in the body and I did not compute them; given #6227/#6239's history of wrong contrast arithmetic, a measured number would be better than an assumed one if a11y review wants it. On coverage: the two new simulator cases prove the state machine's rule — any bus departure during Commanding finishes as a restart — which is the robust claim and holds whether or not the 0x35-after-2 s model of the Lynovation beta is exactly right. The firmware behavior itself rests on "Lynn saw it on Windows", not on the test.

Recommendation

Approve with nits. The restart fix is the right shape — it keys on the phase, which provably implies a restart request, and it turns a working switch that was reported as a lost device into a success. The extraction is behavior-preserving, the new tests are genuinely red without the change, and the eleven conversions preserve every dismissal semantic I could trace. Nit 1 is the one I'd fix before merge: two lines in the constructor, and it makes the whole converted surface plain-text by default instead of relying on each call site, in the one file whose neighbours already carry a comment saying not to render device and manifest strings as rich text. Nits 2-4 are small and can ride a follow-up. The thing most worth a decision is the ordering against #6733 and #6735, since all three touch docs/style/dialog-patterns.md and two of them want this primitive.

Nice work — the mutation testing in the body, and restarted() being one helper for both the answered and unanswered paths rather than two copies, both made this quick to verify.

---

_{🤖 aethersdr-agent · cost: $19.1339 · model: claude-opus-5}

[aethersdr-agent]

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

• [HIGH] CS-VLN-SSRF-LOCALHOST — Vulnerability: CS-VLN-SSRF-LOCALHOST in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:2581
• [HIGH] CS-VLN-SSRF-LOCALHOST — Vulnerability: CS-VLN-SSRF-LOCALHOST in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:4221
• [MEDIUM] CS-PII-EMAIL — PII detected: email in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:3879
• [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:1124
• [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:2581
• [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:2627
• [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:2661
• [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:2758
• [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:2798
• [LOW] CS-PII-IP — PII detected: ip_address in docs/user/screens.json /tmp/aetherclaude/pr-6736/docs/user/screens.json:4221
• [MEDIUM] CS-VLN-SSRF-HEX — Vulnerability: CS-VLN-SSRF-HEX in src/core/Ctr2FirmwareUpdater.cpp /tmp/aetherclaude/pr-6736/src/core/Ctr2FirmwareUpdater.cpp:20
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/core/FirmwareCurrency.h /tmp/aetherclaude/pr-6736/src/core/FirmwareCurrency.h:54
• [HIGH] CS-PII-BANK — PII detected: bank_account in src/core/FirmwareStager.cpp /tmp/aetherclaude/pr-6736/src/core/FirmwareStager.cpp:165
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/core/FirmwareStager.h /tmp/aetherclaude/pr-6736/src/core/FirmwareStager.h:26
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/core/FirmwareStager.h /tmp/aetherclaude/pr-6736/src/core/FirmwareStager.h:47
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/core/FirmwareStager.h /tmp/aetherclaude/pr-6736/src/core/FirmwareStager.h:120
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/FirmwareReleasesDialog.cpp /tmp/aetherclaude/pr-6736/src/gui/FirmwareReleasesDialog.cpp:89
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/FirmwareReleasesDialog.cpp /tmp/aetherclaude/pr-6736/src/gui/FirmwareReleasesDialog.cpp:91
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:9721
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:9833
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:9955
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:9956
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:9957
• [HIGH] CS-VLN-PATH-PROC — Vulnerability: CS-VLN-PATH-PROC in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:6221
• [HIGH] CS-VLN-PATH-PROC — Vulnerability: CS-VLN-PATH-PROC in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:6300
• [CRITICAL] CS-VLN-CMDI-AMP — Vulnerability: CS-VLN-CMDI-AMP in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:521
• [CRITICAL] CS-VLN-CMDI-AMP — Vulnerability: CS-VLN-CMDI-AMP in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:3577
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:519
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:1219
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:1702
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:1790
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:1867
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:1909
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:1944
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:2240
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:3510
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:3610
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:6163
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:8040
• [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-6736/src/gui/MainWindow.cpp:8114

_{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.3337 · model: claude-opus-5}

[ten9876]

Thanks. Your review was of a284c3192. Here is where each nit stands at 182af47bc:

1. Plain text. Fixed in 182af47bc. The text and further-text labels are Qt::PlainText. So is the box's own header title, set on the canonHeaderTitle label inside CanonMessageBox, which leaves the shared makeCanonHeader() unchanged. New check: markup in all three texts stays plain. It fails with the main text's format line removed.
2. Commanding branch. This was already narrowed in a284c3192, the commit your review covered, as you suggested. A port loss counts as the restart only once reportsSent has covered the request; a failed write is still "Lost the device". 5269db7b1 makes the count exact: writes that went out before a failing write in the same tick are reported before the failure, with a new ctr2_hid_thread_port_test case. A request that could not be queued at all now fails at once. Any loss after the request is written still reads as restarting, including a pulled cable. That is the maintainer's ruling.
3. Stale m_clicked. Fixed in 182af47bc: it is cleared in showEvent(). New check: a box shown again after a click, then dismissed with Escape, reports no button. It fails without the reset.
4. No default. Buttons became auto-default in 5269db7b1, as QMessageBox's are, so a focused button takes Return itself. setDefaultButton() now says every box sets one, and all eleven do.
5. Windows test coverage. Noted. The test runs on the Linux and macOS full-tree builds.

On the corner close button: since 5269db7b1 the test clicks the real QToolButton instead of calling close().

All 19 related tests and the four static gates pass at 182af47bc.