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

> **✅ Resolution status (2026-05-31, Session 1):** **C1 ✅** (solver `ok` flag
> exposed), **C2/C3 ✅** (coverage script honest + CI gate in ramp-up),
> **I2/I3/I4 ✅** (serialization / mesh / spherical-hessian negative tests),
> **H2 ✅** (five Newton loops unified into `newton_core`, folding in B2/B3/B4/B5).
> **I1 ✅** (`NewtonStatus` enum) + **H1 ✅** (iteration count) done in S2.
> **Open:** **H3/H4/H5** → S3; **I5** (coverage on full suite, un-gates the
> 80/70/90 threshold) → S5.
> See [`finding-orchestration.md`](finding-orchestration.md). 298/298 tests green.

> **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

```bash
# 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.

```cpp
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.

```cpp
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`:

```bash
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:
  ```bash
  [ -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`:

```cpp
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`:
```cpp
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
```cpp
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`:
```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:
```cpp
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
```bash
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
```cpp
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.