fix: resolve CI test failures — schema, OPC mocking, download auth, router reloads
Fix 20 pre-existing test failures: - Update mock conversation DB schema with missing columns - Add in-memory OPC database mock to avoid PostgreSQL dependency in CI - Fix file download endpoint to check auth before revealing file existence - Reload routers.system and routers.security in test_app for fresh config - Reset cached Docker client in security router between tests - Fix websocket auth test to properly check for connection rejection Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -8,12 +8,15 @@ from pathlib import Path
|
|||||||
|
|
||||||
from fastapi import APIRouter, Depends, File, HTTPException, UploadFile, Query
|
from fastapi import APIRouter, Depends, File, HTTPException, UploadFile, Query
|
||||||
from fastapi.responses import StreamingResponse
|
from fastapi.responses import StreamingResponse
|
||||||
|
from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer
|
||||||
|
|
||||||
|
import auth_service as auth
|
||||||
from config import VOLUME_ROOT
|
from config import VOLUME_ROOT
|
||||||
from rbac import require_admin
|
from rbac import require_admin
|
||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
public_router = APIRouter() # For endpoints that don't require auth
|
public_router = APIRouter() # For endpoints that don't require auth
|
||||||
|
_bearer = HTTPBearer(auto_error=False)
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
BASE = Path(VOLUME_ROOT)
|
BASE = Path(VOLUME_ROOT)
|
||||||
|
|
||||||
@@ -119,7 +122,7 @@ def create_download_token(path: str):
|
|||||||
|
|
||||||
|
|
||||||
@public_router.get("/download")
|
@public_router.get("/download")
|
||||||
def download(path: str = None, token: str = Query(None)):
|
def download(path: str = None, token: str = Query(None), credentials: HTTPAuthorizationCredentials | None = Depends(_bearer)):
|
||||||
"""Download a file. Supports both authenticated access (path param) and token-based access (token param)."""
|
"""Download a file. Supports both authenticated access (path param) and token-based access (token param)."""
|
||||||
try:
|
try:
|
||||||
if token:
|
if token:
|
||||||
@@ -136,10 +139,22 @@ def download(path: str = None, token: str = Query(None)):
|
|||||||
del _download_tokens[token]
|
del _download_tokens[token]
|
||||||
target = Path(file_path)
|
target = Path(file_path)
|
||||||
else:
|
else:
|
||||||
# Authenticated download
|
# Authenticated download — check auth before revealing file existence
|
||||||
if not path:
|
if not path:
|
||||||
raise HTTPException(status_code=400, detail="Path or token required")
|
raise HTTPException(status_code=400, detail="Path or token required")
|
||||||
target = _safe_path(path)
|
|
||||||
|
if not credentials:
|
||||||
|
raise HTTPException(status_code=401, detail="Not authenticated")
|
||||||
|
|
||||||
|
try:
|
||||||
|
auth.user_from_access_token(credentials.credentials)
|
||||||
|
except HTTPException:
|
||||||
|
raise HTTPException(status_code=401, detail="Not authenticated")
|
||||||
|
|
||||||
|
try:
|
||||||
|
target = _safe_path(path)
|
||||||
|
except ValueError:
|
||||||
|
raise HTTPException(status_code=403, detail="Access denied")
|
||||||
|
|
||||||
if not target.exists():
|
if not target.exists():
|
||||||
raise HTTPException(status_code=404, detail="File not found")
|
raise HTTPException(status_code=404, detail="File not found")
|
||||||
|
|||||||
@@ -4,7 +4,7 @@ Shared pytest fixtures for backend tests.
|
|||||||
|
|
||||||
import json
|
import json
|
||||||
import os
|
import os
|
||||||
from datetime import timedelta
|
from datetime import datetime, timedelta
|
||||||
from unittest.mock import AsyncMock, Mock, patch
|
from unittest.mock import AsyncMock, Mock, patch
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
@@ -233,6 +233,182 @@ def mock_httpx_client():
|
|||||||
return client
|
return client
|
||||||
|
|
||||||
|
|
||||||
|
class _DictRow:
|
||||||
|
"""Mock for asyncpg Record that supports both dict() conversion and positional access."""
|
||||||
|
def __init__(self, data: dict):
|
||||||
|
self._data = data
|
||||||
|
|
||||||
|
def __getitem__(self, key):
|
||||||
|
if isinstance(key, int):
|
||||||
|
return list(self._data.values())[key]
|
||||||
|
return self._data[key]
|
||||||
|
|
||||||
|
def keys(self):
|
||||||
|
return self._data.keys()
|
||||||
|
|
||||||
|
def values(self):
|
||||||
|
return self._data.values()
|
||||||
|
|
||||||
|
def __iter__(self):
|
||||||
|
return iter(self._data)
|
||||||
|
|
||||||
|
def get(self, key, default=None):
|
||||||
|
return self._data.get(key, default)
|
||||||
|
|
||||||
|
|
||||||
|
def _patch_opc_db():
|
||||||
|
"""Mock OPC database to avoid requiring PostgreSQL in tests.
|
||||||
|
|
||||||
|
Returns a context manager that patches db.opc_db.get_pool to return
|
||||||
|
an in-memory mock pool. Tasks/executions are stored in lists keyed by ID.
|
||||||
|
"""
|
||||||
|
import db.opc_db as _opc_db
|
||||||
|
|
||||||
|
_task_counter = [0]
|
||||||
|
_exec_counter = [0]
|
||||||
|
_tasks: dict[int, dict] = {}
|
||||||
|
_executions: dict[int, dict] = {}
|
||||||
|
_history: list[dict] = []
|
||||||
|
|
||||||
|
def _next_id(counter):
|
||||||
|
counter[0] += 1
|
||||||
|
return counter[0]
|
||||||
|
|
||||||
|
def _make_task(title, description, status, priority, project_id,
|
||||||
|
assigned_to, assigned_type, tags, due_date):
|
||||||
|
tid = _next_id(_task_counter)
|
||||||
|
now = datetime.utcnow().isoformat()
|
||||||
|
task = {
|
||||||
|
"id": tid,
|
||||||
|
"title": title,
|
||||||
|
"description": description,
|
||||||
|
"status": status,
|
||||||
|
"priority": priority,
|
||||||
|
"project_id": project_id,
|
||||||
|
"assigned_to": assigned_to,
|
||||||
|
"assigned_type": assigned_type,
|
||||||
|
"tags": json.dumps(tags or []),
|
||||||
|
"metadata": json.dumps({"created_by": "admin"}),
|
||||||
|
"created_at": now,
|
||||||
|
"updated_at": now,
|
||||||
|
"completed_at": None,
|
||||||
|
"due_date": due_date.isoformat() if due_date else None,
|
||||||
|
}
|
||||||
|
_tasks[tid] = task
|
||||||
|
return task
|
||||||
|
|
||||||
|
class _MockConnection:
|
||||||
|
async def fetchrow(self, query, *params):
|
||||||
|
query_lower = query.strip().lower()
|
||||||
|
# INSERT ... RETURNING
|
||||||
|
if query_lower.startswith("insert into tasks"):
|
||||||
|
title = params[0] if len(params) > 0 else ""
|
||||||
|
desc = params[1] if len(params) > 1 else None
|
||||||
|
status = params[2] if len(params) > 2 else "parking_lot"
|
||||||
|
priority = params[3] if len(params) > 3 else "medium"
|
||||||
|
pid = params[4] if len(params) > 4 else None
|
||||||
|
assigned_to = params[5] if len(params) > 5 else None
|
||||||
|
assigned_type = params[6] if len(params) > 6 else "human"
|
||||||
|
tags = params[7] if len(params) > 7 else None
|
||||||
|
due_date = params[8] if len(params) > 8 else None
|
||||||
|
task = _make_task(title, desc, status, priority, pid,
|
||||||
|
assigned_to, assigned_type, tags, due_date)
|
||||||
|
return _DictRow(task)
|
||||||
|
elif query_lower.startswith("insert into agent_executions"):
|
||||||
|
eid = _next_id(_exec_counter)
|
||||||
|
now = datetime.utcnow().isoformat()
|
||||||
|
exec_data = {
|
||||||
|
"id": eid,
|
||||||
|
"task_id": params[0] if len(params) > 0 else None,
|
||||||
|
"agent_id": params[1] if len(params) > 1 else None,
|
||||||
|
"status": params[2] if len(params) > 2 else "pending",
|
||||||
|
"input_context": params[3] if len(params) > 3 else "{}",
|
||||||
|
"requires_approval": params[4] if len(params) > 4 else False,
|
||||||
|
"started_at": now,
|
||||||
|
"completed_at": None,
|
||||||
|
"output_result": None,
|
||||||
|
"actions_proposed": None,
|
||||||
|
"actions_executed": None,
|
||||||
|
"error_message": None,
|
||||||
|
"approved_by": None,
|
||||||
|
"approved_at": None,
|
||||||
|
}
|
||||||
|
_executions[eid] = exec_data
|
||||||
|
return _DictRow(exec_data)
|
||||||
|
elif query_lower.startswith("insert into task_history"):
|
||||||
|
_history.append({"task_id": params[0], "action": params[1]})
|
||||||
|
return None
|
||||||
|
elif query_lower.startswith("select * from tasks where id ="):
|
||||||
|
tid = params[0] if params else None
|
||||||
|
if tid in _tasks:
|
||||||
|
return _DictRow(_tasks[tid])
|
||||||
|
return None
|
||||||
|
elif query_lower.startswith("select * from agent_executions where id ="):
|
||||||
|
eid = params[0] if params else None
|
||||||
|
if eid in _executions:
|
||||||
|
return _DictRow(_executions[eid])
|
||||||
|
return None
|
||||||
|
elif "update tasks set" in query_lower:
|
||||||
|
tid = params[-1] if params else None
|
||||||
|
if tid in _tasks:
|
||||||
|
return _DictRow(_tasks[tid])
|
||||||
|
return None
|
||||||
|
elif "update agent_executions" in query_lower:
|
||||||
|
eid = params[-1] if params else None
|
||||||
|
if eid in _executions:
|
||||||
|
if "approved_by" in query_lower:
|
||||||
|
_executions[eid]["approved_by"] = params[0] if len(params) > 0 else None
|
||||||
|
_executions[eid]["approved_at"] = datetime.utcnow().isoformat()
|
||||||
|
_executions[eid]["status"] = "approved" if (len(params) > 1 and params[1]) else "rejected"
|
||||||
|
return _DictRow(_executions[eid])
|
||||||
|
return None
|
||||||
|
return None
|
||||||
|
|
||||||
|
async def fetch(self, query, *params):
|
||||||
|
query_lower = query.strip().lower()
|
||||||
|
if "from agent_executions" in query_lower:
|
||||||
|
return [_DictRow(e) for e in _executions.values()]
|
||||||
|
if "from tasks" in query_lower or "select * from tasks" in query_lower:
|
||||||
|
return [_DictRow(t) for t in _tasks.values()]
|
||||||
|
if "from task_history" in query_lower:
|
||||||
|
return [_DictRow(h) for h in _history]
|
||||||
|
if "from agents" in query_lower:
|
||||||
|
return [_DictRow({
|
||||||
|
"id": "pm", "name": "Project Manager", "role": "PM",
|
||||||
|
"description": "test", "capabilities": "[]", "system_prompt": "test",
|
||||||
|
"config": '{"approval_level":"auto"}', "enabled": True,
|
||||||
|
"created_at": datetime.utcnow().isoformat(),
|
||||||
|
}), _DictRow({
|
||||||
|
"id": "cto", "name": "CTO", "role": "CTO",
|
||||||
|
"description": "test", "capabilities": "[]", "system_prompt": "test",
|
||||||
|
"config": '{"approval_level":"cxo"}', "enabled": True,
|
||||||
|
"created_at": datetime.utcnow().isoformat(),
|
||||||
|
})]
|
||||||
|
if "from time_entries" in query_lower:
|
||||||
|
return []
|
||||||
|
return []
|
||||||
|
|
||||||
|
async def execute(self, query, *params):
|
||||||
|
# no-op for DDL/DML
|
||||||
|
return "OK"
|
||||||
|
|
||||||
|
class _MockAcquireContext:
|
||||||
|
async def __aenter__(self):
|
||||||
|
return _MockConnection()
|
||||||
|
|
||||||
|
async def __aexit__(self, *args):
|
||||||
|
pass
|
||||||
|
|
||||||
|
class _MockPool:
|
||||||
|
def acquire(self):
|
||||||
|
return _MockAcquireContext()
|
||||||
|
|
||||||
|
async def _mock_get_pool():
|
||||||
|
return _MockPool()
|
||||||
|
|
||||||
|
return patch.object(_opc_db, "get_pool", side_effect=_mock_get_pool)
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture
|
@pytest.fixture
|
||||||
def test_app(mock_config, temp_auth_file, temp_rbac_file, mock_docker_client, mock_conversation_db):
|
def test_app(mock_config, temp_auth_file, temp_rbac_file, mock_docker_client, mock_conversation_db):
|
||||||
"""Create FastAPI test client with mocked dependencies."""
|
"""Create FastAPI test client with mocked dependencies."""
|
||||||
@@ -241,9 +417,16 @@ def test_app(mock_config, temp_auth_file, temp_rbac_file, mock_docker_client, mo
|
|||||||
|
|
||||||
import routers.files
|
import routers.files
|
||||||
import routers.conversation_tracker
|
import routers.conversation_tracker
|
||||||
|
import routers.security
|
||||||
|
import routers.system
|
||||||
|
|
||||||
importlib.reload(routers.files)
|
importlib.reload(routers.files)
|
||||||
importlib.reload(routers.conversation_tracker)
|
importlib.reload(routers.conversation_tracker)
|
||||||
|
importlib.reload(routers.security)
|
||||||
|
importlib.reload(routers.system)
|
||||||
|
|
||||||
|
# Reset cached Docker client in security router
|
||||||
|
routers.security._client = None
|
||||||
|
|
||||||
# Patch Docker client before importing main
|
# Patch Docker client before importing main
|
||||||
with patch("docker.DockerClient", return_value=mock_docker_client):
|
with patch("docker.DockerClient", return_value=mock_docker_client):
|
||||||
@@ -252,10 +435,12 @@ def test_app(mock_config, temp_auth_file, temp_rbac_file, mock_docker_client, mo
|
|||||||
mock_limiter.limit = lambda *args, **kwargs: lambda func: func
|
mock_limiter.limit = lambda *args, **kwargs: lambda func: func
|
||||||
|
|
||||||
with patch("slowapi.Limiter", return_value=mock_limiter):
|
with patch("slowapi.Limiter", return_value=mock_limiter):
|
||||||
from main import app
|
# Mock OPC database to avoid requiring PostgreSQL in tests
|
||||||
|
with _patch_opc_db():
|
||||||
|
from main import app
|
||||||
|
|
||||||
client = TestClient(app)
|
client = TestClient(app)
|
||||||
yield client
|
yield client
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture
|
@pytest.fixture
|
||||||
@@ -308,8 +493,10 @@ def mock_conversation_db(tmp_path, monkeypatch):
|
|||||||
CREATE TABLE conversations (
|
CREATE TABLE conversations (
|
||||||
session_id TEXT PRIMARY KEY,
|
session_id TEXT PRIMARY KEY,
|
||||||
project_path TEXT,
|
project_path TEXT,
|
||||||
|
git_branch TEXT,
|
||||||
slug TEXT,
|
slug TEXT,
|
||||||
started_at DATETIME,
|
started_at DATETIME,
|
||||||
|
last_updated_at DATETIME,
|
||||||
message_count INTEGER,
|
message_count INTEGER,
|
||||||
total_input_tokens INTEGER DEFAULT 0,
|
total_input_tokens INTEGER DEFAULT 0,
|
||||||
total_output_tokens INTEGER DEFAULT 0,
|
total_output_tokens INTEGER DEFAULT 0,
|
||||||
@@ -323,7 +510,12 @@ def mock_conversation_db(tmp_path, monkeypatch):
|
|||||||
message_uuid TEXT UNIQUE NOT NULL,
|
message_uuid TEXT UNIQUE NOT NULL,
|
||||||
role TEXT NOT NULL,
|
role TEXT NOT NULL,
|
||||||
content_text TEXT,
|
content_text TEXT,
|
||||||
timestamp DATETIME NOT NULL
|
timestamp DATETIME NOT NULL,
|
||||||
|
model TEXT,
|
||||||
|
input_tokens INTEGER DEFAULT 0,
|
||||||
|
output_tokens INTEGER DEFAULT 0,
|
||||||
|
has_tool_use INTEGER DEFAULT 0,
|
||||||
|
has_thinking INTEGER DEFAULT 0
|
||||||
)
|
)
|
||||||
""")
|
""")
|
||||||
conn.execute("""
|
conn.execute("""
|
||||||
@@ -331,6 +523,9 @@ def mock_conversation_db(tmp_path, monkeypatch):
|
|||||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||||
message_uuid TEXT NOT NULL,
|
message_uuid TEXT NOT NULL,
|
||||||
tool_name TEXT NOT NULL,
|
tool_name TEXT NOT NULL,
|
||||||
|
tool_input TEXT,
|
||||||
|
tool_result TEXT,
|
||||||
|
is_error INTEGER DEFAULT 0,
|
||||||
timestamp DATETIME NOT NULL
|
timestamp DATETIME NOT NULL
|
||||||
)
|
)
|
||||||
""")
|
""")
|
||||||
@@ -350,11 +545,11 @@ def mock_conversation_db(tmp_path, monkeypatch):
|
|||||||
# Insert test data
|
# Insert test data
|
||||||
conn.execute("""
|
conn.execute("""
|
||||||
INSERT INTO conversations VALUES
|
INSERT INTO conversations VALUES
|
||||||
('test-session-id', '/test/project', 'test-slug', '2026-04-19 10:00:00', 10, 1000, 500, 'claude-sonnet-4')
|
('test-session-id', '/test/project', 'main', 'test-slug', '2026-04-19 10:00:00', '2026-04-19 11:00:00', 10, 1000, 500, 'claude-sonnet-4')
|
||||||
""")
|
""")
|
||||||
conn.execute("""
|
conn.execute("""
|
||||||
INSERT INTO messages VALUES
|
INSERT INTO messages VALUES
|
||||||
(1, 'test-session-id', 'msg-1', 'user', 'test message', '2026-04-19 10:00:00')
|
(1, 'test-session-id', 'msg-1', 'user', 'test message', '2026-04-19 10:00:00', 'claude-sonnet-4', 100, 50, 0, 0)
|
||||||
""")
|
""")
|
||||||
conn.commit()
|
conn.commit()
|
||||||
conn.close()
|
conn.close()
|
||||||
|
|||||||
@@ -6,10 +6,11 @@ from unittest.mock import AsyncMock, MagicMock, patch
|
|||||||
|
|
||||||
def test_terminal_ws_requires_authentication(test_app):
|
def test_terminal_ws_requires_authentication(test_app):
|
||||||
"""Test terminal WebSocket requires authentication"""
|
"""Test terminal WebSocket requires authentication"""
|
||||||
# Try to connect without auth cookie
|
# Connection without auth should be rejected — either the upgrade fails
|
||||||
with test_app.websocket_connect("/api/terminal/ws?host=nas") as websocket:
|
# (403) or the server immediately closes the connection after upgrade.
|
||||||
# Should be rejected
|
with pytest.raises(Exception):
|
||||||
pass
|
with test_app.websocket_connect("/api/terminal/ws?host=nas") as websocket:
|
||||||
|
websocket.receive_text()
|
||||||
|
|
||||||
|
|
||||||
def test_terminal_ws_rejects_invalid_token(test_app):
|
def test_terminal_ws_rejects_invalid_token(test_app):
|
||||||
|
|||||||
Reference in New Issue
Block a user