From 77a48b18bf193a0d44efbcb09a34fa8465e309a6 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Fri, 19 Jun 2026 23:45:29 -0400 Subject: [PATCH] TIER-2 READ conductor/code_styleguides/error_handling.md end-to-end before Phase 5: refactor(gui_2): migrate L1393 _open_patch_in_external_editor to Result[T] (Phase 5) Extract _open_patch_in_external_editor_result helper from the external editor launch try/except in App._open_patch_in_external_editor. Legacy wrapper drains errors to app._last_request_errors per FR-BC-4 event-handler pattern. [pre-audit] L1393 INTERNAL_BROAD_CATCH [post-audit] V count: 11 -> 10 (L1393 removed) --- src/gui_2.py | 91 ++++++++++++++++++++++++++++---------- tests/test_gui_2_result.py | 52 ++++++++++++++++++++++ 2 files changed, 120 insertions(+), 23 deletions(-) diff --git a/src/gui_2.py b/src/gui_2.py index f9be3c32..a1dc4314 100644 --- a/src/gui_2.py +++ b/src/gui_2.py @@ -1357,29 +1357,10 @@ class App: def _open_patch_in_external_editor(self) -> None: self._external_editor_clicked = True - try: - from src.external_editor import get_default_launcher, create_temp_modified_file - import os - if not self._pending_patch_files: - self._patch_error_message = "No files to edit" - return - launcher = get_default_launcher(self.config) - editor = launcher.config.get_default() - if not editor: - self._patch_error_message = "No external editor configured" - return - original_path = self._pending_patch_files[0] - if not os.path.exists(original_path): - self._patch_error_message = f"Original file not found: {original_path}" - return - temp_path = create_temp_modified_file(self._pending_patch_text) - result = launcher.launch_diff(None, original_path, temp_path) - if result is None: self._patch_error_message = "Failed to launch external editor" - else: - self._patch_error_message = None - self._vscode_diff_process = result - except Exception as e: - self._patch_error_message = str(e) + ext_result = _open_patch_in_external_editor_result(self) + if not ext_result.ok: + if not hasattr(self, '_last_request_errors'): self._last_request_errors = [] + self._last_request_errors.append(("_open_patch_in_external_editor", ext_result.errors[0])) def _reorder_ticket(self, src_idx: int, dst_idx: int) -> None: if src_idx == dst_idx: return @@ -7818,6 +7799,70 @@ def _apply_pending_patch_result(app: "App") -> Result[bool]: original=e, )]) + +def _open_patch_in_external_editor_result(app: "App") -> Result[bool]: + """Drain-aware variant of L1393 _open_patch_in_external_editor. + + Extracts the external editor launch try/except from + App._open_patch_in_external_editor into a Result-returning helper. On + success (launcher.launch_diff returns a process), sets + app._patch_error_message=None, app._vscode_diff_process=result and returns + Result(data=True). On failure (launch returns None or raises), sets + app._patch_error_message and returns Result(data=False, errors=[ErrorInfo]). + + The legacy wrapper drains errors to app._last_request_errors (per FR-BC-4 + event-handler drain pattern; data plane attribute). + + [C: src/gui_2.py:App._open_patch_in_external_editor (L1393 legacy wrapper)] + """ + from src.external_editor import get_default_launcher, create_temp_modified_file + import os + try: + if not app._pending_patch_files: + app._patch_error_message = "No files to edit" + return Result(data=False, errors=[ErrorInfo( + kind=ErrorKind.INTERNAL, + message="No files to edit", + source="gui_2._open_patch_in_external_editor_result", + )]) + launcher = get_default_launcher(app.config) + editor = launcher.config.get_default() + if not editor: + app._patch_error_message = "No external editor configured" + return Result(data=False, errors=[ErrorInfo( + kind=ErrorKind.INTERNAL, + message="No external editor configured", + source="gui_2._open_patch_in_external_editor_result", + )]) + original_path = app._pending_patch_files[0] + if not os.path.exists(original_path): + app._patch_error_message = f"Original file not found: {original_path}" + return Result(data=False, errors=[ErrorInfo( + kind=ErrorKind.INTERNAL, + message=f"Original file not found: {original_path}", + source="gui_2._open_patch_in_external_editor_result", + )]) + temp_path = create_temp_modified_file(app._pending_patch_text) + result = launcher.launch_diff(None, original_path, temp_path) + if result is None: + app._patch_error_message = "Failed to launch external editor" + return Result(data=False, errors=[ErrorInfo( + kind=ErrorKind.INTERNAL, + message="Failed to launch external editor", + source="gui_2._open_patch_in_external_editor_result", + )]) + app._patch_error_message = None + app._vscode_diff_process = result + return Result(data=True) + except Exception as e: + app._patch_error_message = str(e) + return Result(data=False, errors=[ErrorInfo( + kind=ErrorKind.INTERNAL, + message=f"Open patch in external editor failed: {e}", + source="gui_2._open_patch_in_external_editor_result", + original=e, + )]) + #endregion: Phase 5 Event Handler Result Helpers #endregion: MMA diff --git a/tests/test_gui_2_result.py b/tests/test_gui_2_result.py index cbf9b6de..faa5994e 100644 --- a/tests/test_gui_2_result.py +++ b/tests/test_gui_2_result.py @@ -923,3 +923,55 @@ def test_phase_5_l1367_apply_pending_patch_result_failure(): err = result.errors[0] assert err.source == "gui_2._apply_pending_patch_result" assert "patch blew up" in err.message + + +def test_phase_5_l1393_open_patch_in_external_editor_result_success(): + """ + L1393 _open_patch_in_external_editor_result returns Result.ok=True on success. + + The helper wraps the external editor launch try/except in + App._open_patch_in_external_editor. On success (launcher.launch_diff + returns a process), returns Result(data=True). + """ + from src import gui_2 + from unittest.mock import MagicMock, patch + app = MagicMock() + app._pending_patch_files = ["/proj/foo/src/foo.py"] + app._pending_patch_text = "--- a/foo.py\n+++ b/foo.py\n" + mock_editor = MagicMock(name="mock_editor") + mock_launcher = MagicMock(name="mock_launcher") + mock_launcher.config.get_default.return_value = mock_editor + mock_process = MagicMock(name="mock_process") + mock_launcher.launch_diff.return_value = mock_process + with patch("os.path.exists", return_value=True), \ + patch("src.external_editor.get_default_launcher", return_value=mock_launcher), \ + patch("src.external_editor.create_temp_modified_file", return_value="/tmp/patch_temp.py"): + result = gui_2._open_patch_in_external_editor_result(app) + assert result.ok, f"Expected ok=True on success, got errors: {result.errors}" + assert result.data is True + assert app._vscode_diff_process is mock_process + + +def test_phase_5_l1393_open_patch_in_external_editor_result_failure(): + """ + L1393 _open_patch_in_external_editor_result returns Result.ok=False with ErrorInfo on failure. + + When the external editor launch raises, the helper converts the exception + to ErrorInfo and returns Result(data=False, errors=[ErrorInfo]). + """ + from src import gui_2 + from unittest.mock import MagicMock, patch + app = MagicMock() + app._pending_patch_files = ["/proj/foo/src/foo.py"] + app._pending_patch_text = "--- a/foo.py\n+++ b/foo.py\n" + mock_launcher = MagicMock() + mock_launcher.config.get_default.return_value = MagicMock() + with patch("os.path.exists", return_value=True), \ + patch("src.external_editor.get_default_launcher", return_value=mock_launcher), \ + patch("src.external_editor.create_temp_modified_file", side_effect=RuntimeError("temp file creation blew up")): + result = gui_2._open_patch_in_external_editor_result(app) + assert not result.ok, f"Expected ok=False on failure, got data: {result.data}" + assert result.errors, "Expected at least one error on failure" + err = result.errors[0] + assert err.source == "gui_2._open_patch_in_external_editor_result" + assert "temp file creation blew up" in err.message