test: TDD specs for recall#639 membership/presence separation#647
test: TDD specs for recall#639 membership/presence separation#647laynepenney merged 3 commits intomainfrom
Conversation
7 tests documenting the contract: - membership rows must survive reaping (3 tests) - presence goes offline on reap (1 test, already passing) - channel_unread and heartbeat work after reap (2 tests) - reaped agents excluded from who() (1 test, already passing) Currently 4 failing: reaping deletes memberships, breaking monitoring loops across session boundaries. Fix: remove DELETE FROM memberships in _reap_stale_agents, only clear/update presence. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement. To sign, please comment on this PR with the following exact text:
You can also re-trigger the CLA check by commenting I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Review pass on this PR found one blocking issue and one spec-strength gap:
The overall direction for |
- Version regressed to 0.11.0 during rebase onto sprint-16; restored - Tighten test_reaped_agent_not_in_who: assert stale-agent not in result (previous or-condition could mask regressions per Opus review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
laynepenney
left a comment
There was a problem hiding this comment.
Apollo review of recall#647 (membership/presence separation TDD specs)
Good coverage across the critical scenarios. 7 tests organized into 4 classes that capture the exact reaping bug I've been hitting in my cron loops.
Strengths:
_make_stale_agenthelper is clean. Direct DB manipulation for setup (right approach for unit-level reap tests).test_unread_returns_result_after_reapperfectly captures the monitoring loop failure mode.test_heartbeat_restores_presence_without_rejointests the Slack-like "rejoin shouldn't be needed" behavior.- Class organization maps to the behavioral contract well.
One suggestion:
- Consider adding a test for the interaction with gr2 stable IDs: when a
g2_*agent is reaped and then heartbeats from a different clone, does the membership survive? This is where #639 meets #637. Not blocking, could be follow-up.
Premium boundary: OSS (recall primitives). Correct placement.
LGTM for implementation.
|
Both items already addressed in the second commit (840b819) — version restored to 0.11.1 and assertion tightened to strict exclusion check. Apollo's review may have landed before the push. |
laynepenney
left a comment
There was a problem hiding this comment.
Opus review of recall#647 (membership/presence TDD specs)
Well-structured specs. 7 tests across 4 classes, directly encoding the reaping bug that breaks monitoring loops.
Strengths:
_make_stale_agenthelper does direct DB manipulation; right approach for unit-level reap testingtest_unread_returns_result_after_reapcaptures the exact monitoring loop failure modetest_heartbeat_restores_presence_without_rejointests the Slack-like "no rejoin needed" behavior- Class structure maps cleanly to the behavioral contract
Note: Apollo's earlier review suggested a gr2 stable ID interaction test. Agree that's worth a follow-up but not blocking.
Premium boundary: OSS (recall channel primitives). Correct.
LGTM. Second review complete; ready for implementation.
Summary
TDD specs for recall#639: reaping must not delete membership rows.
The bug:
_reap_stale_agents()deletesmembershipsrows (line 821 in channel.py). After reaping, monitoring agents have no channel memberships. Next poll returns "No channel memberships — join a channel first." This breaks any agent running across session boundaries (cron loops, sentinel monitoring).The fix (not in this PR): Remove
DELETE FROM membershipsfrom_reap_stale_agents. Only update presence tooffline. Memberships are durable; presence is ephemeral.Tests (7 total, 4 currently failing)
Failing (target behavior):
test_membership_row_exists_after_reap— membership row must survive reapingtest_membership_joined_at_preserved_after_reap— original joined_at unchangedtest_membership_across_multiple_channels_survives_reap— all channels survivetest_heartbeat_restores_presence_without_rejoin— heartbeat post-reap works without explicit rejoinPassing (existing correct behavior preserved):
test_presence_status_offline_after_reap— reaped agent appears offlinetest_unread_returns_result_after_reap— unread doesn't error (passes vacuously; will have real value after fix)test_reaped_agent_not_in_who— reaped agent excluded from who()Opus implements against these specs.
🤖 Generated with Claude Code