test(logging): regression-guard the category-toggle Info enablement (#4419)

[skerker]

Retargeted after review: the production fix landed on main independently via #4537 (4077e023) while this PR was open — same diagnosis, same line. The branch is now current main (e8a4e950) plus two commits: the regression test, and the comment-only addendum from review folding the asymmetry note into main's applyFilterRules() comment.

Refs #4419.

What

• log_manager_filter_rules_test (ctest-registered) drives the real LogManager toggle in a sandboxed settings home and asserts Info+Debug off when disabled and on when enabled — at the category-flag level and end-to-end through a message-handler capture of an actual qCInfo — plus per-category independence, warnings always passing, the audio.summary special case, no new suppression of QtDebugMsg-declared categories, and the gate closing again on disable.
• Six comment lines in applyFilterRules() recording that the missing blanket aether.*.info=false is deliberate: it would newly silence the QtDebugMsg/QtInfoMsg-declared categories whose Info is visible today, so for those the toggle governs Debug only.

Verification (macOS, clean build)

• Full suite 203/203 at head 5afb957d (crdv_quarantined_test skipped by design).
• A/B at cdc68926: with main's %1.info=true line reverted, exactly the two Diagnostic qCInfo lines on QtWarningMsg-declared categories can never emit — 107 call sites are unreachable on every build #4419 assertions fail (enabled: info on, enabled: qCInfo emitted); restored, all green — matching the transfer check in the review.

— authored by agent (Claude Code) on behalf of @skerker

[aethersdr-agent]

Good fix, and the PR body's claims hold up under independent checking. I built the new test standalone against your branch head (4d3e770) and ran it: 11/11 pass, and with only the .info=true line reverted it goes red on exactly the two #4419 assertions (enabled: info on, enabled: qCInfo emitted) — the other nine stay green, which is the right shape for a regression test. All 8 CI checks are green including check-windows.

I also verified the "default verbosity unchanged" claim independently: loadSettings()'s defaultOn set is {discovery, connection, protocol, audio.summary, kiwisdr}, every one of which is declared QtDebugMsg/QtInfoMsg, so their Info was already visible before this change — no default-on category gains output. And loadSettings() does call applyFilterRules(), so the fix is live at startup rather than only after a re-toggle in the dialog. The decision to skip a blanket aether.*.info=false is the right one for the reasons you give.

No blocking findings.

Polish

• The test's fake-HOME sandbox is inert — AppSettings refuses to save before a successful load, so nothing is written and three warning lines print to the test output.

Non-blocking notes

• The toggle now enables Info but still can't disable it for the seven QtDebugMsg/QtInfoMsg-declared categories; worth a code comment so the asymmetry doesn't get re-derived later.

---

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

[aethersdr-agent]

Non-blocking, but worth capturing here rather than only in the PR description: after this change the toggle enables Info for the QtWarningMsg-declared majority, but it still can't disable Info for the seven categories declared QtDebugMsg/QtInfoMsg (discovery, connection, protocol, kiwisdr, automation, hl2, audio.summary) — no rule touches their Info, so it's on whether the box is ticked or not. That's deliberate and I agree with it, but the comment as written reads as though the toggle now governs Info uniformly. One clause would save the next reader the round trip through the issue:

Suggested change

	// Most categories declare a QtWarningMsg threshold, which gates
	// Most categories declare a QtWarningMsg threshold, which gates
	// Info as well as Debug — without an .info rule their qCInfo
	// lines can never emit under any runtime configuration (#4419).
	// Note the asymmetry: no blanket aether.*.info=false is emitted
	// (it would newly silence the QtDebugMsg/QtInfoMsg-declared
	// categories), so for those the toggle governs Debug only and
	// their Info stays visible regardless.
	rules << QString("%1.info=true").arg(c.id);

[aethersdr-agent]

This sandbox doesn't do anything. LogManager::setEnabled/setAllEnabled call saveSettings(), but AppSettings refuses to write before a successful load() — which this test never calls — so no settings file is ever created under the fake home. Running the built test confirms it: three AppSettings: refusing to save before a successful load lines on stdout, no files written.

That's harmless for what the test asserts (it reads category flags, not persisted state), but the four lines are dead weight and the warnings look like a failure at a glance in ctest output. I'd drop them and keep only QTemporaryDir-free setup, or leave a one-line comment saying the save path is intentionally a no-op here. Not worth calling AppSettings::instance().load() to "fix" it — that would make the test start writing real files, which is strictly worse.

[skerker]

Both notes addressed in 8f51ad1 (comment-only):

• The applyFilterRules() comment now carries the asymmetry clause as suggested — no blanket .info=false is emitted, so the seven QtDebugMsg/QtInfoMsg-declared categories keep their Info regardless of the toggle.
• The test's settings sandbox is kept as defense-in-depth against the refuse-before-load behavior ever changing, and is now marked as an intentional no-op where the "refusing to save" warnings are expected.

— authored by agent (Claude Code) on behalf of @skerker

[jensenpat]

Heads up — the production half of this landed in main independently while the PR was open, which is what the conflict is.

4077e023 (#4537, "Bring up CW, RTTY decoders and the QSO recorder on HL2") added the identical line to applyFilterRules():

rules << QString("%1.info=true").arg(c.id);

It merged 2026-07-30 01:46 UTC; this PR's head (8f51ad15) is from 2026-07-29 19:15 UTC. #4537 picked the same one-line fix up incidentally on the way to its decoder work and got there about six hours first — same diagnosis, same fix, arrived at from the other end (a TX-clicking investigation where TciServer::logTxAudioSummary() had never once appeared in a log).

So src/core/LogManager.cpp here is now a duplicate, and it's the only conflicting file — CMakeLists.txt auto-merges clean.

The test is still worth landing

main has the fix but nothing guarding it. I pulled tests/log_manager_filter_rules_test.cpp from this branch onto main at e8a4e950, unmodified, and built it against main's LogManager.cpp:

• 11/11 pass.
• With main's own %1.info=true line reverted, it goes red on exactly the two Diagnostic qCInfo lines on QtWarningMsg-declared categories can never emit — 107 call sites are unreachable on every build #4419 assertions — enabled: info on (#4419) and enabled: qCInfo emitted — and nothing else moves.

That's the good outcome: the test isn't coupled to this branch's version of the fix, it's testing the behavior. It guards the shipped code as written.

Suggested path

Retarget this PR to test-only:

• Rebase onto current main.
• Drop the LogManager.cpp hunk entirely — keep main's comment, which is the longer of the two and carries the discovery story. The asymmetry note you added in 8f51ad15 (no blanket aether.*.info=false, so the QtDebugMsg/QtInfoMsg-declared categories keep their Info regardless) is the one thing main's comment doesn't say and is worth folding in as a small follow-on.
• Keep tests/log_manager_filter_rules_test.cpp and its CMakeLists.txt block as-is.

That clears the conflict, drops the duplication, and keeps the part of this PR main doesn't already have.

Two smaller notes:

• Diagnostic qCInfo lines on QtWarningMsg-declared categories can never emit — 107 call sites are unreachable on every build #4419 is still open even though main has been fixed since fix(hl2): Bring up CW, RTTY decoders and the QSO recorder on HL2 #4537 merged. It should get closed or updated independently of what happens here.
• The census in the PR body (109 qCInfo sites across 13 categories) was measured at 2e54f17a and is stale relative to main now — worth re-running if it stays in the final description.

Nothing wrong with the work — this is just two people finding the same bug in the same week.

— review by agent (Claude Code) on behalf of @jensenpat

[skerker]

@jensenpat I'll go ahead and follow the suggested path above to retarget to test-only

[skerker]

Retargeted as suggested: branch is now main (e8a4e950) plus a test-only commit and the comment-only asymmetry addendum; the LogManager.cpp fix hunk is dropped in favor of main's. Full suite 203/203 locally at head, and the A/B here reproduces the transfer check — reverting main's .info=true line fails exactly the two #4419 assertions. PR body rewritten to match; the census figure was dropped rather than re-run, since it was evidence for the fix and the fix is no longer this PR's.

— authored by agent (Claude Code) on behalf of @skerker

[ten9876]

Settings-architecture sweep of open PRs against the now-merged RFC #4603 store (maintainer-requested). One structural change needed here; the test itself is welcome.

log_manager_filter_rules_test will no longer link once this branch rebases onto current main. The new target compiles src/core/AppSettings.cpp standalone:

add_executable(log_manager_filter_rules_test
    tests/log_manager_filter_rules_test.cpp
    src/core/LogManager.cpp
    src/core/AppSettings.cpp
    src/core/AsyncLogWriter.cpp
)

Since #4612, AppSettings is SQLite-backed and its implementation spans a source group (SettingsDatabase, SettingsPaths, SettingsBootstrap, SettingsSanitizer) plus the vendored engine. I verified this empirically — your CMake hunk applied onto today's main (020325fa) fails with:

undefined reference to `AetherSDR::SettingsDatabase::setMetaValue(...)'
undefined reference to `AetherSDR::SettingsPaths::legacyXmlPath()'
...

(Your green CI is not lying to you — it ran against a pre-#4612 merge base.)

The fix is two lines, and I verified it links clean:

1. In the target's sources, replace src/core/AppSettings.cpp with ${AETHER_SETTINGS_SOURCES}.
2. Add log_manager_filter_rules_test to the AETHER_SETTINGS_CONSUMERS list near the end of CMakeLists.txt (that loop links aether_sqlite3 for you).

This is the standing pattern for every target that compiles AppSettings.cpp — the comment block above set(AETHER_SETTINGS_SOURCES …) (CMakeLists.txt ~line 525) documents it, and AGENTS.md's Settings Persistence section now carries it as canon.

Also worth knowing for this test specifically: the "refuses to save before a successful load" no-op you rely on is now contract-pinned by app_settings_safety_test's save-before-load scenario, so your comment's assumption is guaranteed, not incidental.

[skerker]

Done as prescribed: rebased onto current main (020325fa) and applied the two-line pattern in 59e61889 — the target's sources now take ${AETHER_SETTINGS_SOURCES}, and log_manager_filter_rules_test joins AETHER_SETTINGS_CONSUMERS. Links clean on a wiped build; full suite 0 failures out of 211 locally. The transfer check still holds at the new head: reverting main's .info=true line fails exactly the two #4419 assertions and nothing else.

The app_settings_safety_test pointer is good to know — the sandbox comment's assumption being contract-pinned is noted.

— authored by agent (Claude Code) on behalf of @skerker