mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-06-15 17:25:26 -04:00
35b4dd2824
* docs: add implementation plan for fixing chat context drifting (#135) * fix: make Session.history immutable + fix {}.history crash - Session.history now exposes a COPY of the internal _history list - add_message() replaces history with a fresh copy each time - get_context_messages() derives from _history directly - replace_messages() updates both _history and history - truncate_messages() updates both _history and history - _persist_message() line 207: fixed {}.history fallback crash - Added 11 tests for session isolation and edge cases Addresses #135 root cause #1: shared mutable references * fix: task scheduler uses SessionManager methods instead of overwriting sessions - Added ensure_task_session() to SessionManager (checks cache first) - Task scheduler now uses ensure_task_session() instead of direct dict assignment - Task scheduler now uses SessionManager.add_message() for message persistence - Removed direct sess_obj.history.append() that was silently losing data Addresses #135 root causes #2 and #3 * fix: add age guard to cleanup_empty_sessions — don't delete sessions <1h old Prevents the cleanup task from deleting sessions that were just created and haven't received any messages yet (message_count == 0). Addresses #135 root cause #5 * test: comprehensive session isolation tests (10/10 passing) * refactor: consolidate _session_manager into singleton pattern - Added set_session_manager_instance / get_session_manager_instance to core/models - kept backward-compat aliases (set_session_manager, get_session_manager) - session_manager.py re-exports the singleton functions - ai_interaction.set_session_manager now syncs with the core singleton - context_compactor uses get_session_manager_instance() instead of getattr hack - app.py initializes the singleton once Addresses #135 root cause #4: fragile global wiring * test: add concurrent session isolation integration tests Verifies: - Concurrent add_message to different sessions doesn't cross-contaminate - Rapid parallel writes maintain isolation - Read-write concurrent access is safe All 3 async tests pass, proving the immutable history fix works under concurrency * fix: pre-import core.models in conftest to prevent test pollution test_agent_loop.py stubs sys.modules['core.models'] = MagicMock() at module level during collection. Any test collected after it imports Session as a MagicMock. Pre-importing core.models in conftest.py before test_agent_loop.py's module-level code runs prevents this. * fix: make .history authoritative mutable list, address PR review Per review feedback: keep .history as the authoritative mutable list so existing code doing .history.pop(), .history = [...], etc. still works. Fix the cross-contamination bug by ensuring __post_init__() gives each Session its OWN unique history list (never shared). Changes: - core/models.py: .history IS the authoritative list. _history aliases it. Each Session gets its own list in __post_init__. - core/session_manager.py: add_message() delegates to Session.add_message() instead of appending directly — no double-append, single source of truth. - tests/test_session_manager.py: updated test to reflect that .history references see new messages (same list, not a snapshot). - docs/plans/2026-06-01-fix-chat-context-drifting.md: removed (not for shipping — useful design context but too much process/doc to ship). All 272 tests pass (3 pre-existing failures unrelated). * Fix session manager message persistence * Fix session history alias regressions * Fix session history aliasing and task delivery
116 lines
3.9 KiB
Python
116 lines
3.9 KiB
Python
"""Regression tests for task-result delivery into chat sessions (issue #326)."""
|
|
import asyncio
|
|
import sys
|
|
import types as _types
|
|
|
|
import pytest
|
|
|
|
sqlalchemy = pytest.importorskip("sqlalchemy")
|
|
if not isinstance(sqlalchemy, _types.ModuleType):
|
|
pytest.skip("sqlalchemy is stubbed in this environment", allow_module_level=True)
|
|
|
|
from sqlalchemy import create_engine
|
|
from sqlalchemy.orm import sessionmaker
|
|
|
|
from tests.helpers.import_state import clear_fake_database_modules
|
|
|
|
clear_fake_database_modules()
|
|
|
|
import core.database as cdb
|
|
from core.database import Base, Session as DbSession
|
|
from core.models import ChatMessage as MemChatMessage
|
|
from src.task_scheduler import TaskScheduler
|
|
|
|
# This test needs the real core.database (real SQLAlchemy Base/ChatMessage).
|
|
# test_null_owner_gates.py no longer leaks its stubs (per-test fixture cleanup
|
|
# since PR #1513), but several other files still install core.database stubs
|
|
# at module level without teardown (test_model_routes, test_companion_readonly,
|
|
# test_endpoint_probing, test_vault_password_not_in_argv). When any of those
|
|
# are collected before us, core.database is a stub and Base is a MagicMock.
|
|
# Skip in that case — the test passes correctly in isolation or when collected
|
|
# before the stubbing files.
|
|
if type(Base).__name__ == "MagicMock":
|
|
pytest.skip("core.database is stubbed — run this file in isolation", allow_module_level=True)
|
|
|
|
|
|
def _make_db():
|
|
engine = create_engine("sqlite:///:memory:")
|
|
Base.metadata.create_all(engine)
|
|
return sessionmaker(bind=engine)()
|
|
|
|
|
|
def _make_task():
|
|
return _types.SimpleNamespace(
|
|
id="task-1",
|
|
name="Chat Sessions Tidy",
|
|
prompt="tidy",
|
|
output_target="session",
|
|
endpoint_url=None,
|
|
model=None,
|
|
session_id=None,
|
|
owner=None,
|
|
crew_member_id=None,
|
|
)
|
|
|
|
|
|
def test_session_delivery_survives_empty_database(monkeypatch):
|
|
"""On a fresh/wiped database there is no session to inherit endpoint/model
|
|
from, so _resolve_defaults returns None. The delivery must still persist a
|
|
session instead of crashing on the NOT NULL constraint (issue #326)."""
|
|
monkeypatch.setitem(sys.modules, "core.database", cdb)
|
|
parent = sys.modules.get("core")
|
|
if parent is not None:
|
|
monkeypatch.setattr(parent, "database", cdb, raising=False)
|
|
|
|
db = _make_db()
|
|
scheduler = TaskScheduler.__new__(TaskScheduler)
|
|
scheduler._session_manager = None
|
|
|
|
asyncio.run(scheduler._deliver_task_result(_make_task(), "done", db))
|
|
|
|
sessions = db.query(DbSession).all()
|
|
assert len(sessions) == 1
|
|
assert sessions[0].endpoint_url == ""
|
|
assert sessions[0].model == ""
|
|
|
|
|
|
def test_session_delivery_uses_in_memory_messages_with_manager(monkeypatch):
|
|
"""Manager delivery must not construct the SQLAlchemy ChatMessage model."""
|
|
monkeypatch.setitem(sys.modules, "core.database", cdb)
|
|
parent = sys.modules.get("core")
|
|
if parent is not None:
|
|
monkeypatch.setattr(parent, "database", cdb, raising=False)
|
|
|
|
class RecordingManager:
|
|
def __init__(self):
|
|
self.messages = []
|
|
|
|
def add_message(self, session_id, message):
|
|
assert isinstance(message, MemChatMessage)
|
|
self.messages.append((session_id, message))
|
|
|
|
db = _make_db()
|
|
manager = RecordingManager()
|
|
scheduler = TaskScheduler.__new__(TaskScheduler)
|
|
scheduler._session_manager = manager
|
|
task = _make_task()
|
|
task.session_id = "existing-session"
|
|
task.endpoint_url = "http://endpoint"
|
|
task.model = "test-model"
|
|
|
|
asyncio.run(scheduler._deliver_task_result(task, "done", db))
|
|
|
|
assert [message.role for _, message in manager.messages] == [
|
|
"user",
|
|
"assistant",
|
|
]
|
|
assert [message.content for _, message in manager.messages] == [
|
|
"tidy",
|
|
"done",
|
|
]
|
|
assert all(session_id == "existing-session" for session_id, _ in manager.messages)
|
|
assert all(
|
|
message.metadata == {"model": "test-model"}
|
|
for _, message in manager.messages
|
|
)
|