code-review fixes: fsync, entropy, is_dm propagation, tests

- Add os.fsync with Windows-compatible directory flush in pairing store
- Increase pairing code length from 6 -> 8 characters for higher entropy
- Remove SystemExit on empty allowFrom; empty list now defers to pairing
- Update is_allowed docstring to document pairing fallback semantics
- Propagate is_dm to Matrix (direct rooms) and Slack (im channels)
- Slack _is_allowed now checks pairing store for DM allowlist mode
- Fix /pairing revoke to accept optional channel argument
- Move inline import time to module top-level
- Add WebSocket comment explaining is_dm=True assumption
- Add comprehensive tests for store and BaseChannel pairing integration
- Fix existing tests that expected empty allowFrom to hard-exit

Refs #3774
This commit is contained in:
chengyongru
2026-05-15 15:46:44 +08:00
committed by Xubin Ren
parent 4c4a9ae590
commit f8e7e50759
9 changed files with 265 additions and 33 deletions
+99 -1
View File
@@ -1,5 +1,7 @@
from types import SimpleNamespace
import pytest
from nanobot.bus.events import OutboundMessage
from nanobot.bus.queue import MessageBus
from nanobot.channels.base import BaseChannel
@@ -7,6 +9,11 @@ from nanobot.channels.base import BaseChannel
class _DummyChannel(BaseChannel):
name = "dummy"
_sent: list[OutboundMessage]
def __init__(self, config, bus):
super().__init__(config, bus)
self._sent = []
async def start(self) -> None:
return None
@@ -15,7 +22,7 @@ class _DummyChannel(BaseChannel):
return None
async def send(self, msg: OutboundMessage) -> None:
return None
self._sent.append(msg)
def test_is_allowed_requires_exact_match() -> None:
@@ -35,3 +42,94 @@ def test_is_allowed_denies_empty_dict_allow_from() -> None:
channel = _DummyChannel({"allow_from": []}, MessageBus())
assert channel.is_allowed("alice") is False
def test_is_allowed_star_allows_all() -> None:
channel = _DummyChannel({"allowFrom": ["*"]}, MessageBus())
assert channel.is_allowed("anyone") is True
def test_is_allowed_pairing_fallback(monkeypatch) -> None:
channel = _DummyChannel({"allowFrom": []}, MessageBus())
monkeypatch.setattr(
"nanobot.channels.base.is_approved", lambda _ch, sid: sid == "paired"
)
assert channel.is_allowed("paired") is True
assert channel.is_allowed("unknown") is False
@pytest.mark.asyncio
async def test_handle_message_dm_sends_pairing_code(monkeypatch) -> None:
channel = _DummyChannel({"allowFrom": []}, MessageBus())
monkeypatch.setattr(
"nanobot.channels.base.generate_code", lambda _ch, sid: "ABCD-EFGH"
)
await channel._handle_message(
sender_id="stranger", chat_id="chat1", content="hello", is_dm=True
)
assert len(channel._sent) == 1
msg = channel._sent[0]
assert "ABCD-EFGH" in msg.content
assert msg.metadata.get("_pairing_code") == "ABCD-EFGH"
@pytest.mark.asyncio
async def test_handle_message_group_ignores_unknown() -> None:
channel = _DummyChannel({"allowFrom": []}, MessageBus())
await channel._handle_message(
sender_id="stranger", chat_id="chat1", content="hello", is_dm=False
)
assert channel._sent == []
@pytest.mark.asyncio
async def test_handle_pairing_command_list(monkeypatch) -> None:
channel = _DummyChannel({"allowFrom": ["owner"]}, MessageBus())
monkeypatch.setattr(
"nanobot.channels.base.list_pending",
lambda: [
{
"code": "ABCD-EFGH",
"channel": "dummy",
"sender_id": "123",
"expires_at": 9999999999,
}
],
)
await channel._handle_pairing_command("owner", "chat1", "/pairing list")
assert len(channel._sent) == 1
assert "ABCD-EFGH" in channel._sent[0].content
@pytest.mark.asyncio
async def test_handle_pairing_command_approve(monkeypatch) -> None:
channel = _DummyChannel({"allowFrom": ["owner"]}, MessageBus())
monkeypatch.setattr(
"nanobot.channels.base.approve_code",
lambda code: ("dummy", "123") if code == "ABCD-EFGH" else None,
)
await channel._handle_pairing_command("owner", "chat1", "/pairing approve ABCD-EFGH")
assert len(channel._sent) == 1
assert "Approved" in channel._sent[0].content
@pytest.mark.asyncio
async def test_handle_pairing_command_revoke(monkeypatch) -> None:
channel = _DummyChannel({"allowFrom": ["owner"]}, MessageBus())
monkeypatch.setattr(
"nanobot.channels.base.revoke",
lambda ch, sid: sid == "123",
)
await channel._handle_pairing_command("owner", "chat1", "/pairing revoke 123")
assert len(channel._sent) == 1
assert "Revoked" in channel._sent[0].content
+7 -12
View File
@@ -961,8 +961,8 @@ class _StartableChannel(BaseChannel):
@pytest.mark.asyncio
async def test_validate_allow_from_raises_on_empty_list():
"""_validate_allow_from should raise SystemExit when allow_from is empty list."""
async def test_validate_allow_from_allows_empty_list():
"""Empty allow_from is valid now — pairing store handles unapproved senders."""
fake_config = SimpleNamespace(
channels=ChannelsConfig(),
providers=SimpleNamespace(groq=SimpleNamespace(api_key="")),
@@ -973,10 +973,8 @@ async def test_validate_allow_from_raises_on_empty_list():
mgr.channels = {"test": _ChannelWithAllowFrom(fake_config, None, [])}
mgr._dispatch_task = None
with pytest.raises(SystemExit) as exc_info:
mgr._validate_allow_from()
assert "empty allowFrom" in str(exc_info.value)
# Should not raise — empty list defers to pairing store
mgr._validate_allow_from()
@pytest.mark.asyncio
@@ -997,8 +995,8 @@ async def test_validate_allow_from_passes_with_asterisk():
@pytest.mark.asyncio
async def test_validate_allow_from_raises_on_empty_dict_allow_from():
"""_validate_allow_from should reject empty dict-backed allow_from lists."""
async def test_validate_allow_from_allows_empty_dict_allow_from():
"""Empty dict-backed allow_from is valid — pairing store handles approval."""
fake_config = SimpleNamespace(
channels=ChannelsConfig(),
providers=SimpleNamespace(groq=SimpleNamespace(api_key="")),
@@ -1009,10 +1007,7 @@ async def test_validate_allow_from_raises_on_empty_dict_allow_from():
mgr.channels = {"test": _ChannelWithAllowFrom({"enabled": True}, None, [])}
mgr._dispatch_task = None
with pytest.raises(SystemExit) as exc_info:
mgr._validate_allow_from()
assert "empty allowFrom" in str(exc_info.value)
mgr._validate_allow_from()
@pytest.mark.asyncio