refactor(src): eliminate 11 T | None legacy wrappers in favor of _result API

TIER-3 READ AGENTS.md + conductor/workflow.md + conductor/code_styleguides/error_handling.md + the 4 source files + 3 test files before this commit.

The code_path_audit_phase_2_20260624 track (Tier 2) shipped 11 audit
fixes (4 NG1 + 7 NG2) but used a heuristic bypass for 4 of the NG2
wrappers: legacy T | None functions that exist only to maintain test
patcher compatibility. Per the review at
docs/reports/REVIEW_TIER2_code_path_audit_phase_2_20260624.md Finding 8,
this track eliminates the legacy wrappers properly.

11 wrappers eliminated (8 main + 3 _legacy_compat inner):
- src/ai_client.py: get_current_tier (1 src + 1 test consumer)
- src/ai_client.py: _gemini_tool_declaration + _legacy_compat (2 test consumers)
- src/ai_client.py: run_tier4_patch_callback + _legacy_compat (was 0 direct callers
  but had 2 callback references in app_controller/multi_agent_conductor;
  callback contract migrated to Callable[[str, str], Result[str]] instead of
  preserving an Optional[str] adapter)
- src/mcp_client.py: _get_symbol_node + _legacy_compat (8 in-file consumers)
- src/mcp_client.py: find_in_scope (nested inside _get_symbol_node_result;
  private impl detail, audit doesn't catch T | None, left as-is)
- src/external_editor.py: launch_diff (1 src + 3 test + 1 live_gui test consumer)
- src/external_editor.py: launch_editor (no consumers; deleted)
- src/session_logger.py: log_tool_output (2 src + 3 test consumers)
- src/project_manager.py: parse_ts (no consumers; deleted)

For each consumer: replace legacy_fn(args) with legacy_fn_result(args).data.
For T | None checks: replace if x is None: with if not result.ok: or
if not result.ok or not isinstance(result.data, ...) (depending on pattern).

For run_tier4_patch_callback specifically: the wrapper was a callback adapter
(not a backward-compat shim) and had 2 callback references as consumers.
Rather than keep the adapter (which would re-introduce the Optional[str]
return that the strict audit catches), the patch_callback contract was migrated
from Callable[[str, str], Optional[str]] to Callable[[str, str], Result[str]]
in shell_runner.py + app_controller.py + 9 _send_<vendor>_result signatures
in ai_client.py. This propagates the Result[str] through the callback and
lets shell_runner unwrap with if r.ok and r.data instead of if patch_text.

Verification:
- audit_optional_in_3_files --strict: 0 return-type Optional[T] (down from 1)
- audit_exception_handling --strict: 0 violations (unchanged)
- audit_legacy_wrappers: 0 legacy wrappers (unchanged)
- 15 affected test files: 168 tests pass
- 8 mcp_client/structural/baseline test files: 55 tests pass
- 3 session/gui test files: 7 tests pass
- 0 return-type Optional[T] in src/ai_client.py (was 1: run_tier4_patch_callback)

tests/test_ai_client_concurrency.py

@@ -7,7 +7,7 @@ def test_ai_client_tier_isolation():
  def intercepted_append(direction, kind, payload):
   captured_logs.append({
    'thread_name': threading.current_thread().name,
-   'source_tier': ai_client.get_current_tier()
+   'source_tier': ai_client.get_current_tier_result().data
   })
   original_append(direction, kind, payload)
  ai_client._append_comms = intercepted_append

tests/test_ai_loop_regressions_20260614.py

@@ -64,7 +64,7 @@ def test_fr1_error_becomes_discussion_entry(mock_app: App, monkeypatch: pytest.M
  monkeypatch.setattr(ai_client, "set_agent_tools", lambda *a, **kw: None)
  monkeypatch.setattr(ai_client, "set_current_tier", lambda *a, **kw: None)
  monkeypatch.setattr(ai_client, "get_combined_system_prompt", lambda *a, **kw: "")
- monkeypatch.setattr(ai_client, "get_current_tier", lambda *a, **kw: None)
+ monkeypatch.setattr(ai_client, "get_current_tier_result", lambda *a, **kw: Result(data=None))
  monkeypatch.setattr("src.app_controller.AppController._update_gcli_adapter", lambda *a, **kw: None)
  _drain_queue(app)
  app.controller._handle_request_event(_make_event())
@@ -93,7 +93,7 @@ def test_fr1_success_still_works(mock_app: App, monkeypatch: pytest.MonkeyPatch)
  monkeypatch.setattr(ai_client, "set_agent_tools", lambda *a, **kw: None)
  monkeypatch.setattr(ai_client, "set_current_tier", lambda *a, **kw: None)
  monkeypatch.setattr(ai_client, "get_combined_system_prompt", lambda *a, **kw: "")
- monkeypatch.setattr(ai_client, "get_current_tier", lambda *a, **kw: None)
+ monkeypatch.setattr(ai_client, "get_current_tier_result", lambda *a, **kw: Result(data=None))
  monkeypatch.setattr("src.app_controller.AppController._update_gcli_adapter", lambda *a, **kw: None)
  _drain_queue(app)
  app.controller._handle_request_event(_make_event())
@@ -121,7 +121,7 @@ def test_fr1_ai_status_updated(mock_app: App, monkeypatch: pytest.MonkeyPatch) -
  monkeypatch.setattr(ai_client, "set_agent_tools", lambda *a, **kw: None)
  monkeypatch.setattr(ai_client, "set_current_tier", lambda *a, **kw: None)
  monkeypatch.setattr(ai_client, "get_combined_system_prompt", lambda *a, **kw: "")
- monkeypatch.setattr(ai_client, "get_current_tier", lambda *a, **kw: None)
+ monkeypatch.setattr(ai_client, "get_current_tier_result", lambda *a, **kw: Result(data=None))
  monkeypatch.setattr("src.app_controller.AppController._update_gcli_adapter", lambda *a, **kw: None)
  _drain_queue(app)
  app.controller._handle_request_event(_make_event())

tests/test_app_controller_offloading.py

@@ -96,7 +96,7 @@ def test_on_tool_log_offloading(app_controller, tmp_session_dir):
  script = "Get-Process"
  result = "Process list..."
 
- with patch("src.ai_client.get_current_tier", return_value="Tier 3"):
+ with patch("src.ai_client.get_current_tier_result", return_value=Result(data="Tier 3")):
   app_controller._on_tool_log(script, result)
 
  # Verify files were created in session directory

tests/test_external_editor.py

@@ -101,21 +101,24 @@ class TestExternalEditorLauncher:
   assert cmd == ["C:\\path\\to\\code.exe", "--diff", "orig.txt", "mod.txt"]
 
  def test_launch_diff_missing_editor(self, launcher):
-  result = launcher.launch_diff("nonexistent", "orig.txt", "mod.txt")
-  assert result is None
+  result = launcher.launch_diff_result("nonexistent", "orig.txt", "mod.txt")
+  assert not result.ok
+  assert result.data is None
 
  @patch("subprocess.Popen")
  def test_launch_diff_success(self, mock_popen, launcher):
   mock_popen.return_value = MagicMock()
-  result = launcher.launch_diff("vscode", "orig.txt", "mod.txt")
-  assert result is not None
+  result = launcher.launch_diff_result("vscode", "orig.txt", "mod.txt")
+  assert result.ok
+  assert result.data is not None
   mock_popen.assert_called_once()
 
  @patch("subprocess.Popen")
  def test_launch_diff_file_not_found(self, mock_popen, launcher):
   mock_popen.side_effect = FileNotFoundError()
-  result = launcher.launch_diff("vscode", "orig.txt", "mod.txt")
-  assert result is None
+  result = launcher.launch_diff_result("vscode", "orig.txt", "mod.txt")
+  assert not result.ok
+  assert result.data is None
 
 
 class TestHelperFunctions:

tests/test_gui_2_result.py

@@ -1033,7 +1033,7 @@ 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
+ App._open_patch_in_external_editor. On success (launcher.launch_diff_result
  returns a process), returns Result(data=True).
  """
  from src import gui_2
@@ -1045,7 +1045,7 @@ def test_phase_5_l1393_open_patch_in_external_editor_result_success():
  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
+ mock_launcher.launch_diff_result.return_value  = MagicMock(ok=True, data=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"):

tests/test_headless_verification.py

@@ -67,7 +67,7 @@ async def test_headless_verification_error_and_qa_interceptor(vlogger) -> None:
  patch("src.ai_client.confirm_and_run_callback") as mock_run, \
  patch("src.ai_client.run_tier4_analysis", return_value="FIX: Check if path exists.") as mock_qa, \
  patch("src.ai_client._ensure_gemini_client") as mock_ensure, \
- patch("src.ai_client._gemini_tool_declaration", return_value=None), \
+ patch("src.ai_client._gemini_tool_declaration_result", return_value=Result(data=None)), \
  patch("src.multi_agent_conductor.confirm_spawn", return_value=(True, "mock_prompt", "mock_ctx")):
  # Ensure _gemini_client is restored by the mock ensure function
 

tests/test_mma_agent_focus_phase1.py

@@ -12,9 +12,9 @@ def reset_tier():
  ai_client.set_current_tier(None)
 
 def test_get_current_tier_exists() -> None:
- """ai_client must expose a get_current_tier function."""
- assert hasattr(ai_client, "get_current_tier")
- assert callable(ai_client.get_current_tier)
+ """ai_client must expose a get_current_tier_result function."""
+ assert hasattr(ai_client, "get_current_tier_result")
+ assert callable(ai_client.get_current_tier_result)
 
 def test_append_comms_has_source_tier_key() -> None:
  """Dict entries in comms log must have a 'source_tier' key."""

tests/test_session_logger_optimization.py

@@ -78,20 +78,23 @@ def test_log_tool_output_saves_in_session_outputs(temp_session_setup: tuple[Path
  output_content = "This is some tool output content."
  
  # Call log_tool_output
- output_path_str = session_logger.log_tool_output(output_content)
- assert output_path_str is not None
- 
- output_path = Path(output_path_str)
+ output_result = session_logger.log_tool_output_result(output_content)
+ assert output_result.ok, f"log_tool_output failed: {output_result.errors}"
+ assert output_result.data is not None
+
+ output_path = Path(output_result.data)
  assert output_path.parent == outputs_subdir
  assert output_path.name == "output_0001.txt"
  assert output_path.read_text(encoding="utf-8") == output_content
- 
+
  # Verify second call increments sequence
- output_path_str_2 = session_logger.log_tool_output("More content")
- assert output_path_str_2 is not None
- assert Path(output_path_str_2).name == "output_0002.txt"
+ output_result_2 = session_logger.log_tool_output_result("More content")
+ assert output_result_2.ok, f"log_tool_output failed: {output_result_2.errors}"
+ assert output_result_2.data is not None
+ assert Path(output_result_2.data).name == "output_0002.txt"
 
 def test_log_tool_output_returns_none_if_no_session(temp_session_setup: tuple[Path, Path]) -> None:
  # We don't call open_session here
- output_path_str = session_logger.log_tool_output("Should not save")
- assert output_path_str is None
+ output_result = session_logger.log_tool_output_result("Should not save")
+ assert not output_result.ok
+ assert output_result.data is None

tests/test_tool_access_exclusion.py

@@ -14,7 +14,7 @@ def test_set_agent_tools_clears_caches():
 def test_gemini_tool_declaration_excludes_disabled():
  # Test explicit disable
  ai_client.set_agent_tools({"read_file": False})
- tool = ai_client._gemini_tool_declaration()
+ tool = ai_client._gemini_tool_declaration_result().data
  names = [f.name for f in tool.function_declarations] if tool else []
  assert "read_file" not in names
  
@@ -23,7 +23,7 @@ def test_gemini_tool_declaration_excludes_disabled():
  all_tools[ai_client.TOOL_NAME] = False
  all_tools["read_file"] = True
  ai_client.set_agent_tools(all_tools)
- tool = ai_client._gemini_tool_declaration()
+ tool = ai_client._gemini_tool_declaration_result().data
  names = [f.name for f in tool.function_declarations] if tool else []
  assert "read_file" in names
  assert "write_file" not in names

tests/tier2/phase10_invariant_test.py

@@ -48,22 +48,22 @@ def test_phase10_all_helpers_exist():
 
 
 def test_phase10_legacy_functions_preserved():
-    """Legacy functions preserved EXCEPT those OBLITERATED by cruft-removal Phase 4."""
+    """Legacy functions preserved EXCEPT those OBLITERATED by cruft-removal Phase 4 or code_path_audit_phase_2 cleanup."""
     import src.ai_client
     legacy = [
         "_send_gemini",
         "_send_gemini_cli",
         "run_tier4_analysis",
-        "run_tier4_patch_callback",
         "run_tier4_patch_generation",
     ]
     # _list_gemini_models wrapper was OBLITERATED by cruft-removal Phase 4
-    obliterated = ["_list_gemini_models"]
+    # run_tier4_patch_callback wrapper was OBLITERATED by code_path_audit_phase_2 cleanup
+    obliterated = ["_list_gemini_models", "run_tier4_patch_callback"]
     for name in legacy:
         assert hasattr(src.ai_client, name), f"{name} legacy function missing"
         assert callable(getattr(src.ai_client, name)), f"{name} not callable"
     for name in obliterated:
         assert not hasattr(src.ai_client, name), (
-            f"{name} wrapper must be OBLITERATED (cruft-removal Phase 4); "
+            f"{name} wrapper must be OBLITERATED; "
             f"callers must use {name}_result directly"
         )

tests/tier2/phase10_sites789_test.py

@@ -45,10 +45,23 @@ def test_phase10_sites789_all_helpers_return_result():
 
 
 def test_phase10_sites789_legacy_unchanged():
-    """Legacy functions must still exist + be callable."""
+    """Legacy functions preserved EXCEPT those OBLITERATED by code_path_audit_phase_2 cleanup.
+
+    run_tier4_patch_callback was a T|None wrapper (heuristic bypass per review Finding 8)
+    whose only consumers were callback references in app_controller.py and
+    multi_agent_conductor.py. After this cleanup track:
+    - The callback contract migrated to Callable[[str, str], Result[str]]
+    - The 2 callers now pass _run_tier4_patch_callback_result directly
+    - run_tier4_patch_callback wrapper is gone
+    """
     import src.ai_client
-    for name in ("run_tier4_analysis",
-                 "run_tier4_patch_callback",
-                 "run_tier4_patch_generation"):
+    legacy = ["run_tier4_analysis", "run_tier4_patch_generation"]
+    obliterated = ["run_tier4_patch_callback"]
+    for name in legacy:
         assert hasattr(src.ai_client, name), f"{name} missing"
-        assert callable(getattr(src.ai_client, name)), f"{name} not callable"
+        assert callable(getattr(src.ai_client, name)), f"{name} not callable"
+    for name in obliterated:
+        assert not hasattr(src.ai_client, name), (
+            f"{name} wrapper must be OBLITERATED (code_path_audit_phase_2 cleanup); "
+            f"callers must use {name}_result directly"
+        )