diff --git a/agents/IMPLEMENTATION.md b/agents/IMPLEMENTATION.md index 2f75b3e..fd8ae9b 100644 --- a/agents/IMPLEMENTATION.md +++ b/agents/IMPLEMENTATION.md @@ -30,6 +30,8 @@ ## Recently Completed (June 2026) +- **Graceful transient connection-error handling (Jul 13)**: A lost API connection (laptop asleep / no internet) during generation used to be swallowed — a single blip could mark a phase `completed` that never actually ran. Now: (1) `model_routing.classify_error` recognizes `AgentErrorType.CONNECTION_ERROR` via a narrow pattern list (`unable to connect to api`, `socket connection was closed`, `connection refused/reset`, `server disconnected`, …), checked before the 4xx/5xx guard. (2) `execute_all_phases` raises `WorkspaceAbortedError` on a `CONNECTION_ERROR` phase **before** the checkpoint write, so a lost phase is never falsely recorded (a later `retry_generation` re-runs it). **Retry itself is left to the Claude SDK default** (no custom wait-loop / config knobs). Surfacing an all-workspace failure up through the orchestrator (so a total connection loss ends in FAILED rather than "complete") and failure-notification were both intentionally descoped from this PR. Tests: `test_classify_error.py::TestConnectionError`, `test_execute_all_phases_connection_error.py` (backend 2011 passed / 36 skipped). Pre-existing, untouched mypy errors remain in `app/state/workspace_models.py:73`, `app/services/openrouter_api.py:21`, `app/services/model_catalog.py:80`. + - **Quickstart P10Y repository lookup pagination (Jul 8)**: Fixed Quickstart initialization for P10Y organizations with more than one repository page. `create_generation_session_repos.py` now searches generated repos with the qualified GitHub owner/prefix (for example `org/specflow-workspace`) and uses a shared paginated P10Y repository listing helper for ID resolution, re-fetch connection discovery, status checks, and status polling. Tests: `uv run pytest test/scripts/test_create_generation_session_repos.py -q` from `backend` (25 passed). - **TUI workspace notification dedupe (Jul 1)**: Fixed workspace drill-in causing repeated desktop "Workspace phase progressed" notifications every poll during KB init. `MilestoneTracker` now emits workspace progress only when `last_completed_phase` increases, so label-only differences between session-list and `/status` payloads stay silent. `WorkspaceMessagesScreen` feeds its `/status` poll into the shared per-run tracker, and the app-wide active-session watcher excludes the workspace screen's generation to avoid mixed payload sources. Tests: `test_tui_poller.py::TestMilestoneTracker::test_kb_init_phase_label_change_without_progress_is_silent`, `test_tui_app.py::TestWorkspaceDrillIn::test_workspace_screen_excludes_app_level_session_watcher`; broader TUI check: `uv run pytest tests/test_tui_app.py tests/test_tui_poller.py -v` from `mcp_server` (78 passed). diff --git a/backend/app/schemas/agent.py b/backend/app/schemas/agent.py index 7d08310..89ec08a 100644 --- a/backend/app/schemas/agent.py +++ b/backend/app/schemas/agent.py @@ -6,6 +6,7 @@ class AgentErrorType(str, Enum): TOOL_CALL_FAILURE = "tool_call_failure" MODEL_ROUTING_FAILURE = "model_routing_failure" + CONNECTION_ERROR = "connection_error" @dataclass diff --git a/backend/app/services/claude_code.py b/backend/app/services/claude_code.py index 845eccb..3fc7e9d 100644 --- a/backend/app/services/claude_code.py +++ b/backend/app/services/claude_code.py @@ -1270,6 +1270,12 @@ async def execute_all_phases( f"(model={workspace.model}). Aborting workspace — other workspaces continue. " f"Error: {phase_result.result}" ) + if error_type == AgentErrorType.CONNECTION_ERROR: + raise WorkspaceAbortedError( + f"[{workspace_name}] Phase {phase_num} lost connection to the API " + f"(model={workspace.model}). Aborting workspace — other workspaces continue. " + f"Error: {phase_result.result}" + ) consecutive_errors += 1 logger.error( f"[{workspace_name}] Phase {phase_num} reported is_error=True " diff --git a/backend/app/services/model_routing.py b/backend/app/services/model_routing.py index 04899e1..0d68c9f 100644 --- a/backend/app/services/model_routing.py +++ b/backend/app/services/model_routing.py @@ -38,11 +38,26 @@ "empty response", ] +# Transient network/connection failures (laptop asleep, lost internet) — retryable, no HTTP status. +_CONNECTION_ERROR_PATTERNS = [ + "unable to connect to api", + "connectionrefused", + "connection refused", + "socket connection was closed", + "connection error", + "connection reset", + "server disconnected", + "network is unreachable", +] + def classify_error(error_msg: str, api_error_status: Optional[int] = None) -> Optional[AgentErrorType]: """Classify an agent error for targeted logging and telemetry.""" msg_lower = error_msg.lower() if any(p.lower() in msg_lower for p in _TOOL_CALL_ERROR_PATTERNS): return AgentErrorType.TOOL_CALL_FAILURE + # Connection failure checked before the 4xx/5xx guard (there is no HTTP status) and before routing. + if any(p in msg_lower for p in _CONNECTION_ERROR_PATTERNS): + return AgentErrorType.CONNECTION_ERROR # 4xx/5xx = definitive HTTP error, not a routing failure. Routing failures return HTTP 200 # with a malformed body, so api_error_status=200 (or None) must NOT be excluded here. if api_error_status is not None and api_error_status >= 400: diff --git a/backend/test/test_classify_error.py b/backend/test/test_classify_error.py index 0b5788a..0593029 100644 --- a/backend/test/test_classify_error.py +++ b/backend/test/test_classify_error.py @@ -48,7 +48,9 @@ def test_empty_response_case_insensitive(self): assert classify_error("EMPTY RESPONSE received") == AgentErrorType.MODEL_ROUTING_FAILURE def test_does_not_match_unrelated_errors(self): - assert classify_error("connection refused") is None + # "rate limit" and generic "timeout" are intentionally NOT connection errors (out of + # scope for the transient-connection retry). "connection refused" now classifies as + # CONNECTION_ERROR — see TestConnectionError. assert classify_error("rate limit exceeded") is None assert classify_error("timeout after 30s") is None @@ -58,6 +60,40 @@ def test_tool_call_takes_priority_over_routing(self): assert classify_error(msg) == AgentErrorType.TOOL_CALL_FAILURE +class TestConnectionError: + """Transient network/connection failures — retryable, HTTP-status-independent.""" + + def test_unable_to_connect_to_api(self): + # The exact string the SDK emits when the laptop has no internet. + assert classify_error("API Error: Unable to connect to API (ConnectionRefused)") == AgentErrorType.CONNECTION_ERROR + + def test_socket_connection_closed(self): + assert classify_error("API Error: The socket connection was closed unexpectedly.") == AgentErrorType.CONNECTION_ERROR + + def test_connection_refused(self): + assert classify_error("connection refused") == AgentErrorType.CONNECTION_ERROR + + def test_connection_reset(self): + assert classify_error("Connection reset by peer") == AgentErrorType.CONNECTION_ERROR + + def test_server_disconnected(self): + assert classify_error("Server disconnected without sending a response.") == AgentErrorType.CONNECTION_ERROR + + def test_case_insensitive(self): + assert classify_error("UNABLE TO CONNECT TO API") == AgentErrorType.CONNECTION_ERROR + + def test_connection_wins_over_spurious_status_in_message(self): + # A stray number must not demote a real connection error to unclassified. + msg = "Unable to connect to API (ConnectionRefused) after 500ms" + inferred = extract_http_status_from_message(msg) + assert inferred == 500 # extractor would pick this up... + assert classify_error(msg, api_error_status=inferred) == AgentErrorType.CONNECTION_ERROR # ...but connection still wins + + def test_tool_call_takes_priority_over_connection(self): + # tool-call incompatibility must abort immediately, never be retried as transient. + assert classify_error("tool_use failed: connection error") == AgentErrorType.TOOL_CALL_FAILURE + + class TestApiErrorStatusExclusion: def test_known_http_error_not_classified_as_routing_failure(self): msg = "API Error: upstream overloaded" diff --git a/backend/test/test_execute_all_phases_connection_error.py b/backend/test/test_execute_all_phases_connection_error.py new file mode 100644 index 0000000..3916e52 --- /dev/null +++ b/backend/test/test_execute_all_phases_connection_error.py @@ -0,0 +1,111 @@ +""" +execute_all_phases: a lost API connection (surfaced after the SDK's own retries are +exhausted) must abort the workspace BEFORE the checkpoint write, so the phase is never +falsely recorded as completed — a later retry_generation must re-run it, not skip it. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, Mock, patch + +import pytest + +from app.schemas.agent import AgentResult +from app.schemas.planning import PhaseInfo, PlanningResult +from app.schemas.specification import GenerateAppRequest +from app.services.claude_code import WorkspaceAbortedError, execute_all_phases + + +def _mock_workspace(tmp_path: Path) -> Mock: + ws = Mock() + ws.workspace_path = tmp_path + ws.get_isolated_root = Mock(return_value=str(tmp_path)) + ws.model = "m" + ws.provider = "openrouter" + return ws + + +def _mock_manager() -> Mock: + mgr = Mock() + mgr.settings = Mock() + mgr.settings.ROSETTA_MCP_ENABLED = False + mgr.commit_and_push_outstanding = AsyncMock(return_value=True) + return mgr + + +def _planning_one_phase() -> PlanningResult: + return PlanningResult( + phase_count=1, + phases=[PhaseInfo(number=1, name="Backend", description="API", estimated_commits=1)], + plan_file_path="specflow/plan.md", + ) + + +@pytest.mark.asyncio +async def test_connection_error_aborts_without_checkpoint(tmp_path: Path) -> None: + """A connection-lost phase raises WorkspaceAbortedError and never writes the checkpoint.""" + planning = _planning_one_phase() + mock_ws = _mock_workspace(tmp_path) + mock_mgr = _mock_manager() + request = GenerateAppRequest(spec_path="specs", outputs_dir="specflow", generation_id="e1") + + svc = Mock() + svc.update_workspace_phase = AsyncMock() + svc.update_deployment_workspace_phase = AsyncMock() + + async def fake_phase_fn(**kwargs): + return AgentResult( + result="API Error: Unable to connect to API (ConnectionRefused)", + session_id=None, + is_error=True, + ) + + with patch("app.services.claude_code.phase_agent_fn", side_effect=fake_phase_fn), \ + patch("app.services.claude_code.is_skip_mode_enabled", return_value=False): + with pytest.raises(WorkspaceAbortedError) as exc_info: + await execute_all_phases( + planning_data=planning, + workspace=mock_ws, + manager=mock_mgr, + request=request, + logger=Mock(), + generation_session_service=svc, + workspace_id="ws-1", + ) + + assert "lost connection" in str(exc_info.value).lower() + # The linchpin assertion: the errored phase was NOT checkpointed as completed. + svc.update_workspace_phase.assert_not_called() + svc.update_deployment_workspace_phase.assert_not_called() + + +@pytest.mark.asyncio +async def test_connection_error_aborts_on_first_occurrence(tmp_path: Path) -> None: + """Unlike a generic error (tolerated up to 2), a single connection loss aborts immediately.""" + planning = _planning_one_phase() + request = GenerateAppRequest(spec_path="specs", outputs_dir="specflow", generation_id="e1") + + calls = {"n": 0} + + async def fake_phase_fn(**kwargs): + calls["n"] += 1 + return AgentResult( + result="API Error: The socket connection was closed unexpectedly.", + session_id=None, + is_error=True, + ) + + with patch("app.services.claude_code.phase_agent_fn", side_effect=fake_phase_fn), \ + patch("app.services.claude_code.is_skip_mode_enabled", return_value=False): + with pytest.raises(WorkspaceAbortedError): + await execute_all_phases( + planning_data=planning, + workspace=_mock_workspace(tmp_path), + manager=_mock_manager(), + request=request, + logger=Mock(), + ) + + # Aborted after the very first errored phase — no second attempt. + assert calls["n"] == 1