GH-144: Guard genuine division/NaN hazards surfaced by the #40 epsilon audit

[ehkropf]

Fixes #144. Depends on #40 (merged in #145).

The #40 epsilon-selection audit surfaced two genuine, currently unguarded numerical hazards (distinct from #40, which centralized tolerance constants without changing behavior). This PR adds the guards + tests.

1. Grid factories divide by zero for single-line grids

Grid spacing is (max - min) / (count - 1), so a line count of 1 computes 0.0 / 0.0 → NaN, silently poisoning the whole grid.

• New Grid::validateLineCount(name, value) helper throws std::invalid_argument for counts < 2.
• Wired into all three factories (createPolarGrid, createCartesianGrid, createParametricGrid) on every divided axis.
• Also wired into setParameter() for all six count keys — that path reaches the same division via regenerate(), so guarding only the factories would leave a hole (per review decision).
• Decision: reject single-line grids, do not special-case. Hard floor is >= 2 (the true div-by-zero floor); counts 2..9 still compute fine and stay allowed.

2. RootFinder has no NaN/inf guards

newton and ternarySearch never checked std::isfinite, so a NaN spun Newton to maxIterations (then threw the wrong ConvergenceError) and ternary search returned a meaningless midpoint.

• newton (RootFinder.h) and ternarySearch (RootFinder.cpp) now throw ConvergenceError on the first non-finite evaluation.
• std::abs of a std::complex yields a real magnitude, so the Newton guard covers both the double and complex instantiations.

Tests

• GridTest: each factory with a count of 1 on each axis → std::invalid_argument; count of 2 succeeds; setParameter count of 1 → throws.
• RootFinderTest: NaN/inf callables (real + complex) → ConvergenceError. The Newton NaN tests assert the fast-fail message to distinguish the new guard from the pre-existing max-iteration fallback (both throw ConvergenceError, so EXPECT_THROW alone wouldn't verify the guard).
• Regression-verified by reverting each guard and confirming the new tests fail, then restoring.

314/314 tests pass on the real /opt/local toolchain (Release).

Out of scope (own issues if pursued, per #144)

• AnalyticBoundaryComponent::findParameterization atan2 fallback bug.
• CGSolver unguarded beta = rsnew/rsold / restart improvement-rate divisions.

🤖 Generated with Claude Code

[ehkropf]

Multi-Agent PR Review (code / tests / error-handling / comments)

Verdict: No critical or merge-blocking issues. The guards are correctly implemented, correctly typed (std::invalid_argument for validation, ConvergenceError for solver runtime), and correctly placed before the hazardous use. Four review agents converged — the code itself is clean; the substantive findings are about scope and integration, not correctness.

Critical Issues

None.

Important Issues

1. The Grid guards harden a class with zero production callers — the live GUI div path is untouched.
Grid is instantiated only in tests/grids.cpp. The user-facing grid generation lives in src/gui/VisualizationPanel.cpp (generateSourceGrid), driven by the "Grid Density" slider (MainWindow.cpp:254). That path has its own divisions and a weaker density > 0 guard (VisualizationPanel.cpp:104). The slider is currently clamped to [4,16] so there's no live bug — but the issue's div-by-zero goal isn't met on the path a user can actually drive. Worth a note so reviewers don't assume the GUI is now protected.

2. The new non-finite ConvergenceError is swallowed by existing broad catch blocks.
BoundaryComponent.cpp:76, InvertedEllipseComponent.cpp:96, SplineBoundaryComponent.cpp:146 all catch (const ConvergenceError&) without inspecting .what(). A NaN objective now fast-fails (good — better than spinning to maxIterations) but lands in the same fallback as ordinary non-convergence — including the documented atan2 silent-wrong-value bug. The NaN is caught faster but not surfaced distinctly. A NonFiniteEvaluationError : ConvergenceError subclass would let catch sites distinguish "geometry produced NaN" from "didn't converge." Reasonable follow-up, not a blocker.

3. Parametric setParameter guards are untested.
Grid.cpp:488-489 guards numULines/numVLines, but SetParameterRejectsSingleLineCount only covers polar + cartesian (4 of 6 guarded keys). Each else if is an independent call site — a regression deleting the parametric guard would pass the full suite today. Low-cost fix: extend the test with a parametric case.

Suggestions

• Doc inaccuracy: Grid.h:71 claims spacing is uniformly (max-min)/(count-1), but polar radial-angle division is /count (Grid.cpp:156,201). The >=2 guard keeps it safe, but the comment over-generalizes. (Flagged independently by three agents.)
• setParameter contract: returns bool but now throws on bad values (unrecognized name → false, bad value → throw). No live caller checks the return, but add @throws std::invalid_argument to its docstring.
• ternarySearch inf case untested — only NaN is exercised; std::isfinite treats them identically, so low value.
• Comment polish: clarify the inf-derivative Newton test exercises a silent-return failure mode (distinct from the "spin to maxIterations" the blanket comment describes); tighten the Grid.h @brief since the guard also rejects 0/negative, not just single-line.

Strengths

• Guard placement verified correct in both newton and ternarySearch — fires before the tolerance/comparison logic and before the fx/dfx division.
• Complex template instantiation handled soundly (std::isfinite(std::abs(fx))) and explicitly tested (NewtonRejectsNonFiniteComplexValue) — the std::abs wrapper is load-bearing since std::isfinite rejects std::complex directly.
• Message-string assertions are justified, not brittle — they're the only way to distinguish the new guard from the same-typed max-iteration fallback.
• Regression comments correctly use past tense per CLAUDE.md; no line-number references in comments.
• StatusManager null-guard pattern correctly N/A — neither class holds a StatusManager, so throwing directly is the right (and only) choice.

Recommended Action

1. Optional before merge: add the parametric setParameter test (Fix Theodorsen implementation #3) and fix the Grid.h:71 spacing-formula comment — both cheap, both flagged by multiple agents.
2. Note in PR description: the live GUI grid path (Domain representation #1) and the catch-block swallowing (FFT utilities #2) are not addressed here — consistent with this PR's "out of scope, own issues if pursued" framing, but worth making explicit.
3. The remaining suggestions are documentation polish.

🤖 Generated with Claude Code

[ehkropf]

Response to multi-agent review (commit b69907e)

Thanks — verified every finding against the code; all accurate. Triage below.

Applied (cheap, in-scope)

• Fix Theodorsen implementation #3 Parametric setParameter test gap — SetParameterRejectsSingleLineCount only covered polar + cartesian (4 of 6 guarded keys). Added a parametric case (numULines/numVLines); each else if is an independent call site, so a deleted parametric guard would otherwise pass the suite.
• Grid.h spacing-formula doc — reworded. Polar radial-angle division is /count (Grid.cpp:156,201), not /(count-1), so the comment no longer over-generalizes; @brief now states it rejects any count < 2 (covers 0/negative).
• setParameter contract — added @throws std::invalid_argument to the docstring.
• inf-derivative Newton test comment — clarified it exercises the silent-return mode (fx/dfx collapses to 0 → returns the initial guess), distinct from the NaN spin-to-maxIterations the block comment describes.

All 314/314 tests green on the real /opt/local toolchain (Release).

Confirmed out of scope (not addressed here, consistent with #144's framing)

• Domain representation #1 Live GUI grid path — confirmed VisualizationPanel::generateSourceGrid is a separate path dividing by m_gridDensity / m_gridDensity/2, with the slider clamped to [4,16] (MainWindow.cpp:254) and a density > 0 guard (VisualizationPanel.cpp:104). No live div-by-zero; the Grid class Guard genuine division/NaN hazards surfaced by the #40 epsilon audit #144 targets has no production callers. Worth its own issue if the GUI path's guard should be hardened to match.
• FFT utilities #2 Broad catch (const ConvergenceError&) at BoundaryComponent.cpp:76, InvertedEllipseComponent.cpp:96, SplineBoundaryComponent.cpp:146 — the new fast-fail lands in the same fallback as ordinary non-convergence (incl. the documented atan2 silent-wrong-value bug). A NonFiniteEvaluationError : ConvergenceError subclass would let those sites distinguish "geometry produced NaN" from "didn't converge." Reasonable follow-up.

Skipped (low value, review concurred)

• ternarySearch inf case — std::isfinite treats NaN/inf identically and NaN is already exercised.

🤖 Generated with Claude Code