Fixes flagged by code review

[jomosh]

This pull request is to address issues highlighted by a code review for a previous PR.

The focused review on the previous branch found a few high-signal risks worth addressing:

src/process/clutter/WienerHopf.cpp: nBins is computed as delayMax - delayMin but the class contract/documentation expects inclusive bins (+1), and a[0] is still accessed without guarding the zero-bin case.
src/process/clutter/WienerHopf.cpp: no validation for delayMax < delayMin; with unsigned nBins this can underflow and trigger huge allocations.
src/capture/rspduo/RspDuo.cpp: callback file writes use saveIqFileLocal->write(...) without saveIqFileMutex, while Source::close_file() closes under mutex; this leaves a possible write/close race.
src/process/clutter/WienerHopf.cpp: destructor destroys FFTW plans but does not free heap arrays allocated with new[].

I couldn’t run full/targeted binaries in this sandbox because the configured toolchain/build deps are missing (/scripts/buildsystems/vcpkg.cmake, compilers/make), so the review above is static-analysis based.

[Copilot]

Pull request overview

This PR addresses several correctness and safety issues previously flagged in the real-time radar pipeline’s clutter filter and RspDuo capture path, and adds a focused unit test to prevent regressions.

Changes:

• Fixes WienerHopf bin count to be inclusive (delayMax - delayMin + 1) and adds constructor input validation, plus destructor cleanup for FFTW resources and heap arrays.
• Adds a mutex-protected file-write path in RspDuo stream callback to avoid write/close races.
• Introduces a Catch2 unit test target for WienerHopf and wires it into CMake/CTest.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/unit/process/clutter/TestWienerHopf.cpp	Adds unit coverage for constructor guards and short-input behavior.
src/process/clutter/WienerHopf.h	Initializes FFTW plan/pointer members to nullptr and includes <complex> for member declarations.
src/process/clutter/WienerHopf.cpp	Adds constructor guards, fixes inclusive nBins, frees allocated arrays, and null-checks FFTW plan destruction.
src/capture/rspduo/RspDuo.cpp	Wraps file writes with saveIqFileMutex and checks is_open() under lock.
CMakeLists.txt	Adds testWienerHopf executable and registers it with CTest.

[jomosh]

@copilot a fix been made after your review. re-run your review, but don't fix anything directly. just highlight it for me.

[Copilot]

@copilot a fix been made after your review. re-run your review, but don't fix anything directly. just highlight it for me.

Re-ran review on the latest branch state.

• The prior nBins/underflow/OOB concern appears addressed (binCount computed in int64_t with nBins <= nSamples guard).
• One remaining high-signal risk: WienerHopf still does not validate FFTW size/plan creation safety. Plans are created without null checks (src/process/clutter/WienerHopf.cpp:55-68) and then executed unconditionally in process() (:133-134,141,172,213-214,221). Under very large configs or allocation pressure this can crash.
• Coverage gap: no test currently exercises the new overflow guard path (binCount > uint32_t max) at src/process/clutter/WienerHopf.cpp:23-27.

No fixes applied, per your request.

[jomosh]

@copilot I've added some fixes after your last code review. Re-run your review, but don't fix directly into code. just highlight the issues to me.

[Copilot]

@copilot I've added some fixes after your last code review. Re-run your review, but don't fix directly into code. just highlight the issues to me.

Re-ran review on the latest fixes.

Remaining high-signal issue:

• WienerHopf still lacks bounds checks for FFT length arithmetic. nBins + nSamples + 1 is computed from uint32_t values with no overflow/upper-bound validation before allocations/FFTW plans (src/process/clutter/WienerHopf.cpp:52-54, 63-68, 258-279).
• fftw_plan_dft_1d takes int; nSamples and the composite length can exceed INT_MAX and violate FFTW size contract (src/process/clutter/WienerHopf.cpp:55-68).

What looks fixed:

• Plan-null checks/cleanup and throw on plan creation failure are now present.
• Overflow-guard test for binCount (int32 min/max) was added in test/unit/process/clutter/TestWienerHopf.cpp.

No direct code changes made.

[jomosh]

@copilot a few fixes have been applied after your last review to address issues you've highlighted. re-run your review and don't fix directly in code. just let me know.

[Copilot]

@copilot a few fixes have been applied after your last review to address issues you've highlighted. re-run your review and don't fix directly in code. just let me know.

Re-ran review on the latest commit.

What looks fixed:

• FFT length bounds/FFTW int limit checks are now added.
• Filter length overflow handling and related constructor tests were added.

Remaining high-signal issues:

• WienerHopf::process still has an index math bug when delayMin > 0 because i is uint32_t and delayMin is int32_t in ((((i - delayMin) % nSamples) + nSamples) % nSamples) (src/process/clutter/WienerHopf.cpp). This can silently misalign reference samples.
• WienerHopf is copyable while owning raw pointers/FFTW plans; accidental copy can cause double free/destroy (src/process/clutter/WienerHopf.h + destructor in src/process/clutter/WienerHopf.cpp).
• process() does not guard x/y against null pointers before dereference.

No code changes applied.

[jomosh]

@copilot a few fixes have been applied after your last review to address issues you've highlighted. re-run your review and don't fix directly in code. just let me know.

[Copilot]

@copilot a few fixes have been applied after your last review to address issues you've highlighted. re-run your review and don't fix directly in code. just let me know.

Re-ran review on the latest branch state.

Most previously flagged items now look addressed (index math, null-input guard, and copy/move deletion are in place).

One remaining issue to highlight:

• WienerHopf constructor still does multiple raw new[] allocations in sequence (src/process/clutter/WienerHopf.cpp:63-71). If one later allocation throws, earlier allocations are leaked because constructor unwind won’t run the destructor for a not-fully-constructed object. Using RAII-owned buffers (or staged cleanup wrappers) would avoid this exception-safety leak path.

I didn’t apply any code changes.

[jomosh]

@copilot a few fixes have been applied after your last review to address issues you've highlighted. re-run your review and don't fix directly in code. just let me know.

[Copilot]

@copilot a few fixes have been applied after your last review to address issues you've highlighted. re-run your review and don't fix directly in code. just let me know.

Re-ran review on the latest commit.

I don’t see a remaining high-signal issue in the WienerHopf changes now. The constructor allocation path is now exception-safe with RAII locals, and the previously flagged index/null/copyability and bounds checks still look correct.

No code changes applied.