[ten9876]

/pr-review and /pr-land both tell the agent to build the PR head. The obvious way to do that builds the default all target, which is the whole tree. This PR changes both skills to configure a lean build and build only the targets the review actually runs.

Why: what a full build costs

Measured while reviewing #6195 and #6198. Each head was built on Linux (GCC, Qt 6.12.0, RelWithDebInfo, static core: the default configuration):

	Full tree (default all)
Build directory	84 GB per PR (two PRs plus one merge base: ~172 GB)
Of which, top-level executables	83.7 GB
Test executables	~628; 232 of them over 100 MB each
AetherSDR	832 MB, of which ~680 MB is .debug_* sections and ~137 MB code and data
Largest tests	radio_setup_*_test ~397 MB each, weather_radar_loading_test 408 MB

The review itself ran about twenty of those tests.

Two causes:

1. The static core is copied into every test. Each executable that links aethercore absorbs a full static copy. The AETHER_SHARED_CORE comment in CMakeLists.txt already records this ("~200 MB in a Debug ASan build … shared, they average ~13 MB"); the sanitizer lane switched to a shared core for exactly this reason.
2. Full debug info per binary. -g under RelWithDebInfo makes debug info about 5× the size of the code. That ratio repeats across every binary.

What changes

/pr-review step 7, new "Build only what you run" paragraph:

cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DAETHER_SHARED_CORE=ON \
  "-DCMAKE_CXX_FLAGS_RELWITHDEBINFO=-O2 -g1 -DNDEBUG" \
  "-DCMAKE_C_FLAGS_RELWITHDEBINFO=-O2 -g1 -DNDEBUG"
cmake --build build --parallel --target AetherSDR <test_a> <test_b>
ctest --test-dir build -R '^(test_a|test_b)$' --output-on-failure

• Choosing tests: the ones the PR adds or modifies, the ones its body names, and the ones whose tests/tests.cmake block compiles a file the PR touches.
• Full-tree builds are kept for claims about the tree itself (CMake or link-contract changes), and the report must say when one was done.
• The merge-base comparison ("before blaming the PR") builds the same targets instead of a clean full tree.
• Caveats, stated in the skill:
  • Linux and macOS only. AETHER_SHARED_CORE is a configure error on Windows, and -g1 is a GCC/Clang flag.
  • The app loads the core as a shared library, which isn't the shipped shape. Fine for driving behaviour; a finding that could depend on link mode should say so.
  • A test that calls WDSP directly can't rely on the shared core exporting it (WDSP is a private, hidden-visibility dependency). It links aether_wdsp explicitly or goes through WdspChannel.h; no static reconfigure is needed.
  • -R matches the add_test(NAME …) name, which can differ from the target name, so both recipes pass --no-tests=error. A filter that matches nothing fails instead of passing silently.
  • One binary can also be registered under several names (deepfist_cw_model_test five times; test: green the sanitizer lane — scale TIMEOUTs under sanitizers; feed rtl_capture_worker_test in lockstep. Principle VIII. #6208 adds rtl_capture_worker_slow_device_test on rtl_capture_worker_test). A filter built from the target name runs only one, and --no-tests=error can't notice, so the names come from grep -n 'COMMAND <target>' tests/tests.cmake.
  • Test selection follows an OBJECT library or …_SOURCES list to its consumers. After build: compile shared test sources once, and default RelWithDebInfo to -g1 (#6214) #6217 the logging sources appear once in tests.cmake, and files compiled into aethercore reach every test that links it.
  • An unfiltered ctest reports unbuilt tests as "Not Run". That means the -R filter is missing, not that a test failed.

/pr-land step 5: the old cmake --build build --parallel (full tree) becomes the same lean configuration and named targets. The selection also includes the tests covering the findings being fixed.

Evidence for the lean recipe

Same machine and toolchain, on main at this branch's base. Built AetherSDR plus radio_setup_label_theme_token_test, sim_backend_test and frameless_resizer_test:

	Full tree (before)	Lean build (this PR)
Build directory	84 GB	1.3 GB
AetherSDR	832 MB	144 MB (+ 134 MB libaethercore.so, shared by all targets)
radio_setup_label_theme_token_test	397 MB	18 MB
Cold build, all ccache misses	—	219 s

• ldd build/AetherSDR resolves libaethercore.so from the build tree, so the shared core really is the one in use.
• ctest -R '^(radio_setup_label_theme_token_test|sim_backend_test|frameless_resizer_test)$': 100% of 3 passed.

For reference, a second worktree at the same commit, building the same targets, took 41 s with a 99.4% ccache hit rate (1094/1101). That needs a local ccache setting (base_dir set and hash_dir=false, so absolute worktree paths don't split the cache key). It is a per-machine choice, so it is not in the skill text.

Also: a test that tested nothing under -DNDEBUG

tests/digital_voice_mode_registry_test.c does all of its checking inside plain assert(), with the calls under test inside them. RelWithDebInfo and Release define NDEBUG, so there it compiled to an empty main and passed whatever the registry did. That covers CMake's default RelWithDebInfo flags, CI's build and TSan lanes, and this PR's recipe; only the Debug (ASan) lane ever ran it.

#undef NDEBUG before <assert.h> fixes it, since <assert.h> re-reads NDEBUG on each inclusion. Built at -O2 -g1 -DNDEBUG, with the first assertion inverted on purpose:

	Unmodified test	First assertion inverted
Before	100% passed	100% passed (the check never ran)
After	100% passed	0% passed, 1 failed

It is the only test that includes <assert.h> without handling NDEBUG. The automated code-review pass found it while checking this PR's recipe, and it is folded in here by maintainer decision.

Not in this PR

• Per-test recompiles: build: compile shared test sources once, and default RelWithDebInfo to -g1 (#6214) #6217 compiles the settings store and logging sources once; the widget sources (DragValuePopup.cpp ×40 and others) remain open in build(tests): factor the app sources tests.cmake recompiles per target into shared test libraries #6214.
• No build-logic change. CMakeLists.txt and BUILD-OPTIONS.md change only AETHER_SHARED_CORE's help string, comment and table row: it now names local review builds as well as the sanitizer lane. tests/tests.cmake, CI and release builds are untouched, and shipping builds keep the static core.

🤖 Generated with Claude Code

[aethersdr-agent]

Issue fit

No linked issue, and none is required: GOVERNANCE.md:115 lists "Documentation additions and corrections" under What does NOT require an RFC, and this changes only skill prose. Reviewed against the PR's own stated intent — replace the implicit full-tree all build in /pr-review and /pr-land with a lean, named-target build — and it delivers exactly that. The body's measurement table is unusually good for a docs PR; most of what I checked below was checking it, not the prose.

One note on the body: the "Caveats, stated in the skill" bullet about WDSP is wrong in the same way the skill text is (finding 1), so the error is in both places and the body bullet needs the same correction.

Scope

File	What it changes	Claimed by title/body?	Verdict
.claude/skills/pr-review/SKILL.md	Adds the "Build only what you run" block (457-489); rewords the merge-base rule at 500-502 to build the same targets rather than a clean full tree	Yes — body describes both	In scope
.claude/skills/pr-land/SKILL.md	Replaces the full-tree cmake --build build --parallel at 442-452 with the lean configure + named targets, cross-referencing /pr-review step 7	Yes — body's "/pr-land step 5" section	In scope

Everything in the diff is explained by the stated intent. No source, CMake, CI, or test file is touched — I confirmed the body's "No change to CMakeLists.txt, tests/tests.cmake, CI or release builds" claim against the file list, and it holds. No new public surface, no CHANGELOG.md entry (correct), no deleted guard. The - lines remove only the two superseded build commands.

Blockers

None.

Nits

1. .claude/skills/pr-review/SKILL.md:475-476 — the WDSP caveat inverts its source and prescribes a needless full reconfigure.

A test that calls WDSP directly does not
link against the shared core by design; reconfigure static for that one.

The comment this paraphrases (CMakeLists.txt:156-160) says something different, and gives a different remedy:

A test that calls WDSP directly while linking only aethercore links against
the static core and fails here; go through core/dsp/WdspChannel.h.

Two problems. "Does not link against the shared core by design" reads as a graceful fallback; the mechanism described in CMake is a link failure — WDSP is a PRIVATE dependency (CMakeLists.txt:1616) built with hidden visibility, so its symbols stop at the shared-library boundary. And the sanctioned fix is to route through WdspChannel.h, not to reconfigure the tree static — which costs the full rebuild this PR exists to avoid.

In practice the case does not arise today: both tests that reach WDSP directly already link it explicitly — tests/tests.cmake:1570 (wdsp_allocation_scope_test PRIVATE aether_wdsp Threads::Threads) and :1575 (rtl_receive_pipeline_test PRIVATE aethercore aether_wdsp Qt6::Core). And .github/workflows/sanitizers.yml builds the whole tree (cmake --build build --parallel, line 162) with shared_core: "ON" in both matrix rows (lines 42, 75), with no exclusion for either test. So the entire tree, those two included, builds shared in CI today. Suggested replacement:

finding could depend on link mode. A test that reaches into WDSP while linking
only `aethercore` fails to link under a shared core: WDSP is a PRIVATE
dependency built with hidden visibility, so its symbols stop at the library
boundary. Both such tests in the tree already link `aether_wdsp` explicitly
(`tests/tests.cmake:1570`, `:1575`); on a new one, go through
`core/dsp/WdspChannel.h` or link `aether_wdsp` rather than reconfiguring static.

2. .claude/skills/pr-review/SKILL.md:466 — the anchored -R assumes test name == target name, which is true 568 times out of 569.

ctest --test-dir build -R '^(test_a|test_b)$' --output-on-failure

The recipe reuses one token for both --target and the anchored -R. Across tests/tests.cmake that holds for every registration but one: add_test(NAME settings_browser_dialog COMMAND settings_browser_dialog_test) — building settings_browser_dialog_test and filtering ^settings_browser_dialog_test$ matches nothing. Worth half a sentence that names usually equal targets and that ctest --test-dir build -N resolves it when -R comes back empty, since an empty filter looks like a pass.

3. BUILD-OPTIONS.md:152 and CMakeLists.txt:151 still scope AETHER_SHARED_CORE to "Sanitizer CI only." The option's help string reads "Build libaethercore as a shared library (sanitizer CI only)" and the options table says "Sanitizer CI only, never for releases." This PR makes it the standard local review configuration, which I think is fine — a review build is not a release, and the skill discloses the link-mode caveat honestly. But the option's own documentation now understates where it is used; a line in the BUILD-OPTIONS.md row mentioning local review builds would keep the two in sync. Non-blocking, and arguably a maintainer's call on whether to broaden the sanctioned use in writing.

4. .claude/skills/pr-review/SKILL.md:471-474 — a dangling "then" after the Windows aside. "Both are for Linux and macOS: on Windows, drop the shared core … and the GCC/Clang flags. The app then loads the core as a shared library, which is not how it ships." The antecedent is the shared-core configure two sentences back, but following the Windows sentence it can read as the Windows outcome — which is the opposite of true. Moving the Windows sentence after the link-mode caveat fixes it.

What I tried to break

• "step 7" cross-reference in /pr-land:444 — held. The new block sits at pr-review/SKILL.md:457-489, inside ## 7. Code-quality audit (427) and before ## 8 (511). A stale section number was my first guess here and it is correct.
• Invented or wrong compiler flags — held, and this is the strongest thing in the PR. The flag pair is verbatim what CI already uses: "-DCMAKE_C_FLAGS_RELWITHDEBINFO=-O2 -g1 -DNDEBUG" / CXX appear identically at ci.yml:270-271 and full-suite.yml:103-104. RelWithDebInfo is canon's own build type (AGENTS.md:189, BUILD-OPTIONS.md:20), so the added -DNDEBUG is not a new hazard — I went looking for tests whose assertions it would silently disable and the exposure is pre-existing to canon, not introduced here.
• Multi-target --target A B C — held. That form needs CMake ≥ 3.15; CMakeLists.txt:1 pins cmake_minimum_required(VERSION 3.25), so the central command cannot fail on a floor install.
• Windows claim and target name — held. CMakeLists.txt:153-155 is message(FATAL_ERROR "AETHER_SHARED_CORE is not supported on Windows"), so "it is a configure error" is exact; add_executable(AetherSDR exists at CMakeLists.txt:1636.
• A stale sibling build recipe left behind — held. Grepping both skills for cmake --build, cmake -S, cmake -B and ctest returns only the lines this PR added or rewrote; no third place still tells the agent to build all.
• Scale claims — the "several hundred test executables" figure checks out (569 add_test registrations over 535 unique executables, 651 add_executable calls in tests.cmake), and the body's own 397 MB → 18 MB measurement does support "roughly twentyfold." I could not re-run the measurements: I am headless with no build, so the GB figures, the 219 s cold build, and the ldd result are taken on the body's word and not reproduced.
• CI — all required checks green on b2aafd9, including Static checks and Sanitizer option configures (static-checks.yml:144-153 configures AETHER_SHARED_CORE=ON and asserts a libaethercore.so rule, which is the probe most relevant to this recipe). Analyze (python) is cancelled, not failed. No socket test is added or removed by this PR.

Every finding above is reasoned from reading the checkout at /tmp/aetherclaude/pr-6215 and the CI configuration — none is a runtime observation.

Recommendation

Approve with nits. The recipe is sound, and the two claims I most expected to be wrong — the cross-reference and the flag set — both check out against CI's own configuration rather than against the author's assertion. Finding 1 is the one worth fixing before merge: it is a docs PR whose job is to be followed literally, and that sentence sends a reviewer into a full static reconfigure for a case that no longer exists in the tree. The replacement text is above; findings 2-4 can ride along or be skipped. Thanks for the measurement table — it made this reviewable.

---

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

[ten9876]

Reviewed b2aafd9 against its stated documentation intent; there is no closing issue. Documentation corrections need no RFC under GOVERNANCE.md. Recommendation: Approve with nits, subject to the usual CODEOWNERS/check requirements. No blockers.

Scope	Change	Claimed?	Verdict
.claude/skills/pr-review/SKILL.md	Lean shared-core configuration, named targets and matching base comparison	Yes	In scope
.claude/skills/pr-land/SKILL.md	Uses that recipe for remediation verification	Yes	In scope

The two commits contain only these skill edits. No runtime, release, CI, test registration, autonomy, or CHANGELOG change is bundled.

Nits

1. .claude/skills/pr-review/SKILL.md:475 — the WDSP caveat sends reviewers into an unnecessary static reconfigure. wdsp_allocation_scope_test already links aether_wdsp directly, and rtl_receive_pipeline_test links both aethercore and aether_wdsp. The failure described in CMake is specifically direct WDSP calls while linking only shared aethercore; existing direct-call tests already handle it. Replace the sentence with: “Direct WDSP calls cannot rely on shared aethercore exporting its private WDSP dependency. Existing direct-call tests link aether_wdsp explicitly; prefer the WdspChannel seam for a new engine test.” This confirms the earlier bot's substantive caveat finding.

2. .claude/skills/pr-review/SKILL.md:466 and .claude/skills/pr-land/SKILL.md:446 — distinguish executable targets from registered CTest names and fail on an empty filter. settings_browser_dialog_test is the target, but settings_browser_dialog is its CTest name. I ran the recipe's anchored target-name filter: it printed “No tests were found!!!” and exited 0. Suggested commands:

cmake --build build --parallel --target settings_browser_dialog_test
ctest --test-dir build -R '^settings_browser_dialog$' --no-tests=error --output-on-failure

State that the filter uses add_test(NAME ...), which may differ from the build target, and add --no-tests=error to both recipes. This is a robustness nit rather than a workflow-policy violation: canon's mandatory flag rule applies to workflow invocations.

Verification / attacks survived

• Configured the exact head with shared core and the stated -O2 -g1 -DNDEBUG C/C++ flags; built the three socket-free targets named in the body's measurement: radio_setup_label_theme_token_test, sim_backend_test, and frameless_resizer_test. All three passed with the desktop theme plugin disabled for this sandbox. ldd confirms all three load the same scratch libaethercore.so.
• Checked the Windows configure refusal, CMake's multi-target support, actual core/WDSP link rules, the /pr-land step reference and the matching flag recipe in CI. No socket-owning test is introduced or modified. No app or live-radio session was needed for these documentation claims.
• This is a focused build, not the full tree or the author's AetherSDR-plus-tests benchmark; I have not reproduced the 84 GB, twentyfold, cold-build timing, or ccache measurements. ASR and RADE were disabled in my local build; those options do not validate their own paths here.
• Required CI build/macOS/Windows/static checks passed on this head. Their runs were created at 21:03 UTC; current main's tip 173c0d485 was committed at 21:22:46 UTC, so those greens cover an older integration. The CodeQL Python job was cancelled and its workflow reports failure; this is not an observed source defect in this docs-only diff.
• An invocable automated code-review skill was unavailable; manual review and independent checks above were used. The relevant bot claims were checked against the actual head.

Posted as COMMENT because the authenticated reviewer is also this PR's author; GitHub does not permit self-approval.

[ten9876]

Reviewed b2aafd9 as a pair with #6217 (head 7154f88). Both were merged onto current main 173c0d485, in both orders, and an affected subset was built. Recommendation: approve with nits. No blockers. I'm posting this as COMMENT because the reviewing account is the author.

Issue fit: there is no linked issue, and none is needed: GOVERNANCE.md exempts documentation corrections from the RFC process. The PR does what it says. Both skills now use a lean, shared-core build of named targets instead of all.

Scope

File	Change	Claimed?	Verdict
.claude/skills/pr-review/SKILL.md	"Build only what you run" block; merge-base rule builds the same targets	Yes	In scope
.claude/skills/pr-land/SKILL.md	Step 5 uses the same lean recipe	Yes	In scope

Everything in the diff is explained by the body. There is no CMake, CI, test or CHANGELOG change.

Pair findings (new; neither earlier review could see these)

1. .claude/skills/pr-review/SKILL.md:479-481 (and pr-land/SKILL.md:447-448): once #6217 lands, the test-selection grep misses the logging consumers. It already misses the settings consumers.

The rule is "the ones whose tests/tests.cmake block compiles a file the PR touches (grep -n 'Touched.cpp' tests/tests.cmake)". These are non-comment hits for the source path, counted on each tree:

File	main	main + #6217
LogManager.cpp	63	1 (the add_library(aether_test_log_manager OBJECT …) line)
AsyncLogWriter.cpp	65	1
AppSettings.cpp	0 (behind ${AETHER_SETTINGS_SOURCES})	0

Take a PR that touches LogManager.cpp. A reviewer who follows this text literally after #6217 builds none of its 63 consumers. Separately, build.ninja shows that 270 of 626 executables link aethercore, so no grep of tests.cmake sees any consumer of a file compiled into the core. That limit already exists without this PR, but the skill should say so. Suggested replacement for the sentence:

Pick the tests from the diff: the ones the PR adds or modifies, the ones its
body names, and the ones whose `tests/tests.cmake` block compiles a file the
PR touches (`grep -n 'Touched.cpp' tests/tests.cmake`). When the hit is an
`OBJECT` library or a `set(..._SOURCES` list rather than a test, grep again
for that target or variable name. A file compiled into `aethercore` reaches
every test that links it, so no grep finds them all; pick the ones that
exercise the touched code.

Severity is a nit on this PR alone, and the one to fix if #6217 lands.

2. Non-blocking: after #6217, the explicit -g1 flags in the recipe (SKILL.md:461-463) do nothing on GCC/Clang. I confirmed this with a fresh configure of the merged tree and no flags: the cache holds CMAKE_CXX_FLAGS_RELWITHDEBINFO=-O2 -g1 -DNDEBUG. They are harmless, and keeping them keeps the recipe correct on a checkout without #6217. Mentioned only so nobody reads them as load-bearing.

Nits from the earlier reviews, still open at this head (not re-argued)

• SKILL.md:475-476: the WDSP sentence still prescribes a static reconfigure. Both direct-WDSP tests already link aether_wdsp; route new ones through WdspChannel.h.
• SKILL.md:466, pr-land/SKILL.md:446: -R matches the add_test(NAME …) name, not the target name (settings_browser_dialog_test → settings_browser_dialog). Add --no-tests=error, because an empty filter exits 0.
• SKILL.md:472-474: after the Windows sentence, "The app then loads the core as a shared library" reads as if it describes Windows.
• CMakeLists.txt:151 and BUILD-OPTIONS.md:172 still describe AETHER_SHARED_CORE as "sanitizer CI only". This PR makes it the standard review configuration.

What I tried to break

• Pair merge: main + #6217 + #6215 and main + #6215 + #6217 both merge cleanly and give an identical tree. The two pr-review/SKILL.md hunks (the step 5 CMake-contract line and the step 7 block) do not overlap, so landing order doesn't matter.
• The recipe on the merged tree: I configured exactly as written (shared core, -g1, plus local mold, ccache and Qt 6.12; ASR and RADE off). I then built split_qsy_settings_test log_manager_filter_rules_test async_log_writer_test theme_manager_test transmit_model_test sim_backend_test. That took 42 s, and the build dir is 428 MB. ctest -R '^(…)$' --no-tests=error: 6/6 passed. ldd build/sim_backend_test resolves libaethercore.so from the build tree. The largest of these test binaries is 10 MB.
• A shared core next to #6217's object libraries: from build.ninja, 77 executables link the new object libraries, and 0 of them also link libaethercore/libaetherdesktop_support. So 6215's recipe does not create the second-AppSettings-singleton case that #6217's comment warns about.
• Not reproduced: the 84 GB full-tree figure and the 219 s / 41 s timings. I did not build all.
• CI: the earliest run on this head was created at 21:02:58Z, before main's tip 173c0d485 (21:22:46Z). The green therefore describes an older merge. My clean merge onto current main stands in for it on this docs-only diff. Analyze (python) is cancelled, not failed.
• Bridge: not applicable. There is nothing to drive in a skills-text change.