diff --git a/delphi/polismath/conversation/conversation.py b/delphi/polismath/conversation/conversation.py index 47a6ca150..0a0b34f55 100644 --- a/delphi/polismath/conversation/conversation.py +++ b/delphi/polismath/conversation/conversation.py @@ -489,17 +489,26 @@ def _apply_moderation(self) -> None: """ Apply moderation settings to create filtered rating matrix. - Matches Clojure behavior (named_matrix.clj:214-230): - - Moderated-out participants are removed (rows dropped) - - Moderated-out comments are ZEROED OUT, not removed — the column - stays in the matrix with all values set to 0. This preserves - matrix structure so that tids, column indices, and dimensions - match between Python and Clojure. + Comment moderation matches Clojure (named_matrix.clj:214-230): + moderated-out comments are ZEROED OUT, not removed — the column stays + in the matrix with all values set to 0. This preserves matrix + structure so that tids, column indices, and dimensions match between + Python and Clojure. + + Participant bans (mod_out_ptpts) are a Python-only feature: the + Clojure worker's ingest path has no participants.mod filter, so + banned participants keep influencing every downstream node + (CLOJURE_QUIRKS Q1). 'clojure-legacy' mode replicates that leak — + the set is stored but NOT applied; 'improved' mode drops the rows. """ - # Filter out moderated participants (remove rows). + # Filter out banned participants (remove rows) — improved mode only; + # legacy mode leaks like Clojure (Q1). # Preserve raw_rating_mat row order (vote encounter order) — see # update_votes() comment on why row order matters for Clojure parity. - keep_ptpts = [p for p in self.raw_rating_mat.index if p not in self.mod_out_ptpts] + if resolve_engine_mode() == ENGINE_MODE_LEGACY: + keep_ptpts = list(self.raw_rating_mat.index) + else: + keep_ptpts = [p for p in self.raw_rating_mat.index if p not in self.mod_out_ptpts] self.rating_mat = self.raw_rating_mat.loc[keep_ptpts].copy() # Zero out moderated-out comments (keep columns, set values to 0) @@ -1723,10 +1732,13 @@ def _compute_user_vote_counts(self) -> Dict[str, int]: import time start_time = time.time() # raw_rating_mat for the COLUMN view (preserves moderated-out comments — D15 - # parity), but filtered to rating_mat.index for the ROW view so moderated-out - # *participants* (mod_out_ptpts, dropped by _apply_moderation) don't leak - # into vote counts. Both filters together give the moderation-applied state - # with un-zeroed values, matching what Clojure produces. + # parity), but filtered to rating_mat.index for the ROW view so banned + # participants (mod_out_ptpts, dropped by _apply_moderation in improved + # mode) don't leak into vote counts. In clojure-legacy mode + # rating_mat.index keeps banned rows (Q1 leak replication), so this + # matches Clojure's unfiltered user-vote-counts there. Both filters + # together give the moderation-applied state with un-zeroed values, + # matching what Clojure produces. mat = self.raw_rating_mat.loc[self.rating_mat.index] logger.info(f"Starting _compute_user_vote_counts for {mat.shape[0]} participants") @@ -1821,13 +1833,13 @@ def _get_in_conv_participants(self) -> Set[Any]: # Legacy: carry forward the persisted in-conv set, then union the # threshold set into it (Clojure `(into in-conv ...)`, conversation.clj:247-256). - # PRUNE the carry to participants still present in vote_counts first: - # vote_counts is keyed off rating_mat.index, which drops mod_out_ptpts - # (banned participants — a Python-only feature Clojure lacks). Without the - # intersection a participant banned AFTER being carried would linger in the - # set forever, inflating the size check so the greedy floor never re-fires - # to top the actually-clustered pool (proj ∩ in_conv) back up, and growing - # the carry unboundedly. Clojure-parity is unaffected (no ban feature there). + # The intersection with vote_counts is belt-and-braces: since the Q1 + # ban-leak replication, legacy-mode rating_mat keeps banned rows, so + # vote_counts covers every carried pid and the intersection is inert + # (append-only votes mean a counted pid can never vanish). It stays as + # defense against any future row-view change re-opening the stale-carry + # trap #2623's T1 fixed (a carried pid missing from vote_counts would + # inflate the size check so the greedy floor never re-fires). in_conv = (set(self.in_conv) & set(vote_counts.keys())) | threshold_set # Greedy floor (conversation.clj:259-268): if under 15, admit the top diff --git a/delphi/tests/test_in_conv_greedy_carry.py b/delphi/tests/test_in_conv_greedy_carry.py index 7146ab3c1..25b6dd378 100644 --- a/delphi/tests/test_in_conv_greedy_carry.py +++ b/delphi/tests/test_in_conv_greedy_carry.py @@ -151,16 +151,16 @@ def test_qualifier_stays_in_across_ticks(self, monkeypatch, mode): assert 'H0' in _clustered_pids(conv) -class TestCarryPruneOnParticipantBan: - """T1 (#2623): a participant carried in self.in_conv and then BANNED - (mod_out_ptpts, a Python-only feature Clojure lacks) must be pruned from the - carry so the greedy floor tops the pool back up. Before the fix, the raw - carried set (`set(self.in_conv) | threshold_set`) still counted the banned - pids, so the size check saw a stale 15 while the actually-clustered pool - (proj ∩ in_conv, which excludes banned) had dropped below the floor — and - nothing topped it up. Legacy-mode-only; improved mode has no carry.""" - - def test_ban_after_carry_tops_floor_back_up(self, monkeypatch): +class TestCarryUnderParticipantBan: + """Q1 leak replication (2026-07-22) SUPERSEDES #2623's T1 scenario: in + clojure-legacy mode a ban is stored but NOT applied (Clojure's worker never + honored participants.mod = -1), so banning can no longer shrink the + legacy-mode clustering pool and the stale-carry trap T1 fixed cannot arise. + The vote_counts intersection in _get_in_conv_participants stays as + belt-and-braces (see its comment). This test pins the new semantics: + carry and clustering are ban-invariant in legacy mode.""" + + def test_ban_after_carry_changes_nothing(self, monkeypatch): _mode(monkeypatch, 'clojure-legacy') # Tick 1: greedy floor fills to 15 (H0,H1 + L0..L12) and persists them. conv = Conversation('ban').update_votes(_votes(_TICK1_SPECS)) @@ -168,17 +168,13 @@ def test_ban_after_carry_tops_floor_back_up(self, monkeypatch): banned = {'L0', 'L1', 'L2', 'L3', 'L4'} assert banned.issubset(conv.in_conv) # all 5 are carried greedy admits - # Ban 5 of the 15 carried participants. vote_counts (from rating_mat.index) - # now excludes them, so the effective pool drops to 10 and the floor must - # re-admit 5 more of the remaining lows (L13..L19) to reach 15. + # Ban 5 of the 15 carried participants. Q1 leak: the set is stored but + # the pool, carry and clustering are unchanged — exactly as if Clojure + # had processed the same stream. conv2 = conv.update_moderation({'mod_out_ptpts': list(banned)}) + assert conv2.mod_out_ptpts == banned # stored ... clustered = _clustered_pids(conv2) - # FIX: pruned carry -> floor re-fires -> back up to 15. - # BEFORE FIX: stale carry keeps count at 15 (floor never fires), but the - # banned rows aren't clustered -> only 10 clustered. - assert len(clustered) == 15 - assert not (banned & clustered) # banned never clustered - assert not (banned & conv2.in_conv) # banned pruned from carry (no stale growth) - assert {'H0', 'H1'}.issubset(clustered) # qualifiers retained - assert len(conv2.in_conv) == 15 + assert banned.issubset(clustered) # ... but still clustered + assert conv2.in_conv == conv.in_conv # carry untouched + assert len(clustered) == 15 # pool unchanged, floor idle diff --git a/delphi/tests/test_mod_ptpt_leak_parity.py b/delphi/tests/test_mod_ptpt_leak_parity.py new file mode 100644 index 000000000..629bae8cc --- /dev/null +++ b/delphi/tests/test_mod_ptpt_leak_parity.py @@ -0,0 +1,103 @@ +#!/usr/bin/env python3 +""" +Participant-ban (participants.mod = -1) leak replication — CLOJURE_QUIRKS Q1. + +The Clojure math worker NEVER honored participant bans: its ingest path has no +participants.mod filter, so mod_out_ptpts never reaches the conv and banned +participants keep influencing user-vote-counts, in-conv, PCA, clustering and +repness. Python gained a real ban feature (mod_out_ptpts row drop in +_apply_moderation, 2026-06-10) — correct, but a certification divergence. + +Per Q1: 'clojure-legacy' mode must LEAK the ban exactly like Clojure (rows +kept everywhere); 'improved' mode keeps the fix. This supersedes the legacy- +mode premise of #2623's TestCarryPruneOnParticipantBan (banning can no longer +shrink the legacy clustering pool, so the carry-prune scenario cannot arise). +""" + +import os +import sys + +import pytest + +sys.path.append(os.path.abspath(os.path.dirname(__file__))) + +from polismath.pca_kmeans_rep.pca import PCA_IMPL_ENV_VAR +from polismath.utils.engine_mode import ENGINE_MODE_ENV_VAR +from polismath.conversation.conversation import Conversation + + +N_CMTS = 8 + + +def _bloc_votes(): + """Two blocs of 6, everyone votes all comments (all above threshold).""" + votes = [] + for i in range(6): + for t in range(N_CMTS): + votes.append({'pid': f'a{i}', 'tid': f'c{t}', + 'vote': 1.0 if t < 4 else -1.0}) + votes.append({'pid': f'b{i}', 'tid': f'c{t}', + 'vote': -1.0 if t < 4 else 1.0}) + return {'votes': votes} + + +def _clustered_pids(conv): + return {m for bc in conv.base_clusters for m in bc['members']} + + +@pytest.fixture +def legacy_mode(monkeypatch): + monkeypatch.delenv(PCA_IMPL_ENV_VAR, raising=False) + monkeypatch.setenv(ENGINE_MODE_ENV_VAR, 'clojure-legacy') + + +@pytest.fixture +def improved_mode(monkeypatch): + monkeypatch.delenv(PCA_IMPL_ENV_VAR, raising=False) + monkeypatch.setenv(ENGINE_MODE_ENV_VAR, 'improved') + + +class TestLegacyBanLeak: + + def test_banned_participant_rows_kept(self, legacy_mode): + conv = Conversation('leak').update_votes(_bloc_votes()) + conv = conv.update_moderation({'mod_out_ptpts': ['a0', 'b0']}) + # The ban is STORED (payload bookkeeping unchanged) ... + assert conv.mod_out_ptpts == {'a0', 'b0'} + # ... but NOT applied: Clojure's worker never drops banned rows. + assert 'a0' in conv.rating_mat.index + assert 'b0' in conv.rating_mat.index + + def test_banned_participant_still_clustered(self, legacy_mode): + conv = Conversation('leak').update_votes(_bloc_votes()) + conv = conv.update_moderation({'mod_out_ptpts': ['a0', 'b0']}) + clustered = _clustered_pids(conv) + assert {'a0', 'b0'}.issubset(clustered) + # And they stay through a subsequent vote tick. + conv = conv.update_votes({'votes': [ + {'pid': 'a1', 'tid': 'c0', 'vote': 1.0}]}) + assert {'a0', 'b0'}.issubset(_clustered_pids(conv)) + + def test_banned_participant_counted_in_user_vote_counts(self, legacy_mode): + conv = Conversation('leak').update_votes(_bloc_votes()) + conv = conv.update_moderation({'mod_out_ptpts': ['a0']}) + assert conv._compute_user_vote_counts().get('a0') == N_CMTS + + def test_banned_participant_stays_in_conv(self, legacy_mode): + conv = Conversation('leak').update_votes(_bloc_votes()) + conv = conv.update_moderation({'mod_out_ptpts': ['a0']}) + assert 'a0' in conv.in_conv + + +class TestImprovedBanKept: + """Improved mode keeps the real ban feature byte-for-byte.""" + + def test_banned_participant_dropped_and_not_clustered(self, improved_mode): + conv = Conversation('leak').update_votes(_bloc_votes()) + conv = conv.update_moderation({'mod_out_ptpts': ['a0']}) + assert 'a0' not in conv.rating_mat.index + assert 'a0' not in _clustered_pids(conv) + + +if __name__ == '__main__': + pytest.main([__file__, '-v'])