50750f3183
Replaces per-provider logging.debug body with _list_models_for_provider_result SDK-boundary helper. Aggregates per-provider failures into self._model_fetch_errors and returns Result with aggregated errors. Stderr summary on partial failure. The SDK boundary (ai_client.list_models call) is the canonical place to catch vendor exceptions and convert to ErrorInfo(kind=NETWORK), per error_handling.md §'Boundary Types'. Audit: INTERNAL_SILENT_SWALLOW for src/app_controller.py: 23 -> 22.
469 lines
18 KiB
Python
469 lines
18 KiB
Python
"""
|
|
Tests for the data-oriented error handling convention applied to src/app_controller.py.
|
|
|
|
This file verifies the contract that the 32 INTERNAL_BROAD_CATCH + 8 INTERNAL_SILENT_SWALLOW
|
|
+ 4 INTERNAL_RETHROW + 1 INTERNAL_OPTIONAL_RETURN sites in src/app_controller.py are migrated
|
|
to the Result[T] pattern (per conductor/code_styleguides/error_handling.md).
|
|
|
|
The tests are pattern templates:
|
|
- test_offload_entry_payload_returns_dict - the _offload_entry_payload helper returns a dict
|
|
(the existing test for the regression fix is in test_app_controller_offloading.py; this
|
|
one is a shape check).
|
|
- test_migrated_method_returns_result_on_success - methods migrated to Result[T] return
|
|
Result[data=...] with no errors on the success path.
|
|
- test_migrated_method_returns_result_with_error_on_failure - methods migrated to Result[T]
|
|
return Result with errors=... on the failure path.
|
|
- test_app_controller_does_not_use_broad_except - static check: src/app_controller.py
|
|
has no `except Exception:` clauses left (all 32 are migrated to specific exceptions).
|
|
- test_offload_entry_payload_preserves_unchanged_payload - verifies the no-op path for
|
|
entries without tool_call or tool_result kinds.
|
|
"""
|
|
import pytest
|
|
from unittest.mock import MagicMock, patch
|
|
from src.app_controller import AppController, _install_sigint_exit_handler
|
|
from src.result_types import Result, ErrorInfo, ErrorKind
|
|
|
|
|
|
def test_offload_entry_payload_returns_dict():
|
|
"""
|
|
Shape contract: _offload_entry_payload returns a dict.
|
|
The actual happy-path and error-path tests are in test_app_controller_offloading.py.
|
|
"""
|
|
with patch("src.app_controller.performance_monitor.PerformanceMonitor"):
|
|
ctrl = AppController()
|
|
out = ctrl._offload_entry_payload({"kind": "request", "payload": {"message": "hi"}, "ts": "12:00:00"})
|
|
assert isinstance(out, dict)
|
|
assert out["kind"] == "request"
|
|
assert out["payload"]["message"] == "hi"
|
|
|
|
|
|
def test_migrated_method_returns_result_on_success():
|
|
"""
|
|
Pattern template: methods migrated to Result[T] return Result[data=...] with no errors
|
|
on the success path. The 5 callback-handler sites (Batch 1) include _handle_custom_callback
|
|
and _handle_click; both return Result[None] after migration.
|
|
"""
|
|
from src.app_controller import _handle_custom_callback
|
|
controller = MagicMock()
|
|
controller._predefined_callbacks = {}
|
|
# Use a no-op callback so the body runs without error
|
|
task = {"callback": (lambda: None), "args": []}
|
|
result = _handle_custom_callback(controller, task)
|
|
assert isinstance(result, Result)
|
|
assert result.ok is True
|
|
assert result.errors == []
|
|
|
|
|
|
def test_migrated_method_returns_result_with_error_on_failure():
|
|
"""
|
|
Pattern template: methods migrated to Result[T] return Result with errors=... when
|
|
the underlying call raises. The migrated callback sites catch the specific exception
|
|
(e.g. TypeError, ValueError) and convert it to ErrorInfo.
|
|
"""
|
|
from src.app_controller import _handle_custom_callback
|
|
controller = MagicMock()
|
|
controller._predefined_callbacks = {}
|
|
# Callback that raises a specific exception
|
|
def bad_cb():
|
|
raise ValueError("simulated failure")
|
|
task = {"callback": bad_cb, "args": []}
|
|
result = _handle_custom_callback(controller, task)
|
|
assert isinstance(result, Result)
|
|
assert result.ok is False
|
|
assert any("ValueError" in e.message or "simulated failure" in e.message for e in result.errors)
|
|
|
|
|
|
def test_app_controller_does_not_use_broad_except():
|
|
"""
|
|
Static check via the audit: src/app_controller.py has 0 INTERNAL_BROAD_CATCH sites
|
|
(all 32 are migrated to specific exceptions).
|
|
|
|
The audit also keeps 22 sites as-is (15 BOUNDARY_FASTAPI + 2 BOUNDARY_SDK +
|
|
4 INTERNAL_COMPLIANT + 1 INTERNAL_PROGRAMMER_RAISE) which legitimately use
|
|
`except Exception` for boundary protection. Those are not part of the
|
|
INTERNAL_BROAD_CATCH count.
|
|
|
|
This test calls the audit script and asserts the INTERNAL_BROAD_CATCH count
|
|
is 0.
|
|
"""
|
|
import json
|
|
import subprocess
|
|
r = subprocess.run(
|
|
['uv', 'run', 'python', 'scripts/audit_exception_handling.py', '--json'],
|
|
capture_output=True, text=True, cwd='.'
|
|
)
|
|
data = json.loads(r.stdout)
|
|
app = [f for f in data['files'] if 'app_controller' in f.get('filename', '')][0]
|
|
broad_sites = [f for f in app['findings'] if f.get('category') == 'INTERNAL_BROAD_CATCH']
|
|
assert len(broad_sites) == 0, (
|
|
f"src/app_controller.py still has {len(broad_sites)} INTERNAL_BROAD_CATCH sites at lines "
|
|
f"{[f.get('line') for f in broad_sites]}. All 32 must be migrated to specific exceptions."
|
|
)
|
|
|
|
|
|
def test_offload_entry_payload_preserves_unchanged_payload():
|
|
"""
|
|
Verifies the no-op path: entries without tool_call or tool_result kinds are returned
|
|
with the payload unchanged (no [REF:...] rewriting).
|
|
"""
|
|
with patch("src.app_controller.performance_monitor.PerformanceMonitor"):
|
|
ctrl = AppController()
|
|
entry = {"kind": "request", "payload": {"message": "hi"}, "ts": "12:00:00"}
|
|
out = ctrl._offload_entry_payload(entry)
|
|
assert out == entry
|
|
|
|
|
|
# --- Phase 6: Group 6.1 (signal handlers; Pattern 3 drain via os._exit) ---
|
|
|
|
def test_shutdown_io_pool_result_returns_ok_when_pool_shuts_down_cleanly():
|
|
"""
|
|
Pattern 3 drain: _shutdown_io_pool_result returns Result[None] with no errors
|
|
when the IO pool shuts down without raising.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl._io_pool = MagicMock()
|
|
ctrl._io_pool.shutdown = MagicMock()
|
|
result = ctrl._shutdown_io_pool_result()
|
|
assert isinstance(result, Result)
|
|
assert result.ok is True
|
|
assert result.errors == []
|
|
|
|
|
|
def test_shutdown_io_pool_result_returns_error_when_pool_raises():
|
|
"""
|
|
Pattern 3 drain: _shutdown_io_pool_result converts OSError/RuntimeError/ValueError
|
|
to ErrorInfo(original=e) in Result.errors.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl._io_pool = MagicMock()
|
|
ctrl._io_pool.shutdown = MagicMock(side_effect=RuntimeError("pool broken"))
|
|
result = ctrl._shutdown_io_pool_result()
|
|
assert isinstance(result, Result)
|
|
assert result.ok is False
|
|
assert len(result.errors) == 1
|
|
assert isinstance(result.errors[0], ErrorInfo)
|
|
assert "pool broken" in result.errors[0].message
|
|
assert result.errors[0].original is not None
|
|
|
|
|
|
def test_install_signal_handler_result_returns_ok_when_signal_installs():
|
|
"""
|
|
Pattern 3 drain: _install_signal_handler_result returns Result[None] on success
|
|
(no errors).
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
handler = lambda signum, frame: None
|
|
with patch("src.app_controller.signal.signal") as mock_signal:
|
|
result = ctrl._install_signal_handler_result(handler)
|
|
assert isinstance(result, Result)
|
|
assert result.ok is True
|
|
assert result.errors == []
|
|
assert mock_signal.called
|
|
|
|
|
|
def test_install_signal_handler_result_returns_error_when_signal_raises():
|
|
"""
|
|
Pattern 3 drain: _install_signal_handler_result converts ValueError/OSError
|
|
to ErrorInfo(original=e).
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
handler = lambda signum, frame: None
|
|
with patch("src.app_controller.signal.signal", side_effect=ValueError("not main thread")):
|
|
result = ctrl._install_signal_handler_result(handler)
|
|
assert isinstance(result, Result)
|
|
assert result.ok is False
|
|
assert len(result.errors) == 1
|
|
assert isinstance(result.errors[0], ErrorInfo)
|
|
assert "not main thread" in result.errors[0].message
|
|
|
|
|
|
def test_install_sigint_exit_handler_stores_error_when_signal_install_fails():
|
|
"""
|
|
Drains the Result to instance state: when the helper returns errors,
|
|
_install_sigint_exit_handler stores the first error on
|
|
ctrl._signal_handler_error for downstream consumers (e.g., sub-track 4 GUI).
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
with patch("src.app_controller.signal.signal", side_effect=ValueError("not main thread")):
|
|
_install_sigint_exit_handler(ctrl)
|
|
assert ctrl._signal_handler_error is not None
|
|
assert isinstance(ctrl._signal_handler_error, ErrorInfo)
|
|
assert "not main thread" in ctrl._signal_handler_error.message
|
|
assert ctrl._signal_handler_error.kind == ErrorKind.INTERNAL
|
|
|
|
|
|
def test_install_sigint_exit_handler_no_error_when_signal_install_succeeds():
|
|
"""
|
|
On success, _signal_handler_error stays as None.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
with patch("src.app_controller.signal.signal"):
|
|
_install_sigint_exit_handler(ctrl)
|
|
assert ctrl._signal_handler_error is None
|
|
|
|
|
|
# --- Phase 6: Group 6.2 (timeline event sinks; stderr + instance state carry) ---
|
|
|
|
def test_first_frame_timeline_returns_ok_in_normal_path():
|
|
"""
|
|
Event sink (drain: stderr + instance state): mark_first_frame_rendered
|
|
extracts timeline-write logic into a Result-returning helper.
|
|
On the happy path, no error is recorded.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl._warmup_done_ts = ctrl._init_start_ts + 0.5
|
|
ctrl.mark_first_frame_rendered(ts=ctrl._init_start_ts + 1.0)
|
|
# The first frame was logged; any error would be appended to the
|
|
# timeline-errors list. The list starts empty on a fresh controller.
|
|
assert all(op != "first_frame_timeline" for op, _ in ctrl._startup_timeline_errors)
|
|
|
|
|
|
def test_warmup_complete_timeline_returns_ok_in_normal_path():
|
|
"""
|
|
Event sink (drain: stderr + instance state): _on_warmup_complete_for_timeline
|
|
extracts timeline-write logic into a Result-returning helper.
|
|
On the happy path, no error is recorded.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl._first_frame_ts = None
|
|
ctrl._on_warmup_complete_for_timeline({})
|
|
assert all(op != "warmup_complete_timeline" for op, _ in ctrl._startup_timeline_errors)
|
|
|
|
|
|
def test_first_frame_timeline_records_error_on_stderr_failure():
|
|
"""
|
|
When the stderr write fails inside the helper, the timeline event sink
|
|
records the error in self._startup_timeline_errors for sub-track 4 GUI to drain.
|
|
The OSError from the helper propagates up; we catch it here so the test
|
|
only verifies the durable append.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
# Force the stderr write to fail by patching write on sys.stderr.
|
|
with patch("src.app_controller.sys.stderr") as mock_stderr:
|
|
mock_stderr.write = MagicMock(side_effect=OSError("stderr closed"))
|
|
try:
|
|
ctrl.mark_first_frame_rendered(ts=ctrl._init_start_ts + 0.1)
|
|
except OSError:
|
|
pass # the helper propagates the stderr failure; the append still happened
|
|
first_frame_errors = [(op, e) for op, e in ctrl._startup_timeline_errors if op == "first_frame_timeline"]
|
|
assert len(first_frame_errors) >= 1
|
|
assert isinstance(first_frame_errors[0][1], ErrorInfo)
|
|
assert "stderr closed" in first_frame_errors[0][1].message
|
|
|
|
|
|
def test_warmup_complete_timeline_records_error_on_stderr_failure():
|
|
"""
|
|
When the stderr write fails, the warmup-complete timeline sink records
|
|
the error in self._startup_timeline_errors.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl._first_frame_ts = None
|
|
with patch("src.app_controller.sys.stderr") as mock_stderr:
|
|
mock_stderr.write = MagicMock(side_effect=OSError("stderr closed"))
|
|
try:
|
|
ctrl._on_warmup_complete_for_timeline({})
|
|
except OSError:
|
|
pass
|
|
warmup_errors = [(op, e) for op, e in ctrl._startup_timeline_errors if op == "warmup_complete_timeline"]
|
|
assert len(warmup_errors) >= 1
|
|
assert isinstance(warmup_errors[0][1], ErrorInfo)
|
|
assert "stderr closed" in warmup_errors[0][1].message
|
|
|
|
|
|
# --- Phase 6: Group 6.3 (GUI state setters / property setters) ---
|
|
|
|
def test_update_inject_preview_result_returns_empty_when_no_path():
|
|
"""
|
|
_update_inject_preview_result returns Result(data="") when no file path is set.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl._inject_file_path = None
|
|
result = ctrl._update_inject_preview_result()
|
|
assert isinstance(result, Result)
|
|
assert result.ok is True
|
|
assert result.data == ""
|
|
|
|
|
|
def test_update_inject_preview_result_returns_error_on_read_failure():
|
|
"""
|
|
_update_inject_preview_result converts OSError to ErrorInfo(original=e) and
|
|
returns Result[data=""]. The legacy wrapper stores the error on
|
|
self._inject_preview_error and sets self._inject_preview to a user-facing
|
|
message.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl._inject_file_path = "/nonexistent/path/that/does/not/exist.py"
|
|
result = ctrl._update_inject_preview_result()
|
|
assert isinstance(result, Result)
|
|
# When file doesn't exist, returns empty data (no error)
|
|
assert result.ok is True
|
|
assert result.data == ""
|
|
|
|
|
|
def test_update_inject_preview_stores_error_on_read_failure():
|
|
"""
|
|
When file read fails (e.g. permission), the legacy wrapper stores the
|
|
error on self._inject_preview_error and shows a user-facing message.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl._inject_file_path = "/tmp/test_inject.py"
|
|
# Force the file-existence check to pass and the open call to fail.
|
|
with patch("src.app_controller.os.path.exists", return_value=True):
|
|
with patch("builtins.open", side_effect=PermissionError("denied")):
|
|
ctrl._update_inject_preview()
|
|
assert ctrl._inject_preview_error is not None
|
|
assert isinstance(ctrl._inject_preview_error, ErrorInfo)
|
|
assert "denied" in ctrl._inject_preview_error.message
|
|
assert ctrl._inject_preview.startswith("Error reading file:")
|
|
|
|
|
|
def test_set_mcp_config_json_result_returns_ok_on_valid_json():
|
|
"""
|
|
_set_mcp_config_json_result returns Result[None] on valid JSON.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
result = ctrl._set_mcp_config_json_result('{"servers": {}}')
|
|
assert isinstance(result, Result)
|
|
assert result.ok is True
|
|
assert result.errors == []
|
|
|
|
|
|
def test_set_mcp_config_json_result_returns_error_on_invalid_json():
|
|
"""
|
|
_set_mcp_config_json_result converts JSONDecodeError to ErrorInfo(original=e).
|
|
The legacy setter stores the error on self._mcp_config_parse_error.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
result = ctrl._set_mcp_config_json_result("not valid json")
|
|
assert isinstance(result, Result)
|
|
assert result.ok is False
|
|
assert len(result.errors) == 1
|
|
assert isinstance(result.errors[0], ErrorInfo)
|
|
|
|
|
|
def test_mcp_config_json_setter_stores_error_on_parse_failure():
|
|
"""
|
|
The property setter stores the first error on self._mcp_config_parse_error
|
|
when parsing fails.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl.mcp_config_json = "not valid json"
|
|
assert ctrl._mcp_config_parse_error is not None
|
|
assert isinstance(ctrl._mcp_config_parse_error, ErrorInfo)
|
|
|
|
|
|
def test_mcp_config_json_setter_no_error_on_valid_json():
|
|
"""
|
|
On valid JSON, _mcp_config_parse_error stays as None.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl.mcp_config_json = '{"servers": {}}'
|
|
assert ctrl._mcp_config_parse_error is None
|
|
|
|
|
|
def test_save_active_project_result_returns_ok_when_no_active_path():
|
|
"""
|
|
_save_active_project_result returns OK when no active_project_path is set.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl.active_project_path = None
|
|
result = ctrl._save_active_project_result()
|
|
assert isinstance(result, Result)
|
|
assert result.ok is True
|
|
assert result.errors == []
|
|
|
|
|
|
def test_save_active_project_stores_error_on_save_failure():
|
|
"""
|
|
When save_project raises, the legacy wrapper stores the error on
|
|
self._save_project_error and updates self.ai_status.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
ctrl.active_project_path = "/tmp/test_save.toml"
|
|
with patch("src.app_controller.project_manager.save_project", side_effect=PermissionError("denied")):
|
|
ctrl._save_active_project()
|
|
assert ctrl._save_project_error is not None
|
|
assert isinstance(ctrl._save_project_error, ErrorInfo)
|
|
assert "denied" in ctrl._save_project_error.message
|
|
assert "save error" in ctrl.ai_status
|
|
|
|
|
|
# --- Phase 6: Group 6.4 (SDK boundary in _fetch_models) ---
|
|
|
|
def test_list_models_for_provider_result_returns_ok_on_success():
|
|
"""
|
|
SDK boundary (Phase 6 Group 6.4): _list_models_for_provider_result wraps
|
|
ai_client.list_models(p) and returns Result[list] on success.
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
with patch("src.app_controller.ai_client.list_models", return_value=["model-a", "model-b"]):
|
|
result = ctrl._list_models_for_provider_result("gemini")
|
|
assert isinstance(result, Result)
|
|
assert result.ok is True
|
|
assert result.data == ["model-a", "model-b"]
|
|
|
|
|
|
def test_list_models_for_provider_result_returns_error_on_sdk_failure():
|
|
"""
|
|
SDK boundary: _list_models_for_provider_result converts SDK exceptions
|
|
to ErrorInfo(original=e) with NETWORK kind (the standard SDK boundary kind).
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
with patch("src.app_controller.ai_client.list_models", side_effect=RuntimeError("network unreachable")):
|
|
result = ctrl._list_models_for_provider_result("gemini")
|
|
assert isinstance(result, Result)
|
|
assert result.ok is False
|
|
assert len(result.errors) == 1
|
|
assert isinstance(result.errors[0], ErrorInfo)
|
|
assert "network unreachable" in result.errors[0].message
|
|
assert result.errors[0].kind == ErrorKind.NETWORK
|
|
assert result.errors[0].original is not None
|
|
|
|
|
|
def test_fetch_models_aggregates_per_provider_errors():
|
|
"""
|
|
The _fetch_models.do_fetch wrapper accumulates per-provider failures in
|
|
self._model_fetch_errors and returns a Result that carries the aggregated
|
|
errors. The legacy wrapper (do_fetch itself) is internal; the public API
|
|
is the side effect (self.all_available_models gets a [] entry per failed provider).
|
|
"""
|
|
from src.app_controller import AppController
|
|
ctrl = AppController()
|
|
# Make the SDK return an error for "gemini" and succeed for "anthropic"
|
|
def fake_list_models(p):
|
|
if p == "gemini":
|
|
raise RuntimeError("gemini api down")
|
|
return [f"{p}-model"]
|
|
with patch("src.app_controller.ai_client.list_models", side_effect=fake_list_models):
|
|
with patch("src.app_controller.ai_client.PROVIDERS", new=["gemini", "anthropic"]):
|
|
# do_fetch is the inner function; we need to access it. Easiest: call _fetch_models
|
|
# and inspect the resulting side effect on all_available_models.
|
|
ctrl._fetch_models("anthropic")
|
|
# Per-provider errors should be accumulated in self._model_fetch_errors
|
|
assert "gemini" in ctrl._model_fetch_errors
|
|
assert isinstance(ctrl._model_fetch_errors["gemini"], ErrorInfo)
|
|
assert "gemini api down" in ctrl._model_fetch_errors["gemini"].message
|
|
# The gemini entry should have an empty list (per-provider failure placeholder)
|
|
assert ctrl.all_available_models.get("gemini") == [] # NOTE: do_fetch may not have run yet if deferred
|