refactor(app_controller): migrate 3 GUI state-setter sites to Result (Phase 6 Group 6.3)
Replaces logging.debug bodies in: - _update_inject_preview (L1542): Result[str] variant; legacy wrapper stores error on self._inject_preview_error - mcp_config_json setter (L1685): sibling _set_mcp_config_json_result helper (property setters can't return values); setter stores error on self._mcp_config_parse_error - _save_active_project (L3124): Result[None] variant; legacy wrapper stores error on self._save_project_error and updates self.ai_status Each error-carrying state attribute is the durable data plane for sub-track 4 GUI to display; stderr write is the visible-but-incomplete drain (full drain = GUI modal in sub-track 4). Audit: INTERNAL_SILENT_SWALLOW for src/app_controller.py: 26 -> 23.
This commit is contained in:
+77
-15
@@ -837,6 +837,14 @@ class AppController:
|
||||
# sub-track 4 lands GUI-side error display); the instance list is
|
||||
# the durable data plane for sub-track 4 to drain.
|
||||
self._startup_timeline_errors: List[Tuple[str, ErrorInfo]] = [] # (op_name, error)
|
||||
# --- GUI state-setter error state (Phase 6: Group 6.3) ---
|
||||
# Each carries the first ErrorInfo when the corresponding setter/operation
|
||||
# fails. The legacy wrapper stores the error here for sub-track 4 GUI to
|
||||
# display; stderr write IS the visible-but-incomplete drain (full drain =
|
||||
# GUI modal in sub-track 4).
|
||||
self._inject_preview_error: Optional[ErrorInfo] = None
|
||||
self._mcp_config_parse_error: Optional[ErrorInfo] = None
|
||||
self._save_project_error: Optional[ErrorInfo] = None
|
||||
# --- Shared background pool + proactive warmup (startup_speedup_20260606) ---
|
||||
self._io_pool = make_io_pool()
|
||||
_install_sigint_exit_handler(self)
|
||||
@@ -1518,15 +1526,27 @@ class AppController:
|
||||
Updates the preview content based on the selected file and injection mode.
|
||||
[C: src/gui_2.py:App._render_text_viewer_window, tests/test_skeleton_injection.py:test_update_inject_preview_full, tests/test_skeleton_injection.py:test_update_inject_preview_skeleton, tests/test_skeleton_injection.py:test_update_inject_preview_truncation]
|
||||
"""
|
||||
result = self._update_inject_preview_result()
|
||||
if result.ok:
|
||||
self._inject_preview = result.data
|
||||
self._inject_preview_error = None
|
||||
else:
|
||||
err = result.errors[0]
|
||||
self._inject_preview_error = err
|
||||
self._inject_preview = f"Error reading file: {err.message}"
|
||||
|
||||
def _update_inject_preview_result(self) -> "Result[str]":
|
||||
"""Read the file for the inject preview. Returns Result[str] (Phase 6 Group 6.3).
|
||||
On failure: OSError/IOError/UnicodeDecodeError -> ErrorInfo(original=e).
|
||||
Caller (`_update_inject_preview`) stores the error on
|
||||
`self._inject_preview_error` for sub-track 4 GUI."""
|
||||
if not self._inject_file_path:
|
||||
self._inject_preview = ""
|
||||
return
|
||||
return Result(data="")
|
||||
target_path = self._inject_file_path
|
||||
if not os.path.isabs(target_path):
|
||||
target_path = os.path.join(self.active_project_root, target_path)
|
||||
if not os.path.exists(target_path):
|
||||
self._inject_preview = ""
|
||||
return
|
||||
return Result(data="")
|
||||
try:
|
||||
with open(target_path, "r", encoding="utf-8") as f:
|
||||
content = f.read()
|
||||
@@ -1538,10 +1558,14 @@ class AppController:
|
||||
lines = preview.splitlines()
|
||||
if len(lines) > 500:
|
||||
preview = "\n".join(lines[:500]) + "\n... (truncated)"
|
||||
self._inject_preview = preview
|
||||
return Result(data=preview)
|
||||
except (OSError, IOError, UnicodeDecodeError) as e:
|
||||
logging.getLogger(__name__).debug("inject preview file read failed: %s", e, extra={"source": "app_controller._update_inject_preview"})
|
||||
self._inject_preview = f"Error reading file: {e}"
|
||||
return Result(data="", errors=[ErrorInfo(
|
||||
kind=ErrorKind.INTERNAL,
|
||||
message=str(e),
|
||||
source="app_controller._update_inject_preview_result",
|
||||
original=e,
|
||||
)])
|
||||
|
||||
@property
|
||||
def ai_status(self) -> str:
|
||||
@@ -1684,11 +1708,30 @@ class AppController:
|
||||
return json.dumps(self.mcp_config.to_dict()) if self.mcp_config else "{}"
|
||||
@mcp_config_json.setter
|
||||
def mcp_config_json(self, value: str) -> None:
|
||||
result = self._set_mcp_config_json_result(value)
|
||||
if not result.ok:
|
||||
err = result.errors[0]
|
||||
self._mcp_config_parse_error = err
|
||||
sys.stderr.write(f"mcp_config parse failed: {err.ui_message()}\n")
|
||||
sys.stderr.flush()
|
||||
|
||||
def _set_mcp_config_json_result(self, value: str) -> "Result[None]":
|
||||
"""Parse and assign mcp_config from a JSON string. Returns Result[None] (Phase 6 Group 6.3).
|
||||
On failure: json.JSONDecodeError/ValueError/TypeError/KeyError/AttributeError -> ErrorInfo(original=e).
|
||||
Property setters cannot return values; this sibling helper carries the
|
||||
structured result. Caller (`mcp_config_json` setter) stores the error
|
||||
on `self._mcp_config_parse_error` for sub-track 4 GUI."""
|
||||
try:
|
||||
data = json.loads(value)
|
||||
self.mcp_config = models.MCPConfiguration.from_dict(data)
|
||||
return OK
|
||||
except (json.JSONDecodeError, ValueError, TypeError, KeyError, AttributeError) as e:
|
||||
logging.getLogger(__name__).debug("mcp config parse failed: %s", e, extra={"source": "app_controller.mcp_config_json"})
|
||||
return Result(data=None, errors=[ErrorInfo(
|
||||
kind=ErrorKind.INVALID_INPUT,
|
||||
message=str(e),
|
||||
source="app_controller._set_mcp_config_json_result",
|
||||
original=e,
|
||||
)])
|
||||
|
||||
@property
|
||||
def ui_file_paths(self) -> list[str]:
|
||||
@@ -3125,13 +3168,32 @@ class AppController:
|
||||
"""
|
||||
[C: src/gui_2.py:App.delete_context_preset, src/gui_2.py:App.save_context_preset]
|
||||
"""
|
||||
if self.active_project_path:
|
||||
try:
|
||||
cleaned = project_manager.clean_nones(self.project)
|
||||
project_manager.save_project(cleaned, self.active_project_path)
|
||||
except (OSError, IOError, ValueError, TypeError, KeyError, AttributeError) as e:
|
||||
logging.getLogger(__name__).debug("save_project failed: %s", e, extra={"source": "app_controller._save_active_project"})
|
||||
self.ai_status = f"save error: {e}"
|
||||
result = self._save_active_project_result()
|
||||
if not result.ok:
|
||||
err = result.errors[0]
|
||||
self._save_project_error = err
|
||||
self.ai_status = f"save error: {err.message}"
|
||||
sys.stderr.write(f"save_project failed: {err.ui_message()}\n")
|
||||
sys.stderr.flush()
|
||||
|
||||
def _save_active_project_result(self) -> "Result[None]":
|
||||
"""Save the active project to its current path. Returns Result[None] (Phase 6 Group 6.3).
|
||||
On failure: OSError/IOError/ValueError/TypeError/KeyError/AttributeError -> ErrorInfo(original=e).
|
||||
Caller (`_save_active_project`) stores the error on
|
||||
`self._save_project_error` for sub-track 4 GUI and updates self.ai_status."""
|
||||
if not self.active_project_path:
|
||||
return OK
|
||||
try:
|
||||
cleaned = project_manager.clean_nones(self.project)
|
||||
project_manager.save_project(cleaned, self.active_project_path)
|
||||
return OK
|
||||
except (OSError, IOError, ValueError, TypeError, KeyError, AttributeError) as e:
|
||||
return Result(data=None, errors=[ErrorInfo(
|
||||
kind=ErrorKind.INTERNAL,
|
||||
message=str(e),
|
||||
source="app_controller._save_active_project_result",
|
||||
original=e,
|
||||
)])
|
||||
|
||||
#endregion: Project
|
||||
|
||||
|
||||
@@ -278,3 +278,130 @@ def test_warmup_complete_timeline_records_error_on_stderr_failure():
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user