merge(999.73): #576 total-ordering + #539-hang fix (un-quarantine AssignBondOrder test2)

- AromaticityProcessor::aromatize infinite-loop fix (the 60-min AssignBondOrder
  CI hang; a regression from the 999.72 #539 fixed-point pass on the SMARTS path)
- PartialBondOrderAssignment::operator< made a total strict weak ordering (#576);
  AssignBondOrderProcessor_test2 un-quarantined (deterministic across STLs)
- #627 (Directory) NOT yet included — executor stalled before it; follows separately

Verified on macOS-arm64: AssignBondOrderProcessor_test1 3s (was >3600s hang),
test2 PASS 2s, AromaticityProcessor_test PASS.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: okohlbacher

source/QSAR/aromaticityProcessor.C

@@ -708,7 +708,19 @@ namespace BALL
 				// does the ring share a full edge with the existing aromatic system?
 				// (a ring bond whose two endpoints are both already on an aromatic bond)
 				bool shares_aromatic_edge = false;
-				// is any ring bond still non-aromatic (i.e. is there work to do)?
+				// is any ring bond still NOT marked aromatic (i.e. is there work to do)?
+				//
+				// BUG-576 (#576): the termination signal MUST be measured against the
+				// SAME state that the aromatization below mutates. The aromatization
+				// always sets the Bond::IS_AROMATIC *property* (and the bond *order*
+				// only when overwrite_bond_orders_ is true). The original BUG-539 code
+				// gated this loop on b->getOrder() != ORDER__AROMATIC, so when
+				// overwrite_bond_orders_ == false (the SMARTS / evaluatePenalty path,
+				// e.g. BEWCUB.mol2) the order never changes, has_non_aromatic_ring_bond
+				// stays true forever and the while(changed) loop never terminates —
+				// that infinite loop is the CI hang. Gate on the IS_AROMATIC property,
+				// which the aromatization unconditionally sets, so each fused ring is
+				// processed exactly once and the fixed point is actually reached.
 				bool has_non_aromatic_ring_bond = false;
 				for (HashSet<Atom*>::iterator a = ring.begin(); a != ring.end(); ++a)
 				{
@@ -719,7 +731,7 @@ namespace BALL
 						{
 							continue;
 						}
-						if (b->getOrder() != Bond::ORDER__AROMATIC)
+						if (!b->hasProperty(Bond::IS_AROMATIC))
 						{
 							has_non_aromatic_ring_bond = true;
 						}

source/STRUCTURE/BONDORDERS/partialBondOrderAssignment.C

@@ -2,6 +2,8 @@
 #include <BALL/STRUCTURE/assignBondOrderProcessor.h>
 #include <BALL/KERNEL/PTE.h>
 
+#include <algorithm> // BUG-576: std::lexicographical_compare for the total operator< tie-break
+
 namespace BALL
 {
 #define INFINITE_PENALTY 1e5
@@ -33,21 +35,56 @@ namespace BALL
 
 	// the less operator
 	// note: we want a reverse sort, hence we actually return a "greater"
+	//
+	// BUG-576 (#576): This comparator feeds a std::priority_queue (a max-heap that
+	// pops the highest-priority element, i.e. the LOWEST penalty, first). For that
+	// to be deterministic across platforms the ordering MUST be a TOTAL strict weak
+	// ordering: it must distinguish every pair of entries that differ in content,
+	// otherwise equivalent entries pop in heap-internal (libc++ vs libstdc++ vs MSVC
+	// STL) order and apply(n) returns a different n-th solution per platform.
+	//
+	// We cascade through content-derived keys, returning as soon as a key differs:
+	//   (1) coarsePenalty()  (the optimisation objective)
+	//   (2) finePenalty()    ALWAYS (not gated on use_fine_penalty_ — that option
+	//                        decides which solutions are optimal, not how the queue
+	//                        must order them; applying it always keeps the FINE
+	//                        subtest passing and breaks coarse ties deterministically)
+	//   (3) last_bond        (Position, numeric)
+	//   (4) bond_orders      (lexicographic, vector<short>)
+	// Returning false in both directions is correct ONLY when penalties AND
+	// bond_orders are identical, i.e. the entries are genuinely indistinguishable.
+	//
+	// Direction: operator< returns true when *this should pop AFTER b (i.e. *this is
+	// the "greater" entry). At each level we therefore compare *this's key '>' b's key.
 	bool PartialBondOrderAssignment::operator < (const PartialBondOrderAssignment& b) const
 	{
 		bool value = false;
-		if (coarsePenalty() > b.coarsePenalty())
+
+		float coarse_a = coarsePenalty();
+		float coarse_b = b.coarsePenalty();
+		if (coarse_a != coarse_b)
 		{
-			value = true;
+			value = (coarse_a > coarse_b);
 		}
 		else
 		{
-			if (coarsePenalty() == b.coarsePenalty())
+			float fine_a = finePenalty();
+			float fine_b = b.finePenalty();
+			if (fine_a != fine_b)
 			{
-				if (abop->use_fine_penalty_ && (finePenalty() > b.finePenalty()))
-				{
-					value = true;
-				}
+				value = (fine_a > fine_b);
+			}
+			else if (last_bond != b.last_bond)
+			{
+				value = (last_bond > b.last_bond);
+			}
+			else
+			{
+				// total tie-break on the bond-order vector itself.
+				// std::lexicographical_compare(b, a) is true iff b < a lexicographically,
+				// i.e. iff *this is lexicographically greater -> *this pops after b.
+				value = std::lexicographical_compare(b.bond_orders.begin(), b.bond_orders.end(),
+				                                     bond_orders.begin(), bond_orders.end());
 			}
 		}
 

test/AssignBondOrderProcessor_test2.C

@@ -1006,7 +1006,14 @@ CHECK(Option::USE_FINE_PENALTY)
 	storeBondOrders(sys);
 	sys.apply(abop);
 	TEST_EQUAL(compareBondOrder(sys), false)
-	abop.apply(5); //3);
+	// BUG-576 (#576): with the deterministic total ordering in
+	// PartialBondOrderAssignment::operator< the degenerate optimal solutions for
+	// AMPTRB10_kek_sol0 enumerate in a fixed, platform-independent order. The stored
+	// (sol0) assignment is now consistently the n-th solution -> index 3 on every
+	// platform. The old index 5 was the libc++/heap-internal position on Intel/Linux
+	// only and produced a different solution per platform (the non-determinism this
+	// fix removes), which is why test2 was previously quarantined on Apple Silicon.
+	abop.apply(3);
 	TEST_EQUAL(compareBondOrder(sys), true)
 RESULT
 

test/CMakeLists.txt

@@ -81,27 +81,15 @@ SET_TESTS_PROPERTIES (
 # as green. Each quarantine has a backlog stub in ROADMAP.md + a row in
 # .planning/phases/09-test-suite-triage/09-TRIAGE.md.
 #
-# AssignBondOrderProcessor_test2 — QUARANTINED (Phase 9, 09-TRIAGE.md row 3)
-#   Failure: test/AssignBondOrderProcessor_test2.C:1010
-#   CHECK(Option::USE_FINE_PENALTY): compareBondOrder(sys) returns 0, expected 1
-#   after abop.apply(5). The fine-penalty ILP/heuristic code path in
-#   source/STRUCTURE/assignBondOrderProcessor.C produces an incorrect bond-order
-#   assignment for AMPTRB10_kek_sol0.mol2 when the 5th solution is requested.
-#   AssignBondOrderProcessor_test1 (default code path) passes — the regression
-#   is isolated to the fine-penalty branch. Algorithmic fix estimated >2h.
-#   Backlog: Phase 999.34 closed 2026-05-17 (Windows test gatekeeper only); the
-#   AssignBondOrderProcessor fine-penalty algorithmic fix is a separate v1.7 /
-#   v2.0 cycle task (no dedicated phase stub yet — re-file if/when picked up).
-#
-#   NOTE (2026-05-16 Rule-1 fix): WILL_FAIL applies only on Apple Silicon (the
-#   failure platform). Linux x64/arm64 pass this test cleanly; unconditional
-#   WILL_FAIL causes an "unexpected pass" error on Linux (ctest exit 8).
-IF (APPLE AND CMAKE_SYSTEM_PROCESSOR MATCHES "arm64|aarch64")
-	SET_TESTS_PROPERTIES (
-		AssignBondOrderProcessor_test2
-		PROPERTIES WILL_FAIL TRUE
-	)
-ENDIF()
+# AssignBondOrderProcessor_test2 — UN-QUARANTINED (BUG-576, #576, Phase 999.73).
+#   The Apple-Silicon-only WILL_FAIL is gone: the failure was platform-dependent
+#   solution enumeration, not an algorithmic bug. PartialBondOrderAssignment::
+#   operator< is now a TOTAL strict weak ordering (coarse -> fine -> last_bond ->
+#   lexicographic bond_orders), so the degenerate-optimal solutions enumerate in a
+#   fixed order on libc++/libstdc++/MSVC STL alike; the test asserts the stored sol0
+#   at its now-deterministic index 3 (was the heap-internal "5"). The 60-minute CI
+#   hang was a separate defect in AromaticityProcessor::aromatize (BUG-539 fused-ring
+#   propagation looped forever when overwrite_bond_orders_==false) — also fixed here.
 
 # Regressions from parallel-session PR merge (commit 45dce6971, Phase 999.14,
 # feat: merge C++ slice of PRs #546 #554 #550). These tests broke because the