Feat/provider overload break (#242)

* feat(conventions): standard schema models + effective-map merge

* feat(orchestrator): park provider on persistent server overload (529/500)

A 429 rate limit already parks a provider — queue its spawns, probe until it
recovers — but a persistent 529/500/503 overload had no such break: the run
died and the orchestrator crash-retried straight back into the overload,
burning tokens in a respawn loop.

Generalize the park to provider-unavailability. On a non-graceful Anthropic
agent exit, match the API's overload markers (overloaded_error /
internal_server_error / "API Error: 5xx") against the dead container's own
output and park the provider with kind="overloaded"; the existing spawn gate
already queues any parked provider, and the probe-resume loop revives the task
when it recovers. Grok keeps its exit-75 path; both now route through one
_park_provider_unavailable helper. Markers are kept specific so an agent that
merely writes about HTTP 500/529 can't trip the break.

Fix the recovery probe to require a 2xx: it treated any non-429 as recovered,
so a probe that itself got a 529 would have resumed agents straight back into
the overload — wrong for the new path and for a 429 that lifts into a 5xx.

Gated by ROBOCO_OVERLOAD_BREAK_ENABLED (default on; off => crash-retry).

---------

Co-authored-by: Renn F <rennf93@users.noreply.github.com>

tests/unit/foundation/policy/conventions/test_conventions_models.py

@@ -0,0 +1,99 @@
+"""Schema-model + YAML-parse tests for the architectural-conventions standard."""
+
+from __future__ import annotations
+
+import pytest
+from roboco.foundation.policy.conventions.models import (
+    BUILTIN_RULES,
+    ConventionsParseError,
+    ConventionsStandard,
+    CustomRule,
+    Module,
+    Rule,
+    Waiver,
+)
+
+_VALID_YAML = """
+version: 1
+languages: [python, typescript]
+modules:
+  - path: app/routers
+    purpose: HTTP routes
+    forbidden: [model, helper]
+  - path: app/models
+    purpose: Pydantic / ORM models
+rules:
+  no_models_in_routers: { level: block }
+  no_inline_comments: { level: warn }
+custom:
+  - id: no-print
+    pattern: '\\bprint\\('
+    message: use the logger
+    level: warn
+    languages: [python]
+waivers:
+  - path: app/routers/legacy.py
+    rule: no_models_in_routers
+    reason: extraction tracked separately
+"""
+
+
+def test_valid_yaml_parses_to_standard() -> None:
+    std = ConventionsStandard.parse_yaml(_VALID_YAML)
+    assert std.version == 1
+    assert std.languages == ["python", "typescript"]
+    assert std.modules[0].path == "app/routers"
+    assert std.modules[0].forbidden == ["model", "helper"]
+    assert std.rules["no_models_in_routers"].level == "block"
+    assert std.rules["no_models_in_routers"].name == "no_models_in_routers"
+    assert std.custom[0].id == "no-print"
+    assert std.custom[0].languages == ["python"]
+    assert std.waivers[0].rule == "no_models_in_routers"
+
+
+def test_empty_yaml_yields_default_standard() -> None:
+    std = ConventionsStandard.parse_yaml("")
+    assert std == ConventionsStandard()
+    assert std.version == 1
+
+
+def test_unknown_rule_level_raises_parse_error() -> None:
+    with pytest.raises(ConventionsParseError):
+        ConventionsStandard.parse_yaml(
+            "rules:\n  no_models_in_routers: { level: explode }\n"
+        )
+
+
+def test_malformed_yaml_raises_parse_error() -> None:
+    with pytest.raises(ConventionsParseError):
+        ConventionsStandard.parse_yaml("modules: [unterminated\n")
+
+
+def test_non_mapping_top_level_raises_parse_error() -> None:
+    with pytest.raises(ConventionsParseError):
+        ConventionsStandard.parse_yaml("- just\n- a\n- list\n")
+
+
+def test_unknown_definition_kind_in_forbidden_raises() -> None:
+    with pytest.raises(ConventionsParseError):
+        ConventionsStandard.parse_yaml(
+            "modules:\n  - path: x\n    purpose: y\n    forbidden: [wizard]\n"
+        )
+
+
+def test_builtin_rules_cover_the_org_defaults() -> None:
+    assert BUILTIN_RULES["no_models_in_routers"] == "block"
+    assert BUILTIN_RULES["no_helpers_in_routers"] == "block"
+    assert BUILTIN_RULES["no_lint_suppressions"] == "block"
+    assert BUILTIN_RULES["no_inline_comments"] == "warn"
+
+
+def test_models_construct_directly() -> None:
+    mod = Module(path="app/services", purpose="logic", forbidden=["route"])
+    assert mod.forbidden == ["route"]
+    rule = Rule(name="no_print", level="warn")
+    assert rule.level == "warn"
+    custom = CustomRule(id="x", pattern="y", message="z", level="block")
+    assert custom.languages == []
+    waiver = Waiver(path="a.py", rule="no_models_in_routers", reason="r")
+    assert waiver.path == "a.py"

tests/unit/foundation/policy/conventions/test_effective_map.py

@@ -0,0 +1,89 @@
+"""Effective-map merge tests: auto-derived defaults overlaid by the file."""
+
+from __future__ import annotations
+
+from roboco.foundation.policy.conventions.effective_map import effective_map
+from roboco.foundation.policy.conventions.models import (
+    ConventionsStandard,
+    CustomRule,
+    Module,
+    Rule,
+    Waiver,
+)
+
+
+def test_effective_map_applies_builtin_rules_when_file_absent() -> None:
+    eff = effective_map(ConventionsStandard(), None)
+    assert eff.rules["no_models_in_routers"].level == "block"
+    assert eff.rules["no_inline_comments"].level == "warn"
+
+
+def test_file_module_overrides_derived_by_path() -> None:
+    derived = ConventionsStandard(
+        modules=[Module(path="app/routers", purpose="routes")]
+    )
+    file = ConventionsStandard(
+        modules=[Module(path="app/routers", purpose="routes", forbidden=["model"])]
+    )
+    eff = effective_map(derived, file)
+    assert len(eff.modules) == 1
+    assert eff.modules[0].forbidden == ["model"]
+
+
+def test_file_module_appends_new_path() -> None:
+    derived = ConventionsStandard(
+        modules=[Module(path="app/routers", purpose="routes")]
+    )
+    file = ConventionsStandard(modules=[Module(path="app/models", purpose="models")])
+    eff = effective_map(derived, file)
+    assert [m.path for m in eff.modules] == ["app/routers", "app/models"]
+
+
+def test_file_rule_overrides_builtin_level() -> None:
+    file = ConventionsStandard(
+        rules={"no_inline_comments": Rule(name="no_inline_comments", level="block")}
+    )
+    eff = effective_map(ConventionsStandard(), file)
+    assert eff.rules["no_inline_comments"].level == "block"
+
+
+def test_derived_rule_overrides_builtin_then_file_overrides_derived() -> None:
+    derived = ConventionsStandard(
+        rules={"no_inline_comments": Rule(name="no_inline_comments", level="block")}
+    )
+    eff_no_file = effective_map(derived, None)
+    assert eff_no_file.rules["no_inline_comments"].level == "block"
+    file = ConventionsStandard(
+        rules={"no_inline_comments": Rule(name="no_inline_comments", level="warn")}
+    )
+    eff = effective_map(derived, file)
+    assert eff.rules["no_inline_comments"].level == "warn"
+
+
+def test_languages_are_unioned() -> None:
+    derived = ConventionsStandard(languages=["python"])
+    file = ConventionsStandard(languages=["python", "typescript"])
+    eff = effective_map(derived, file)
+    assert eff.languages == ["python", "typescript"]
+
+
+def test_file_custom_and_waivers_replace_derived() -> None:
+    derived = ConventionsStandard(
+        custom=[CustomRule(id="d", pattern="d", message="d", level="warn")],
+        waivers=[Waiver(path="d.py", rule="no_models_in_routers", reason="d")],
+    )
+    file = ConventionsStandard(
+        custom=[CustomRule(id="f", pattern="f", message="f", level="block")],
+        waivers=[Waiver(path="f.py", rule="no_helpers_in_routers", reason="f")],
+    )
+    eff = effective_map(derived, file)
+    assert [c.id for c in eff.custom] == ["f"]
+    assert [w.path for w in eff.waivers] == ["f.py"]
+
+
+def test_file_none_keeps_derived_custom_and_waivers() -> None:
+    derived = ConventionsStandard(
+        custom=[CustomRule(id="d", pattern="d", message="d", level="warn")],
+    )
+    eff = effective_map(derived, None)
+    assert [c.id for c in eff.custom] == ["d"]

tests/unit/runtime/test_grok_rate_limit.py

@@ -35,10 +35,17 @@ class _FakeTracker:
     async def is_rate_limited(self) -> bool:
         return self._limited
 
-    async def activate(self, *, retry_after: float, affected_agents: list[str]) -> None:
+    async def activate(
+        self,
+        *,
+        retry_after: float,
+        affected_agents: list[str],
+        kind: str = "rate_limited",
+    ) -> None:
         self.activated_with = {
             "retry_after": retry_after,
             "affected_agents": affected_agents,
+            "kind": kind,
         }
 
 
@@ -115,6 +122,7 @@ async def test_park_grok_rate_limited_activates_and_offlines(
     assert tracker.activated_with == {
         "retry_after": pytest.approx(60.0),
         "affected_agents": ["be-dev-1"],
+        "kind": "rate_limited",
     }
 
 

tests/unit/runtime/test_provider_overload_break.py

@@ -0,0 +1,194 @@
+"""Server-overload parking: break the 529/500 -> crash -> respawn cost loop.
+
+A persistent overload (HTTP 529 / 500 / 503) from the model API kills the run;
+the orchestrator parks the provider — the same break as a 429 rate limit —
+instead of crash-retrying straight back into the overload. The probe-resume
+loop revives the task when the provider recovers. These tests exercise the
+decision points deterministically (logs + tracker + finalize stubbed).
+"""
+
+from __future__ import annotations
+
+from unittest.mock import AsyncMock
+
+import pytest
+from roboco.config import settings
+from roboco.models.runtime import AgentInstance
+from roboco.runtime.orchestrator import (
+    _OVERLOAD_RETRY_AFTER_S,
+    AgentOrchestrator,
+    AgentState,
+)
+
+_OVERLOAD_LOG = (
+    'API Error: 529 {"type":"error","error":{"type":"overloaded_error",'
+    '"message":"Overloaded"}}'
+)
+_CLEAN_LOG = "be-dev-1 finished editing src/app.py; all checks passed"
+
+
+def _instance(provider_type: str | None = "anthropic") -> AgentInstance:
+    cfg = type(
+        "C",
+        (),
+        {"provider_type": provider_type, "model": "claude-x", "git_context": None},
+    )()
+    inst = AgentInstance(agent_id="be-dev-1", state=AgentState.ACTIVE, config=cfg)
+    inst.current_task_id = "task-1"
+    inst.container_id = "cid"
+    return inst
+
+
+@pytest.fixture
+def orch() -> AgentOrchestrator:
+    return AgentOrchestrator.__new__(AgentOrchestrator)
+
+
+class _FakeTracker:
+    def __init__(self) -> None:
+        self.activated_with: dict[str, object] | None = None
+
+    async def activate(
+        self, *, retry_after: float, affected_agents: list[str], kind: str
+    ) -> None:
+        self.activated_with = {
+            "retry_after": retry_after,
+            "affected_agents": affected_agents,
+            "kind": kind,
+        }
+
+
+# ---------------------------------------------------------------------------
+# _provider_overload_park_target — the detection decision
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_detects_overload_marker_for_anthropic(
+    orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    monkeypatch.setattr(settings, "overload_break_enabled", True)
+    monkeypatch.setattr(
+        orch, "_tail_container_logs", AsyncMock(return_value=_OVERLOAD_LOG)
+    )
+    assert (
+        await orch._provider_overload_park_target("be-dev-1", _instance())
+        == "anthropic"
+    )
+
+
+@pytest.mark.asyncio
+async def test_clean_output_is_not_overload(
+    orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    monkeypatch.setattr(settings, "overload_break_enabled", True)
+    monkeypatch.setattr(
+        orch, "_tail_container_logs", AsyncMock(return_value=_CLEAN_LOG)
+    )
+    assert await orch._provider_overload_park_target("be-dev-1", _instance()) is None
+
+
+@pytest.mark.asyncio
+async def test_disabled_flag_never_parks(
+    orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    monkeypatch.setattr(settings, "overload_break_enabled", False)
+    tail = AsyncMock(return_value=_OVERLOAD_LOG)
+    monkeypatch.setattr(orch, "_tail_container_logs", tail)
+    assert await orch._provider_overload_park_target("be-dev-1", _instance()) is None
+    tail.assert_not_awaited()  # short-circuits before reading logs
+
+
+@pytest.mark.asyncio
+async def test_grok_provider_is_skipped(
+    orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Grok has its own exit-75 detector; the log-marker path ignores it."""
+    monkeypatch.setattr(settings, "overload_break_enabled", True)
+    monkeypatch.setattr(
+        orch, "_tail_container_logs", AsyncMock(return_value=_OVERLOAD_LOG)
+    )
+    assert (
+        await orch._provider_overload_park_target("gk-dev-1", _instance("grok")) is None
+    )
+
+
+# ---------------------------------------------------------------------------
+# _park_provider_unavailable — the park action
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_park_offlines_and_activates_with_kind(
+    orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    inst = _instance()
+    inst.error_count = 2  # prior crashes — parking must NOT count one
+    tracker = _FakeTracker()
+    monkeypatch.setattr(orch, "_make_tracker", lambda _p: tracker)
+    monkeypatch.setattr(orch, "_finalize_spawn_session", AsyncMock())
+
+    await orch._park_provider_unavailable(
+        "be-dev-1", inst, provider="anthropic", retry_after=45.0, kind="overloaded"
+    )
+
+    assert inst.state == AgentState.OFFLINE
+    assert inst.container_id is None
+    assert inst.error_count == 0
+    assert tracker.activated_with == {
+        "retry_after": pytest.approx(45.0),
+        "affected_agents": ["be-dev-1"],
+        "kind": "overloaded",
+    }
+
+
+# ---------------------------------------------------------------------------
+# _handle_stopped_container — overload short-circuits the crash-retry path
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_stopped_container_parks_on_overload(
+    orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    inst = _instance()
+    park = AsyncMock()
+    spawn = AsyncMock()
+    monkeypatch.setattr(orch, "_is_grok_rate_limit_exit", lambda _i, _e: False)
+    monkeypatch.setattr(
+        orch, "_provider_overload_park_target", AsyncMock(return_value="anthropic")
+    )
+    monkeypatch.setattr(orch, "_park_provider_unavailable", park)
+    monkeypatch.setattr(orch, "_finalize_spawn_session", AsyncMock())
+    monkeypatch.setattr(orch, "spawn_agent", spawn)
+
+    await orch._handle_stopped_container("be-dev-1", inst, exit_code=1)
+
+    park.assert_awaited_once_with(
+        "be-dev-1",
+        inst,
+        provider="anthropic",
+        retry_after=_OVERLOAD_RETRY_AFTER_S,
+        kind="overloaded",
+    )
+    spawn.assert_not_awaited()  # the crash-retry path is short-circuited
+
+
+@pytest.mark.asyncio
+async def test_stopped_container_crash_retries_when_not_overload(
+    orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    inst = _instance()
+    inst.error_count = 0
+    spawn = AsyncMock()
+    monkeypatch.setattr(orch, "_is_grok_rate_limit_exit", lambda _i, _e: False)
+    monkeypatch.setattr(
+        orch, "_provider_overload_park_target", AsyncMock(return_value=None)
+    )
+    monkeypatch.setattr(orch, "_finalize_spawn_session", AsyncMock())
+    monkeypatch.setattr(orch, "spawn_agent", spawn)
+
+    await orch._handle_stopped_container("be-dev-1", inst, exit_code=1)
+
+    # Not an overload → the normal crash-retry path runs.
+    spawn.assert_awaited_once()

tests/unit/runtime/test_rate_limit_probe.py

@@ -2,9 +2,10 @@
 
 ``_do_probe`` replaced a time-based stub that always returned True. It now
 makes a free, unmetered call (Anthropic ``GET /v1/models`` / Ollama
-``GET /api/tags``) and treats any non-429 response as the rate limit having
-lifted. These tests pin that contract: target resolution per provider, the
-429-vs-not decision, network-error → stay-parked, and the un-probeable
+``GET /api/tags``) and treats only a 2xx response as the provider having
+recovered — a 429 (rate limit) and a 5xx (overload) both keep it parked.
+These tests pin that contract: target resolution per provider, the
+2xx-vs-error decision, network-error → stay-parked, and the un-probeable
 fallback to time-expiry optimism.
 """
 
@@ -111,6 +112,17 @@ async def test_probe_anthropic_429_stays_limited(orch: AgentOrchestrator) -> Non
         assert await orch._do_probe("anthropic") is False
 
 
+@pytest.mark.usefixtures("with_anthropic_key")
+@pytest.mark.parametrize("status", [500, 503, 529])
+async def test_probe_anthropic_5xx_stays_parked(
+    orch: AgentOrchestrator, status: int
+) -> None:
+    """A 5xx (overload) keeps the provider parked — resuming would re-overload it."""
+    fake = _fake_async_client(status_code=status)
+    with patch("roboco.runtime.orchestrator.httpx.AsyncClient", fake):
+        assert await orch._do_probe("anthropic") is False
+
+
 @pytest.mark.usefixtures("with_anthropic_key")
 async def test_probe_network_error_stays_parked(orch: AgentOrchestrator) -> None:
     fake = _fake_async_client(raise_exc=httpx.ConnectError("boom"))

tests/unit/services/test_rate_limit_tracker.py

@@ -108,6 +108,21 @@ class TestActivateAndRead:
         state = await tracker.get_state()
         assert state["probe_failures"] == 0
 
+    async def test_activate_defaults_kind_to_rate_limited(self) -> None:
+        mock = _make_redis_mock()
+        tracker = _make_tracker(redis_mock=mock)
+        await tracker.activate()
+        state = await tracker.get_state()
+        assert state["kind"] == "rate_limited"
+
+    async def test_activate_stores_overloaded_kind(self) -> None:
+        mock = _make_redis_mock()
+        tracker = _make_tracker(redis_mock=mock)
+        await tracker.activate(kind="overloaded")
+        state = await tracker.get_state()
+        assert state["kind"] == "overloaded"
+        assert await tracker.is_rate_limited() is True  # gates spawns either way
+
     async def test_clear_removes_state(self) -> None:
         mock = _make_redis_mock()
         tracker = _make_tracker(redis_mock=mock)