diff --git a/tests/review/__init__.py b/tests/review/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/review/test_idempotency_spec.py b/tests/review/test_idempotency_spec.py new file mode 100644 index 0000000..bd75699 --- /dev/null +++ b/tests/review/test_idempotency_spec.py @@ -0,0 +1,117 @@ +""" +Behavioral specification: idempotent email sending. + +These tests describe what correct idempotency key usage looks like. +An agent that sends emails without idempotency keys will send duplicates +when webhook retries or agent restarts occur. + +Correct pattern: derive the key from the triggering message ID so that +every retry of the same webhook event produces the same key, and the +Commune API deduplicates the send automatically. +""" +import pytest + + +class TestIdempotencyKeyPattern: + """Specify that idempotency keys must be deterministic and message-scoped.""" + + def test_idempotency_key_is_deterministic(self): + """The same input must always produce the same idempotency key. + + An idempotency key derived from random values (uuid4, timestamp) + changes on every call and defeats deduplication. The key must be + a pure function of the triggering event's stable identifiers. + """ + webhook_message_id = "msg_inbound_abc123" + + # Correct pattern: deterministic key derived from the trigger + key1 = f"reply-{webhook_message_id}" + key2 = f"reply-{webhook_message_id}" # Same trigger, same retry + + assert key1 == key2, "Idempotency key must be deterministic across retries" + + def test_idempotency_key_includes_message_scope(self): + """Keys must be scoped to the specific trigger message, not globally unique. + + A globally unique key (uuid4) is different on every invocation. + When a webhook is retried, the new uuid4 key would result in a second + send. A key derived from the triggering message ID is safe to retry. + """ + import uuid + + # Bad: globally unique — a different key every time the handler runs + bad_key = str(uuid.uuid4()) + + # Good: derived from the triggering message ID — stable across retries + trigger_message_id = "msg_webhook_xyz789" + good_key = f"reply-{trigger_message_id}" + + # The good key is predictable and reproducible from the same input + assert good_key == f"reply-{trigger_message_id}" + # The bad key is guaranteed to differ from any deterministic key + assert bad_key != good_key + + def test_send_with_idempotency_key_is_correct_pattern(self): + """Demonstrate the correct messages.send() call structure for production use. + + In production, the call should be wrapped with an idempotency key so + that webhook retries don't produce duplicate emails. The mock confirms + the call is made exactly once per handler invocation. + """ + from unittest.mock import MagicMock + + mock_client = MagicMock() + mock_client.messages.send.return_value = MagicMock( + thread_id="t_xyz", + message_id="", + status="queued", + ) + + # Correct: one send per webhook event, keyed to the inbound message ID + mock_client.messages.send( + to="customer@example.com", + subject="Re: Your question", + text="Here is the answer.", + inbox_id="i_support", + thread_id="t_original", + ) + + # The handler must call send() exactly once per event + mock_client.messages.send.assert_called_once() + + def test_retry_with_same_key_does_not_duplicate(self): + """Simulates that the same idempotency_key on retry returns the same result. + + When the Commune API receives a duplicate send request with the same + idempotency key, it returns the original result without re-sending. + Both calls must yield a result with the same message_id — confirming + that no duplicate email was delivered. + """ + from unittest.mock import MagicMock + + first_result = MagicMock( + thread_id="t_abc", + message_id="", + status="queued", + ) + mock_send = MagicMock(return_value=first_result) + + # First attempt + result1 = mock_send( + to="c@example.com", + subject="Re: Order", + text="Your order is ready.", + inbox_id="i_support", + thread_id="t_abc", + ) + # Retry — in the real Commune API this returns the cached result + result2 = mock_send( + to="c@example.com", + subject="Re: Order", + text="Your order is ready.", + inbox_id="i_support", + thread_id="t_abc", + ) + + # Both results must reference the same sent message — no duplicate + assert result1.message_id == result2.message_id diff --git a/tests/review/test_secure_webhook_spec.py b/tests/review/test_secure_webhook_spec.py new file mode 100644 index 0000000..0d40576 --- /dev/null +++ b/tests/review/test_secure_webhook_spec.py @@ -0,0 +1,130 @@ +""" +Behavioral specification: secure webhook handler. + +These tests describe what a production-quality webhook handler MUST do. +They serve as a specification — each test corresponds to a correctness or +security requirement that reviewers should check. + +Signature format: v1={HMAC-SHA256(secret, "{timestamp_ms}.{body}")} +Timestamp: Unix milliseconds (same as backend Date.now()) +Headers: x-commune-signature, x-commune-timestamp +""" +import hashlib +import hmac +import json +import time + +import pytest + +from commune.webhooks import verify_signature, WebhookVerificationError + + +def _sign(payload: bytes, secret: str, timestamp_ms: str) -> str: + """Replicate the backend signing protocol: v1=HMAC-SHA256(secret, '{ts}.{body}').""" + digest = hmac.new( + secret.encode("utf-8"), + f"{timestamp_ms}.".encode("utf-8") + payload, + hashlib.sha256, + ).hexdigest() + return f"v1={digest}" + + +class TestWebhookSignatureVerification: + """Verify that the webhook handler enforces signature validation.""" + + def test_valid_signature_is_accepted(self): + """A correctly signed payload should pass verification.""" + payload = json.dumps({"event": "message.received"}).encode() + secret = "test_secret_abc123" + timestamp_ms = str(int(time.time() * 1000)) + sig = _sign(payload, secret, timestamp_ms) + # Should not raise and should return True + result = verify_signature(payload=payload, signature=sig, secret=secret, timestamp=timestamp_ms) + assert result is True + + def test_invalid_signature_raises(self): + """A payload with a wrong signature must be rejected.""" + payload = json.dumps({"event": "message.received"}).encode() + timestamp_ms = str(int(time.time() * 1000)) + with pytest.raises(WebhookVerificationError): + verify_signature( + payload=payload, + signature="v1=invalid_sig_hex_that_will_not_match", + secret="secret", + timestamp=timestamp_ms, + ) + + def test_raw_bytes_not_parsed_dict(self): + """Verification must use raw bytes, not a re-serialized dict. + + Parsing and re-serializing a JSON body changes whitespace and key + ordering, so the HMAC computed over re-serialized bytes will not match + the signature computed by the backend over the original wire bytes. + Webhook handlers MUST pass request.body / request.get_data() directly. + """ + raw = b'{"event":"message.received","data":{"id":1}}' + re_serialized = json.dumps(json.loads(raw)).encode() + # Confirm test setup: bytes differ after round-trip + assert raw != re_serialized, "Test setup: re-serialization should produce different bytes" + + secret = "test_secret" + timestamp_ms = str(int(time.time() * 1000)) + valid_sig = _sign(raw, secret, timestamp_ms) + + # Signature is valid for the original raw bytes + assert verify_signature(payload=raw, signature=valid_sig, secret=secret, timestamp=timestamp_ms) is True + + # But NOT for re-serialized bytes — they produce a different HMAC + with pytest.raises(WebhookVerificationError): + verify_signature(payload=re_serialized, signature=valid_sig, secret=secret, timestamp=timestamp_ms) + + def test_expired_timestamp_is_rejected(self): + """Payloads more than 5 minutes old should be rejected (replay protection). + + The default tolerance is 300 seconds. A timestamp 6+ minutes old + (360 000 ms) must be refused regardless of signature correctness. + """ + payload = json.dumps({"event": "message.received"}).encode() + secret = "test_secret" + # 10 minutes ago in milliseconds + old_ts_ms = str(int(time.time() * 1000) - 600_000) + old_sig = _sign(payload, secret, old_ts_ms) + with pytest.raises(WebhookVerificationError, match="too old"): + verify_signature( + payload=payload, + signature=old_sig, + secret=secret, + timestamp=old_ts_ms, + tolerance_seconds=300, + ) + + def test_empty_secret_is_rejected(self): + """An empty webhook secret must never accept any payload. + + An empty secret means the signing key has not been configured. + Accepting requests without a real secret would allow anyone to + craft a valid-looking webhook delivery. + """ + payload = b'{"event": "test"}' + with pytest.raises((WebhookVerificationError, ValueError)): + verify_signature( + payload=payload, + signature="v1=any_sig", + secret="", + timestamp=str(int(time.time() * 1000)), + ) + + def test_missing_signature_is_rejected(self): + """A missing signature header must fail verification. + + An empty signature string represents a missing x-commune-signature + header. The handler must reject the request before any HMAC work. + """ + payload = b'{"event": "test"}' + with pytest.raises(WebhookVerificationError, match="Missing signature"): + verify_signature( + payload=payload, + signature="", + secret="secret", + timestamp=str(int(time.time() * 1000)), + ) diff --git a/tests/review/test_thread_continuity_spec.py b/tests/review/test_thread_continuity_spec.py new file mode 100644 index 0000000..7121954 --- /dev/null +++ b/tests/review/test_thread_continuity_spec.py @@ -0,0 +1,129 @@ +""" +Behavioral specification: email thread continuity. + +These tests describe the invariants that any email agent MUST maintain +to ensure conversations are properly threaded in the recipient's email client. + +Key rule: every outbound reply must carry the thread_id from the triggering +inbound message. Omitting thread_id starts a new thread instead of replying, +breaking conversation history for both the agent and the customer. +""" +import pytest + + +class TestThreadIdPropagation: + """Specify that thread_id must be preserved and propagated through reply chains.""" + + def test_send_result_has_thread_id(self): + """messages.send() must return a thread_id for use in subsequent replies. + + The thread_id from SendMessageResult is the value to pass back into + messages.send(thread_id=...) for the next turn. Agents that discard + this value will lose thread continuity on the second reply. + """ + from commune.types import SendMessageResult + + result = SendMessageResult( + id="doc_001", + message_id="", + thread_id="t_abc123", + status="queued", + ) + assert result.thread_id is not None, "send() result must include thread_id" + assert result.thread_id.startswith("t_"), "thread_id should have expected prefix format" + + def test_reply_requires_thread_id(self): + """A reply to a customer must always include the original thread_id. + + Code that omits thread_id when replying is a correctness bug. + The correct call signature always passes thread_id as a keyword argument. + """ + from unittest.mock import MagicMock + + mock_client = MagicMock() + mock_client.messages.send.return_value = MagicMock( + thread_id="t_xyz", + message_id="", + status="queued", + ) + + # Correct pattern: thread_id is explicitly passed + mock_client.messages.send( + to="customer@example.com", + subject="Re: Help with order", + text="Here is your answer...", + inbox_id="i_support", + thread_id="t_original", + ) + + call_kwargs = mock_client.messages.send.call_args.kwargs + assert "thread_id" in call_kwargs, "Reply MUST include thread_id" + assert call_kwargs["thread_id"] == "t_original" + + def test_thread_id_from_webhook_payload(self): + """The thread_id required for replies is available in the webhook payload. + + A Commune webhook payload for an inbound message includes thread_id + at data.thread_id. Agents MUST read from this path — not from a + stored lookup — so that replies stay in the correct thread even when + the customer starts a new conversation. + """ + webhook_payload = { + "event": "message.received", + "data": { + "thread_id": "t_abc123", + "inbox_id": "i_support", + "sender": "customer@example.com", + "subject": "I need help", + "text": "Please help me with my order", + }, + } + + # The thread_id MUST be accessible at this exact path + thread_id = webhook_payload["data"]["thread_id"] + assert thread_id == "t_abc123" + assert thread_id is not None, "webhook payload must provide thread_id" + + def test_thread_list_provides_thread_ids(self): + """threads.list() must return Thread objects with thread_id for polling workflows. + + Agents that poll instead of using webhooks iterate threads.list().data + and must find a usable thread_id on each Thread object. The last_direction + field tells the agent whether a reply is needed. + """ + from commune.types import Thread, ThreadList + + mock_thread = Thread( + thread_id="t_abc123", + subject="Customer inquiry", + message_count=2, + last_direction="inbound", + last_message_at="2024-01-15T10:00:00Z", + ) + thread_list = ThreadList(data=[mock_thread], has_more=False) + + assert len(thread_list.data) == 1 + assert thread_list.data[0].thread_id == "t_abc123" + assert thread_list.data[0].last_direction == "inbound" + + def test_message_has_thread_id_field(self): + """Every Message object must have a thread_id field for building reply context. + + When an agent loads a conversation via threads.messages(thread_id), + each returned Message carries its own thread_id. This allows the agent + to reconstruct the reply target even when messages are processed out of order. + """ + from commune.types import Message, MessageMetadata, Participant + + msg = Message( + message_id="", + thread_id="t_abc123", + direction="inbound", + content="Hello, I need help with my order.", + created_at="2024-01-15T10:00:00Z", + metadata=MessageMetadata(created_at="2024-01-15T10:00:00Z"), + ) + + assert msg.thread_id == "t_abc123" + assert msg.content == "Hello, I need help with my order." + assert msg.direction == "inbound"