ConformalLabpp/doc/reviewer/test-coverage-error-handling-audit-2026-05-31.md

Test-Coverage & Error-Handling Audit — ConformalLabpp

Date: 2026-05-31 Auditor: External reviewer (Claude Opus 4.8) Scope: code/include/ (31 headers), code/tests/ (26 fast + 246 CGAL tests) Focus: Test coverage gaps and error-handling correctness — NOT a port-faithfulness review.

This document is self-contained. A new session can pick up any finding below and act on it without prior context. Each finding includes:

• exact file path + line numbers (verified by direct file read)
• a minimal reproduction of the problematic code
• the recommended fix
• acceptance criteria for "done"

Status legend: 🔴 Critical · 🟡 Important · 🔵 Hint / nice-to-have

Note: This audit is complementary to external-audit-2026-05-30.md (which covers port-faithfulness bugs). There is no overlap in findings — this one is strictly about test coverage and error-handling robustness.

---

How to read this document in a new session

# 1. Build the CGAL test suite (needed to verify test-related findings)
cmake -S code -B build-cgal -DWITH_CGAL_TESTS=ON
cmake --build build-cgal --target conformallab_cgal_tests -j$(nproc)

# 2. Run the full suite before making any change (establish baseline)
ctest --test-dir build-cgal -R '^cgal\.' --output-on-failure
# Expected: 246 passed, 0 failed

# 3. Fast suite (for coverage-script findings)
cmake -S code -B build && cmake --build build --target conformallab_tests
ctest --test-dir build --output-on-failure   # Expected: 26 passed

# 4. Pick a finding below, apply the fix, re-run ctest, then commit.

The Java reference implementation (for any "what should the behavior be" question):

/Users/tarikmoussa/Desktop/conformallab/src/de/varylab/discreteconformal/

---

Summary table

ID	Sev	Title	Primary file
C1	🔴	solve_linear_system discards its ok flag — double solver failure is invisible	code/include/newton_solver.hpp:107
C2	🔴	coverage.sh uses || true on every lcov command — script can never fail	scripts/quality/coverage.sh
C3	🔴	Coverage not CI-gated — no enforcement mechanism exists	CI config
I1	🟡	NewtonResult cannot distinguish MaxIter / LinearSolverFailed / LineSearchStalled	code/include/newton_solver.hpp:51
I2	🟡	serialization.hpp: 4 throw statements, 0 negative tests	code/include/serialization.hpp
I3	🟡	load_mesh non-triangle-mesh throw path untested	code/tests/cgal/test_mesh_io.cpp
I4	🟡	spherical_hessian.hpp:114 throw has no EXPECT_THROW test	code/tests/cgal/test_spherical_hessian.cpp
I5	🟡	Coverage measures only the 26 fast tests (~9.6% of suite)	scripts/quality/coverage.sh
H1	🔵	res.iterations is wrong/stale when solver breaks on !ok	code/include/newton_solver.hpp (×5)
H2	🔵	5 near-identical Newton loops — maintenance risk	code/include/newton_solver.hpp
H3	🔵	enforce_gauss_bonnet does not report magnitude of applied correction	code/include/gauss_bonnet.hpp
H4	🔵	reduce_to_fundamental_domain: boundary Im(τ)==0.0 untested	code/tests/cgal/test_phase7.cpp
H5	🔵	Degenerate-triangle path in Newton solver not integration-tested	code/include/euclidean_hessian.hpp:85
H6	🔵	2 ARM64 tests marked @Ignore — platform-specific coverage gap	test suite

---

FINDING-C1 — 🔴 solve_linear_system silently discards its failure flag

Location

code/include/newton_solver.hpp lines 107–114

Problem

The public linear-system solver computes ok from solve_with_fallback but never returns or surfaces it. On double solver failure (both LDLT and SparseQR fail), the function returns Eigen::VectorXd::Zero(n) — indistinguishable from a genuine zero solution.

inline Eigen::VectorXd solve_linear_system(
    const Eigen::SparseMatrix<double>& A,
    const Eigen::VectorXd&             rhs,
    bool*                              fallback_used = nullptr)
{
    bool ok = false;
    return detail::solve_with_fallback(A, rhs, ok, fallback_used);
    //                                            ^^ `ok` computed, then dropped
}

This function is documented as a public primitive ("Exposing it publicly lets callers reuse the logic"). A downstream caller has no way to detect that the solve failed.

Fix

Add an optional bool* ok out-parameter (mirroring fallback_used), or return a small struct { VectorXd x; bool ok; }. Prefer the out-parameter to preserve the existing return type and call sites.

inline Eigen::VectorXd solve_linear_system(
    const Eigen::SparseMatrix<double>& A,
    const Eigen::VectorXd&             rhs,
    bool*                              fallback_used = nullptr,
    bool*                              ok            = nullptr)
{
    bool ok_local = false;
    auto x = detail::solve_with_fallback(A, rhs, ok_local, fallback_used);
    if (ok) *ok = ok_local;
    return x;
}

Acceptance criteria

• A new test in test_newton_solver.cpp constructs a genuinely unsolvable system (e.g. an inconsistent over-determined sparse system that defeats both LDLT and QR) and asserts ok == false.
• Existing 3 SparseQR-fallback tests still pass.

---

FINDING-C2 — 🔴 Coverage script can never fail

Location

scripts/quality/coverage.sh lines 95–112

Problem

Every lcov/genhtml invocation is suffixed with || true:

lcov --capture ... >/dev/null 2>&1 || true
lcov --extract ... --output-file "$BUILD_DIR/coverage.info" ... || true
genhtml ... || true

If lcov fails completely, coverage.info is empty, but the script still exits 0. The script is documented as a future CI quality gate ("once a coverage threshold is agreed … the gate can be a single line in cpp-tests.yml") — but as written it would always pass, defeating the purpose of a gate.

Fix

• Remove || true from the --capture and --extract steps; let real failures propagate (the set -euo pipefail at the top will then catch them).
• Keep tolerance ONLY for the known-noisy genhtml cosmetic step if necessary, but verify coverage.info is non-empty explicitly:

  [ -s "$BUILD_DIR/coverage.info" ] || { echo "FAIL: empty coverage trace" >&2; exit 1; }
  

Acceptance criteria

• Running the script with a deliberately broken lcov (e.g. PATH without gcov data) exits non-zero.
• A successful run still produces a non-empty coverage.info and exits 0.

---

FINDING-C3 — 🔴 Coverage is not CI-gated

Location

CI configuration (.gitea/workflows/cpp-tests.yml) + scripts/quality/coverage.sh:12-14

Problem

The script header states verbatim: "Local-only. Not gated in CI yet; once a coverage threshold is agreed with the reviewer (e.g. 80 %)…". There is no enforcement: coverage can silently regress to 0% with no signal.

Decision — agreed thresholds (2026-05-31)

For a numerical, header-only library the agreed gate is:

Metric	Gate	Rationale
Line coverage	80 %	Industry-standard, defensible, not punitive
Branch coverage	70 %	Numerical code has legitimately hard-to-hit branches (degeneracy checks, solver fallbacks, NaN paths)
Function coverage	90 %	Header-only → an untested function is an obvious gap

100 % is explicitly not the target — the degenerate-triangle and solver-failure branches are expected to be only partially covered.

Fix

1. First fix C2 (so the gate is trustworthy) and I5 (so it measures the right thing — a percentage over only the 26 fast tests is meaningless).
2. Establish a baseline by running the corrected script once.
3. If the measured baseline is below these gates, ratchet up: set the gate a few points below baseline now and raise it as coverage improves — do not block work on day one. If baseline already exceeds them, set the gates as above.
4. Add a --fail-under style check and wire it into the /quality-gates workflow.

Acceptance criteria

• coverage.sh accepts COVERAGE_MIN_LINE / COVERAGE_MIN_BRANCH / COVERAGE_MIN_FUNC env vars (defaults 80 / 70 / 90) and exits 1 when any is missed.
• The quality-gates workflow invokes it with these thresholds.

---

FINDING-I1 — 🟡 NewtonResult cannot tell why it failed

Location

code/include/newton_solver.hpp lines 51–56 (struct) + the 5 solver loops (newton_euclidean, newton_spherical, newton_hyper_ideal, newton_cp_euclidean, newton_inversive_distance)

Problem

All five solvers collapse three distinct failure modes into converged = false:

if (!ok) break;          // linear solver failed (numerically degenerate)
...
if (!improved) break;    // line search stalled (bad start / non-convex region)
...
// loop exits normally    // max_iter reached (might converge with more iterations)

A caller seeing converged == false cannot tell which happened, so cannot choose an appropriate recovery (re-seed start point vs. raise max_iter vs. report ill-posed problem).

Fix

Add to NewtonResult:

enum class Status { Converged, MaxIterReached, LinearSolverFailed, LineSearchStalled };
Status status = Status::MaxIterReached;

Set it at each exit point. Keep the existing converged bool for backward compatibility (converged == (status == Status::Converged)).

Acceptance criteria

• Each of the 4 statuses is asserted by at least one test (the existing convergence tests cover Converged; add targeted tests for the 3 failure modes).

---

FINDING-I2 — 🟡 serialization.hpp: 4 throws, 0 negative tests

Location

code/include/serialization.hpp lines 82, 96, 198, 245 (all throw std::runtime_error) Tests: code/tests/cgal/test_layout.cpp (only ASSERT_NO_THROW happy-path cases)

Problem

if (!ofs) throw std::runtime_error("Cannot write: " + path);   // L82, L198
if (!ifs) throw std::runtime_error("Cannot open: " + path);    // L96, L245

None of the four failure paths is exercised by a test.

Fix

Add to test_layout.cpp:

TEST(Serialization, ThrowsOnUnwritablePath) {
    EXPECT_THROW(save_result_json("/nonexistent_dir/x.json", res, ...), std::runtime_error);
}
TEST(Serialization, ThrowsOnMissingFileRead) {
    EXPECT_THROW(load_result_json("/tmp/does_not_exist_conflab.json"), std::runtime_error);
}

(Repeat for the XML variants to cover all four sites.)

Acceptance criteria

• All four throw sites are covered by EXPECT_THROW.

---

FINDING-I3 — 🟡 load_mesh non-triangle path untested

Location

code/include/mesh_io.hpp lines 61–64 (the !is_triangle_mesh throw) Test file: code/tests/cgal/test_mesh_io.cpp

Problem

test_mesh_io.cpp:111 tests the missing-file throw, but NOT the "file exists but is a quad/polygon mesh" throw — which is the more interesting guard (it protects every downstream functional from silent mis-handling).

Fix

Write a quad-only OFF file to /tmp, then:

TEST(MeshIO, LoadMeshThrowsOnNonTriangleMesh) {
    // write a single quad face OFF to a temp path ...
    EXPECT_THROW(load_mesh(quad_path), std::runtime_error);
}

Acceptance criteria

• The non-triangle branch in load_mesh is covered and asserts the runtime_error.

---

FINDING-I4 — 🟡 spherical_hessian throw untested

Location

code/include/spherical_hessian.hpp line 114 (throw std::logic_error) Test file: code/tests/cgal/test_spherical_hessian.cpp

Problem

The Euclidean analogue (euclidean_hessian.hpp:124) IS tested via test_euclidean_hessian.cpp:237 (EXPECT_THROW(..., std::logic_error)). The spherical equivalent has no such test.

Fix

Mirror the Euclidean test: assign an edge DOF (or whatever triggers the logic_error at L114 — read the surrounding code to confirm the precondition) and assert the throw.

Acceptance criteria

• EXPECT_THROW(spherical_hessian(mesh, x, maps), std::logic_error) present and passing.

---

FINDING-I5 — 🟡 Coverage measures only ~9.6% of the test suite

Location

scripts/quality/coverage.sh line 73

Problem

ctest -E "^cgal\." --output-on-failure   # excludes ALL 246 CGAL tests

Coverage is computed from the 26 fast tests only. The core solvers (newton_solver.hpp, euclidean_functional.hpp, gauss_bonnet.hpp, layout.hpp, …) are exercised almost exclusively by the CGAL suite, so the reported number does not reflect coverage of the project's actual core.

The script's rationale is legitimate (CGAL tests under coverage instrumentation need ~6 GB RAM). But the consequence — a coverage number that excludes the core — should at minimum be made explicit, and ideally addressed.

Fix options

• (a) Document loudly: rename the metric to "fast-suite coverage" everywhere it is reported, so nobody mistakes it for whole-project coverage.
• (b) Tiered coverage: add an opt-in WITH_CGAL_COVERAGE=1 mode that runs the CGAL suite under coverage on a high-memory machine (not the Pi runner), producing a separate coverage-cgal.info.
• (c) Merge traces: if (b) is run, lcov --add-tracefile can combine fast + CGAL into one report.

Acceptance criteria

• Decision recorded; if (a), the script + docs no longer imply whole-project coverage.

---

FINDING-H1 — 🔵 res.iterations stale on !ok break

Location

code/include/newton_solver.hpp — all 5 solver loops (e.g. lines 269–284 for Euclidean)

Problem

Eigen::VectorXd dx = detail::solve_with_fallback(H, -G, ok);
if (!ok) break;          // res.iterations still holds the PREVIOUS iteration's value
...
res.iterations = iter + 1;   // only reached if the solve + line search succeed

On a linear-solver break, res.iterations does not reflect the iteration at which the failure occurred (it's the prior value, or 0 on the first iteration).

Fix

Set res.iterations = iter + 1; immediately at the top of the loop body (or before each break), so it always reflects the iteration count attempted.

Acceptance criteria

• After a forced solver failure, res.iterations equals the iteration index at failure.

---

FINDING-H2 — 🔵 Five near-identical Newton loops

Location

code/include/newton_solver.hpp — newton_euclidean, newton_spherical, newton_hyper_ideal, newton_cp_euclidean, newton_inversive_distance

Problem

The five solvers share an identical loop skeleton (gradient → convergence check → Hessian → solve_with_fallback → line_search → break logic). Differences are only: the gradient fn, the Hessian fn, and the sign handling (spherical negates H). Any fix to the shared logic (e.g. H1 above) must be applied five times.

Fix

Extract a template newton_core(grad_fn, hessian_fn, x0, tol, max_iter) and have the five public functions become thin wrappers. The spherical sign flip can be a parameter or handled inside its hessian_fn lambda.

Acceptance criteria

• All existing Newton tests (11 + 3 fallback) pass unchanged after refactor.
• The five public signatures are preserved (no caller changes).

---

FINDING-H3 — 🔵 enforce_gauss_bonnet correction magnitude is silent

Location

code/include/gauss_bonnet.hpp (the enforce_gauss_bonnet function near L160)

Problem

enforce_gauss_bonnet adjusts cone angles so check_gauss_bonnet will subsequently pass. If the input geometry is far from satisfying Gauss-Bonnet, a large silent correction is applied with no signal to the caller.

Fix

Return (or log) the total/max applied correction so callers can detect pathological input. Optionally warn above a configurable threshold.

Acceptance criteria

• A test feeds intentionally bad cone angles and asserts the reported correction is large.

---

FINDING-H4 — 🔵 reduce_to_fundamental_domain boundary case

Location

code/include/period_matrix.hpp line 89 (if (tau.imag() <= 0.0)) Test: code/tests/cgal/test_phase7.cpp:315

Problem

The guard is <= 0.0, but the existing test uses a clearly-negative Im(τ). The exact-boundary case Im(τ) == 0.0 (on the real axis) is the more numerically interesting trigger and is untested.

Fix

Add EXPECT_THROW(reduce_to_fundamental_domain({1.0, 0.0}), std::domain_error);

Acceptance criteria

• The Im(τ)==0 boundary throws and is asserted.

---

FINDING-H5 — 🔵 Degenerate-triangle path not integration-tested

Location

code/include/euclidean_hessian.hpp lines 85–90 (euclidean_cot_weights returns {0,0,0,false} for degenerate triangles)

Problem

Degenerate faces yield zero cotangent weights, which can make the assembled Hessian singular. The QR fallback will then "solve" it, but with undefined solution quality. There is no test feeding a mesh with degenerate faces through newton_euclidean to characterize the behavior.

Fix

Add a test with a near-degenerate (sliver) triangle mesh and assert either:

• the solver reports a meaningful non-converged status (after I1), or
• the result is documented as best-effort.

Acceptance criteria

• Behavior on degenerate input is characterized by an explicit test, not left undefined.

---

FINDING-H6 — 🔵 Two ARM64 tests marked @Ignore

Location

Test suite (introduced in commit e2b0cdf: "mark 2 known ARM64 failures as @Ignore")

Problem

The primary CI runner is ARM64 (Raspberry Pi). Two tests are skipped there, so those code paths run unverified on the production CI platform.

Fix / Action

• Locate the two @Ignore/GTEST_SKIP-guarded tests (grep for ARM64, aarch64, GTEST_SKIP, DISABLED_).
• Document the root cause (numeric tolerance? CGAL kernel difference?) in this file.
• Either widen tolerances to make them pass on ARM64, or run them on an x86 CI lane.

Acceptance criteria

• Root cause documented; tests either re-enabled or moved to an x86 lane that runs them.

---

What is already good (do not "fix")

• Consistent exception taxonomy: std::runtime_error at I/O boundaries, std::logic_error for programmer errors, std::domain_error for math domain violations — idiomatic and correct.
• NewtonResult.converged flag exists (many implementations omit it).
• Finite-difference gradient checks for every functional — the right validation style.
• Java golden-oracle parity tests — high value for a port.
• LDLT→SparseQR fallback is itself unit-tested (tests #12–14 in test_newton_solver.cpp).
• Header-only design keeps the coverage scope (code/include/) clean.

---

Suggested order of work

1. C2 (make the coverage script honest) — small, unblocks C3 and C5.
2. I5 (decide what coverage measures) — decision, then small script change.
3. C3 (gate it) — needs owner's threshold decision.
4. C1 + I1 + H1 (Newton error-reporting) — do together; they touch the same code.
5. H2 (refactor the 5 loops) — do after C1/I1/H1 so you refactor the fixed version.
6. I2, I3, I4, H4 (missing negative tests) — independent, parallelizable, low-risk.
7. H3, H5, H6 (robustness characterization) — lower priority.