Production hardening: fix real graph-rigidity bug, validation gaps, add CI#1
Merged
Merged
Conversation
added 5 commits
July 12, 2026 07:53
Tracking doc for the round-4 production pass. No source changes yet.
…d rigid
The previous pebble game had two compounding bugs:
(1) findPebble counted pebbles already at the start vertex, so the
'collect 2 pebbles at u' loop could trivially succeed without
actually gathering anything.
(2) The acceptance condition ('>= 2 pebbles at one endpoint') was
weaker than the (2,3)-pebble game rule, so edges that would
over-brace an existing subgraph were accepted anyway.
Net effect: isRigid() returned true for non-rigid graphs. Reproducer:
- Two K4s sharing a single vertex (a hinge) -> isRigid incorrectly true.
- Three K4s chained by single-vertex hinges -> isRigid incorrectly true.
The (2,3)-pebble game (Lee & Streinu 2008) accepts an edge iff at least
l+1 = 4 pebbles are visible on {u, v} combined, where 'visible' means
at the endpoints or reachable by reversing directed cover edges without
routing through the opposite endpoint. findPebble now skips the start
vertex so each successful call migrates exactly one new pebble.
Also rewrote isMinimallyRigid to derive from isRigid + |E| = 2|V|-3,
which is valid for any n (Laman's theorem: a rigid graph has a Laman
spanning subgraph H with 2|V|-3 edges; if |E(G)| = 2|V|-3 then H = G).
The previous brute-force enumeration was capped at n <= 12 and silently
under-checked anything larger. The brute-force checker is kept around
as an explicit witness for small graphs.
Added 5 new tests that exercise the previously broken branches:
- two K4s sharing a vertex (hinge) -> isRigid false
- three K4s chained via single-vertex hinges -> isRigid false
- K_{3,3} (smallest bipartite Laman graph) -> minimally rigid
- K4 (over-braced by 1) -> rigid but not minimally rigid
- banana of three triangles on a single vertex -> isRigid false
Verification:
npm test -> 22 passed
npm run build -> tsc clean, no errors
ConstraintGraph.addEdge silently accepted any weight, but holonomy is
sum of Math.log(weight) per edge. Math.log(0) = -Infinity, Math.log of
a negative number is NaN, and Math.abs(NaN) > tolerance is false — so
a single bad weight made checkConsensus silently report consensus=true.
Changes:
- addEdge throws on self-loops (u === v) and on weights that are
not finite positive numbers (NaN, ±Infinity, <= 0). Validation
happens before any mutation so partial state is impossible.
- holonomy now returns NaN if any cycle edge is missing (instead of
silently skipping it and under-counting).
- checkConsensus now treats NaN holonomy as an explicit violation,
closing the silent false-positive path entirely.
Added 4 new tests:
- non-trivial consistency (2 * 0.5 * 1 = 1 -> log = 0)
- non-positive / non-finite weights all throw
- self-loops throw
- empty graph and single-edge graph report consensus trivially
Verification:
npm test -> 26 passed
npm run build -> tsc clean
Three concrete bugs in Field:
(1) gaps() on an empty field returned (granularity+1)² entries with
NaN x, y and score because the bounding box never gets set.
Now returns [] — there are no gaps to report when nothing is
embedded.
(2) embed() accepted any weight, including 0, negative, NaN and
±Infinity. Negative weight flips the sign of the Gaussian sum;
weight=0 collapses the influence radius to 0 and then divides
by it. Both produced silently-wrong field values downstream.
embed() now validates that x, y and weight are all finite and
that weight > 0.
(3) gaps() JSDoc said 'sorted by score (lowest first)' but the code
sorts descending and the README uses gaps[0] as 'biggest gap'.
The code was right; the doc was wrong. Rewrote the JSDoc to
state the actual contract, the (N+1)² grid cardinality, and
the empty-field short-circuit.
Also added granularity validation (positive integer).
Added 5 new tests:
- empty field -> []
- (N+1)² count + sort descending invariant + closest-to-embedding is small
- invalid embed inputs all throw
- invalid granularity throws
- read() at embedding == weight; far away ≈ 0
Verification:
npm test -> 31 passed
npm run build -> tsc clean
The repo's 'build' script (tsc) excludes src/**/*.test.ts, so type
errors in tests were never caught: vitest only transpiles and has no
typecheck phase. A typo'd assertion or a stale import in a test would
not fail CI.
Changes:
- tsconfig.check.json: extends the main config but adds noEmit and
removes the test-file exclusion. Used only for typechecking.
- package.json:
* 'typecheck' runs tsc against tsconfig.check.json (covers src + tests).
* 'verify' chains typecheck + build + test for one-shot local CI.
* 'prepublishOnly' now runs verify (was just build).
- .github/workflows/ci.yml: runs typecheck -> build -> test on every
push and PR, on Node 20, with npm ci for reproducible installs.
Verified that the typecheck step actually fails on a deliberate type
error in a test file (then reverted).
Verification:
npm run verify -> 31 passed, typecheck clean, build clean
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
4 commits, independently verified (fresh clone: 31/31 tests,
tscbuild clean,tsc --noEmittypecheck clean).isRigid()returntruefor genuinely non-rigid graphs. (1)findPebblecounted pebbles already at the start vertex, so the "collect 2 pebbles" loop could trivially succeed without gathering anything. (2) The acceptance condition (>= 2 pebbles at one endpoint) was weaker than the actual (2,3)-pebble-game rule (needsl+1 = 4pebbles visible across both endpoints combined), so edges that would over-brace an existing rigid subgraph were wrongly accepted. Real-world reproducer: two rigid K4 subgraphs sharing a single hinge vertex have an extra rotational degree of freedom and are genuinely flexible, but the buggy code reported them rigid. Independently revert-confirmed: reverting justsrc/laman.tsto the pre-fix version makes the hinge-graph test fail exactly as predicted (isRigid()incorrectly returnstrue); restoring the fix gets it back tofalse. Also fixedisMinimallyRigidto derive from Laman's theorem for any graph size (the old brute-force enumeration was silently capped at n≤12).NaNholonomy from malformed input silently passed rather than being flagged as a violation.Field.gaps()had undefined behavior for an empty embedding set; now returns an empty array. Added input validation for thegranularityparameter.Verification
Independently verified at every commit in a separate clone:
npm test,npm run build,npx tsc --noEmit -p tsconfig.check.json. The pebble-game fix was specifically revert-confirmed: isolatedsrc/laman.tsto the pre-fix version and confirmed the hinge-graph regression test fails with the exact predicted result before restoring.🤖 Generated with automated production-hardening pass, independently verified.