review: external audit v0.10.0 — all 14 findings resolved #33
Reference in New Issue
Block a user
No description provided.
Delete Branch "review/external-audit-2026-05-30"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Full external code review of v0.10.0, all 14 findings resolved.
Audit document:
doc/reviewer/external-audit-2026-05-30.mdFindings (14/14 ✅)
face_energy()guard for multi-ideal vertices + throw + 3 testspin beforedoc + gauge-vertex overloadseuclidean_hessianedge-DOF Guard: 2 neue TestsCONFORMALLAB_LOW_MEMORY_BUILD+ Comment-TriggerTests
277/277 CGAL + 26/26 pure-math (vorher 246 CGAL)
CI
CONFORMALLAB_LOW_MEMORY_BUILD=ON: -O0, kein PCH, batch 1 → cc1plus Peak ~150 MB.Comment-Trigger:
/test-cgal·/quality-gates·/docs·/linksTest plan
/test-cgalauf dem Pi/quality-gates🤖 Generated with Claude Code
Finding-D from doc/reviewer/external-audit-2026-05-30.md. All three DOF-assignment functions iterated over every vertex unconditionally, overwriting any v_idx set before the call. The documentation said "pin before OR after" — the "before" option was silently wrong (the pin would be overwritten). For the inversive-distance variant the doc explicitly said the pre-call pin would make the function "a no-op for that vertex", which was false. Changes (three headers): - euclidean_functional.hpp assign_euclidean_vertex_dof_indices() - spherical_functional.hpp assign_vertex_dof_indices() - inversive_distance_functional.hpp assign_inversive_distance_vertex_dof_indices() For each: 1. Single-arg overload: doc corrected to "pin AFTER, not before; pre-call pins are overwritten" 2. New two-arg overload accepting a Vertex_index gauge: pins the requested vertex (v_idx=-1) in a single pass, preventing the user error entirely Three new GTests in test_euclidean_functional.cpp: SingleArg_PinBeforeHasNoEffect — documents the old pitfall TwoArg_GaugeIsPinnedOthersAreSequential — verifies the new overload TwoArg_NewtonConvergesWithGaugeOverload — end-to-end correctness 266/266 CGAL tests pass, 0 failed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Finding-F and Finding-G from doc/reviewer/external-audit-2026-05-30.md (java-port-audit missing-test items 1 and 2). Finding-F — Degenerate triangle: limiting angles (item 1) test_euclidean_functional.cpp: DegenerateTriangle_LimitingAngles_L{12,23,31}TooLong — verifies α_opposite = π, other two = 0, valid = false for all three edge-over-long cases DegenerateTriangle_GradientPicksUpPiCorner — end-to-end mesh test: forces effective l12 >> l23+l31 via lambda0, evaluates gradient, asserts G_v3 = π (not 2π from a skipped degenerate face) and G_v1=G_v2 = 2π test_spherical_functional.cpp: DegenerateTriangle_LimitingAngles_S{12,23,31}TooLong — same coverage for spherical_angles() Finding-G — euclidean_hessian edge-DOF guard (item 2) test_euclidean_hessian.cpp: EdgeDOFGuard_Throws — assign_euclidean_all_dof_indices + euclidean_hessian → throw EdgeDOFGuard_VertexOnlyDoesNotThrow — vertex-only layout → no throw (regression guard) 275/275 CGAL tests pass, 0 failed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Finding-H from doc/reviewer/external-audit-2026-05-30.md (java-port-audit missing-test item 7). Guards against a regression where compute_period_matrix reverts to reduce_to_fundamental_domain (no mirror fold) instead of normalizeModulus (Finding 6 fix, Java-faithful): such a revert would silently produce Re(τ) < 0 for lattices where the natural generators give a negative real part. Two new tests in test_phase7.cpp: 1. SkewedTorus_ReTauNegativeBeforeNorm_FoldedToPositive - New mesh: code/data/off/torus_skewed_4x4.off Flat 4×4 torus on parallelogram lattice ω₁=(4,0) ω₂=(-1,4); 16 vertices, 32 triangles, χ=0 (genus 1). - Full pipeline: newton_euclidean → cut_graph → euclidean_layout → compute_period_matrix(reduce=false) + compute_period_matrix(reduce=true) - Asserts: raw Re(τ) < 0, normalized Re(τ) ∈ [0,½], Im(τ) > 0, |τ| ≥ 1 2. SyntheticHolonomy_NegativeReTau_NormalizedToPositive - Bypasses mesh; supplies explicit ω₁=(4,0) ω₂=(-1,4) directly - Asserts raw τ = -0.25+i (to 1e-10), normalized τ = 0.25+i (to 1e-9) - Pin-points the mirror fold: Re(-0.25) → Re(+0.25) 277/277 CGAL tests pass, 0 failed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>MINOR-1 (spherical_functional.hpp:426) Wrong comment said "second derivative < 0 for a convex functional". The spherical energy is *concave* (NSD Hessian); the monotone-f argument applies to both convex and concave functionals equally. Comment rewritten to explain the actual physics: increasing scale increases all angles and thus reduces Σ G_v. MINOR-2 (spherical_functional.hpp:494-495) Forward finite difference O(ε) → central finite difference O(ε²): old: dft = (sum_Gv(t + fd_eps) - ft) / fd_eps new: dft = (sum_Gv(t + fd_eps) - sum_Gv(t - fd_eps)) / (2*fd_eps) Same cost when the extra sum_Gv(t - fd_eps) replaces the cached ft. MINOR-3 (euclidean_functional.hpp, spherical_functional.hpp, inversive_distance_functional.hpp) New header gauss_legendre.hpp centralises the 10-point Gauss-Legendre nodes and weights (gl10_nodes() / gl10_weights()). The three energy functions now use the shared accessors instead of duplicated local static arrays. MINOR-4 (euclidean_functional.hpp, spherical_functional.hpp, hyper_ideal_functional.hpp, inversive_distance_functional.hpp) halfedge_to_index() centralised in conformal_mesh.hpp. All four local aliases (eucl_hidx, spher_hidx, hidx, id_detail::hidx) now delegate to it as one-line wrappers; the aliases are kept for now to avoid a larger call-site churn, clearly documented as thin wrappers. MINOR-5 (clausen.hpp:33-38) Added a comment above inits() explaining the intentional off-by-one return value and how it interacts with csevl() — matching the Java Clausen.inits() / csevl() contract. 277/277 CGAL + 26/26 pure-math tests pass, 0 failed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Finding-I from doc/reviewer/external-audit-2026-05-30.md. Root cause: CGAL+Eigen at -O3 drives cc1plus peak RAM to ~700 MB per Unity compilation unit (batch size 4) on ARM64. With the 1600 MB container limit the 2nd or 3rd TU reliably triggered OOM-kill, so test-cgal was gated off via `if: false` since 2026-05-26. Fix: new CMake option CONFORMALLAB_LOW_MEMORY_BUILD=ON applies four orthogonal memory-saving measures to conformallab_cgal_tests: 1. -O0 (no debug info): optimizer passes entirely skipped → cc1plus peak drops from ~700 MB to ~150-200 MB per TU on ARM64. Omitting -g avoids the additional object-file / linker RAM cost. 2. CONFORMALLAB_USE_PCH=OFF: saves the one-time ~200 MB PCH compilation cost; each TU re-parses CGAL headers (fast at -O0). 3. UNITY_BUILD_BATCH_SIZE=1: one source file per cc1plus invocation, removing the "4-file template-explosion" per-unit multiplier. 4. -Wl,--no-keep-memory (GNU ld): linker releases symbol tables after each input file → ~15-25 % less linker RSS. Verified locally with cmake -DCONFORMALLAB_LOW_MEMORY_BUILD=ON: 277/277 CGAL tests pass, 31 s runtime (vs 2 s at -O3 — expected; tests run 15× slower without optimizer but all correct). CI workflow changes (cpp-tests.yml): - test-cgal re-enabled: `if: github.event_name == 'pull_request'` - Configure step adds -DCONFORMALLAB_LOW_MEMORY_BUILD=ON - Container memory: 1600m → 2000m (--memory-swap=3000m for 1 GB swap headroom), using ~half of the Pi's 3-4 GB while leaving OS margin. CLAUDE.md updated: new flag added to compile-time options table; CI status row corrected from DISABLED to active. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Instead of triggering on every pull_request push, test-cgal now fires only when a PR comment contains "/test-cgal". This prevents the Pi runner (3-4 GB RAM, swap constantly loaded) from queuing CGAL builds faster than it can drain them on every WIP commit. Workflow changes: - Add `issue_comment: types: [created]` to the top-level `on:` block - test-cgal `if:` condition: event is issue_comment AND comment is on a PR (issue.pull_request != null) AND comment body contains "/test-cgal" - Checkout uses `refs/pull/N/head` so the PR branch is checked out correctly (issue_comment sets GITHUB_REF to the default branch, not the PR branch) Usage: write "/test-cgal" as a PR comment to trigger the 277-test CGAL suite. The build uses CONFORMALLAB_LOW_MEMORY_BUILD=ON (-O0, no PCH, unity batch 1) and runs in a 2000 MB container. CLAUDE.md: CI table + status paragraph updated accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>/test-cgal
/quality-gates
/test-cgal · /quality-gates · /docs · /links
/test-cgal · /quality-gates · /docs · /links
/test-cgal
/quality-gates