diff --git a/roboco/config.py b/roboco/config.py index c259ec9e..f5acc90e 100644 --- a/roboco/config.py +++ b/roboco/config.py @@ -1173,6 +1173,16 @@ class Settings(BaseSettings): le=1.0, description="Cosine-similarity floor for injected memory; below it, none.", ) + institutional_memory_timeout_seconds: float = Field( + default=8.0, + gt=0, + description=( + "Deadline for the institutional-memory RAG search (embed + query) " + "during a claim briefing. RAG memory is a nice-to-have enrichment " + "— tight on purpose, so a saturated embedder can never burn the " + "verb's whole timeout budget and 504 the claim." + ), + ) # Sandboxed per-agent-spawn DB/Redis — orchestrator-provisioned throwaway # Postgres/Redis sibling containers so a dev agent's gate runs against an diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 7e1c4041..771abdd5 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -11,6 +11,7 @@ injection so later phases just fill in the bodies. from __future__ import annotations +import asyncio import contextlib from dataclasses import dataclass from datetime import UTC, datetime @@ -1181,7 +1182,8 @@ class Choreographer: Returns ``{"status": ..., "lessons": [...]}`` where status is one of ``disabled`` (subsystem off / no task — ponytail: both mean no search ran), - ``error`` (search raised), ``empty`` (search yielded nothing), + ``error`` (search raised), ``timeout`` (searched, but the memory search + didn't answer in time), ``empty`` (search yielded nothing), ``below_floor`` (searched, nothing met the floor), ``ok`` (lessons injected). Lessons is empty unless status is ``ok`` — the status is additive, the injection behavior is unchanged.""" @@ -1200,11 +1202,22 @@ class Choreographer: else: task_type = str(raw_type) query = shape_memory_query(role, title, task_type) - result = await self._deps.evidence_repo.similar_memory( - query=query, - top_k=_settings.org_memory_top_k, - min_score=_settings.org_memory_min_score, - ) + try: + result = await asyncio.wait_for( + self._deps.evidence_repo.similar_memory( + query=query, + top_k=_settings.org_memory_top_k, + min_score=_settings.org_memory_min_score, + ), + timeout=_settings.institutional_memory_timeout_seconds, + ) + except TimeoutError: + logger.warning( + "institutional_memory_timeout", + agent_id=str(agent_id), + task_id=str(getattr(task, "id", "")), + ) + return {"status": "timeout", "lessons": []} return { "status": str(result.get("status", "error")), "lessons": list(result.get("items", [])), diff --git a/roboco/services/gateway/choreographer/qa.py b/roboco/services/gateway/choreographer/qa.py index 4ac3ab67..13eed6e4 100644 --- a/roboco/services/gateway/choreographer/qa.py +++ b/roboco/services/gateway/choreographer/qa.py @@ -229,6 +229,12 @@ class QAMixin(_Base): was already resolved (warmed) by the diff/list_changed_files legs above in the same evidence build — so the one theoretically-unbounded piece (a cold clone) never actually triggers here in practice. + + That warm-workspace assumption only holds when those legs actually + completed. ``_build_qa_claim_evidence`` now checks for the opposite + signal (``evidence_gaps`` already non-empty) and skips calling this + method entirely in that case, instead of trusting a false assumption + and re-hitting the same cold/contended git ops unbounded. """ if not settings.conventions_enabled: return [] @@ -321,15 +327,36 @@ class QAMixin(_Base): # Leaf-only journals stay (include_ancestors defaults False above); # ancestor *descriptions* are the ask, not work-so-far. parent_context = await self.evidence_repo.ancestor_context_for_task(task_id) - convention_findings = await self._qa_convention_findings( - qa_agent_id, - t, - timeout=min( - settings.conventions_validator_advisory_timeout_seconds, - budget.remaining(), - ), - gaps=evidence_gaps, - ) + convention_findings: list[dict[str, Any]] + if evidence_gaps and settings.conventions_enabled: + # The diff/files_changed legs above already timed out, so the + # "workspace was warmed" assumption _qa_convention_findings relies + # on (see its docstring) is false — skip it rather than re-hit the + # same cold/contended git ops and grind toward the 120s verb wall. + convention_findings = [ + { + "could_not_run": True, + "reason": ( + "skipped: git evidence legs timed out " + "(cold/contended workspace)" + ), + } + ] + evidence_gaps.append( + "conventions findings unavailable: skipped because prior " + "evidence legs timed out (cold/contended workspace) — review " + "the diff manually for architecture-convention issues" + ) + else: + convention_findings = await self._qa_convention_findings( + qa_agent_id, + t, + timeout=min( + settings.conventions_validator_advisory_timeout_seconds, + budget.remaining(), + ), + gaps=evidence_gaps, + ) open_findings = await findings_lib.open_findings_for_task( self.task.session, t.id ) diff --git a/tests/unit/gateway/test_conventions_in_qa_evidence.py b/tests/unit/gateway/test_conventions_in_qa_evidence.py index 9c3b56ce..e6aad609 100644 --- a/tests/unit/gateway/test_conventions_in_qa_evidence.py +++ b/tests/unit/gateway/test_conventions_in_qa_evidence.py @@ -2,6 +2,7 @@ from __future__ import annotations +import asyncio from typing import Any from unittest.mock import AsyncMock, MagicMock from uuid import uuid4 @@ -100,3 +101,146 @@ def test_evidence_payload_convention_findings_default_empty() -> None: ev = build_evidence_for_task(_stub_task(), journal_highlights=[], files_changed=[]) assert ev.convention_findings == [] assert "convention_findings" not in ev.as_dict() + + +# --------------------------------------------------------------------------- +# _build_qa_claim_evidence: skip conventions when the git legs above already +# timed out (evidence_gaps non-empty — the warm-workspace assumption failed). +# --------------------------------------------------------------------------- + + +def _stub_empty_ledger(session: MagicMock) -> None: + session.execute = AsyncMock( + return_value=MagicMock( + scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[]))) + ) + ) + + +def _evidence_choreographer(git_svc: AsyncMock) -> Choreographer: + """Full Choreographer wired for ``_build_qa_claim_evidence`` directly + (not the whole claim_review verb) — journal/evidence_repo/session + stubbed empty so the build reaches the conventions call site cleanly.""" + task_svc = AsyncMock() + _stub_empty_ledger(task_svc.session) + evidence_repo = AsyncMock() + evidence_repo.journal_highlights_for_task.return_value = [] + evidence_repo.ancestor_context_for_task.return_value = [] + deps = ChoreographerDeps( + task=task_svc, + work_session=AsyncMock(), + git=git_svc, + a2a=AsyncMock(), + journal=AsyncMock(), + audit=AsyncMock(), + evidence_repo=evidence_repo, + ) + return Choreographer(deps) + + +def _evidence_task(task_id: Any) -> MagicMock: + return MagicMock( + id=task_id, + branch_name="feature/backend/abc", + pr_number=8, + pr_url="https://github.com/x/y/pull/8", + commits=[], + dev_notes=None, + acceptance_criteria_status=[], + parent_task_id=None, + description=None, + ) + + +@pytest.mark.asyncio +async def test_conventions_skipped_when_git_legs_already_timed_out( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Both the diff and list_changed_files legs time out (evidence_gaps + ends up non-empty) — _qa_convention_findings (and the real + conventions_check_for_task it would call) must never run; the caller + fills in a could_not_run skip entry and its own evidence_gaps note + instead.""" + monkeypatch.setattr(settings, "conventions_enabled", True) + monkeypatch.setattr(settings, "evidence_assembly_timeout_seconds", 0.02) + + async def _hangs(*_args: object, **_kwargs: object) -> Any: + await asyncio.sleep(5) + return "unreachable" + + git_svc = AsyncMock() + git_svc.diff.side_effect = _hangs + git_svc.list_changed_files.side_effect = _hangs + c = _evidence_choreographer(git_svc) + task_id = uuid4() + t = _evidence_task(task_id) + + ev = await c._build_qa_claim_evidence(uuid4(), t, task_id) + body = ev.as_dict() + + git_svc.conventions_check_for_task.assert_not_awaited() + assert body["convention_findings"] == [ + { + "could_not_run": True, + "reason": "skipped: git evidence legs timed out (cold/contended workspace)", + } + ] + gaps = body["evidence_gaps"] + assert any("pr diff unavailable" in g for g in gaps) + assert any("files_changed unavailable" in g for g in gaps) + assert any("conventions findings unavailable" in g for g in gaps) + + +@pytest.mark.asyncio +async def test_conventions_runs_when_legs_succeed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The skip only engages when evidence_gaps is non-empty — a normal + build (both legs succeed) still runs conventions exactly as before.""" + monkeypatch.setattr(settings, "conventions_enabled", True) + git_svc = AsyncMock() + git_svc.diff.return_value = "diff content" + git_svc.list_changed_files.return_value = ["README.md"] + git_svc.conventions_check_for_task.return_value = { + "findings": [], + "could_not_run": False, + } + c = _evidence_choreographer(git_svc) + task_id = uuid4() + t = _evidence_task(task_id) + + ev = await c._build_qa_claim_evidence(uuid4(), t, task_id) + body = ev.as_dict() + + git_svc.conventions_check_for_task.assert_awaited_once() + assert "evidence_gaps" not in body + + +@pytest.mark.asyncio +async def test_conventions_not_skipped_stub_when_flag_off( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Even with degraded legs, a disabled subsystem must stay empty (no + misleading could_not_run stub implying conventions would otherwise have + run) — the skip guard checks conventions_enabled too.""" + monkeypatch.setattr(settings, "conventions_enabled", False) + monkeypatch.setattr(settings, "evidence_assembly_timeout_seconds", 0.02) + + async def _hangs(*_args: object, **_kwargs: object) -> Any: + await asyncio.sleep(5) + return "unreachable" + + git_svc = AsyncMock() + git_svc.diff.side_effect = _hangs + git_svc.list_changed_files.side_effect = _hangs + c = _evidence_choreographer(git_svc) + task_id = uuid4() + t = _evidence_task(task_id) + + ev = await c._build_qa_claim_evidence(uuid4(), t, task_id) + body = ev.as_dict() + + git_svc.conventions_check_for_task.assert_not_awaited() + assert "convention_findings" not in body + gaps = body["evidence_gaps"] + assert not any("conventions findings unavailable" in g for g in gaps) diff --git a/tests/unit/gateway/test_institutional_memory_timeout.py b/tests/unit/gateway/test_institutional_memory_timeout.py new file mode 100644 index 00000000..e975f536 --- /dev/null +++ b/tests/unit/gateway/test_institutional_memory_timeout.py @@ -0,0 +1,79 @@ +"""``_institutional_memory``'s RAG search is bounded by +``institutional_memory_timeout_seconds`` — a saturated Ollama embedder must +never eat the whole verb timeout budget and 504 a claim. See +``roboco.services.gateway.choreographer._impl.Choreographer._institutional_memory``. +""" + +from __future__ import annotations + +import asyncio +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.config import Settings +from roboco.services.gateway.choreographer import Choreographer + +_DEFAULT_TIMEOUT = 8.0 +_OVERRIDE_TIMEOUT = 3.5 + + +def _choreographer(*, similar_memory: AsyncMock) -> Choreographer: + repo = AsyncMock() + repo.similar_memory = similar_memory + task_svc = AsyncMock() + task_svc.agent_for.return_value = MagicMock(role="developer") + choreo = object.__new__(Choreographer) + choreo._deps = MagicMock(evidence_repo=repo, task=task_svc) + return choreo + + +def _task() -> MagicMock: + return MagicMock( + id=uuid4(), title="Add retry backoff", task_type=MagicMock(value="code") + ) + + +class TestInstitutionalMemoryTimeout: + @pytest.mark.asyncio + async def test_slow_search_times_out_without_raising( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr("roboco.config.settings.org_memory_enabled", True) + monkeypatch.setattr( + "roboco.config.settings.institutional_memory_timeout_seconds", 0.01 + ) + + async def _slow(**_kwargs: object) -> dict[str, object]: + await asyncio.sleep(5) + return {"items": [], "status": "ok"} + + choreo = _choreographer(similar_memory=AsyncMock(side_effect=_slow)) + result = await choreo._institutional_memory(uuid4(), _task()) + assert result == {"status": "timeout", "lessons": []} + + @pytest.mark.asyncio + async def test_fast_search_flows_through_unchanged( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr("roboco.config.settings.org_memory_enabled", True) + monkeypatch.setattr( + "roboco.config.settings.institutional_memory_timeout_seconds", 8.0 + ) + lesson = {"kind": "learning", "summary": "s", "source": "src", "score": 0.9} + similar_memory = AsyncMock(return_value={"items": [lesson], "status": "ok"}) + choreo = _choreographer(similar_memory=similar_memory) + result = await choreo._institutional_memory(uuid4(), _task()) + assert result == {"status": "ok", "lessons": [lesson]} + + +class TestInstitutionalMemoryTimeoutConfig: + def test_default_is_eight_seconds(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("ROBOCO_INSTITUTIONAL_MEMORY_TIMEOUT_SECONDS", raising=False) + assert Settings().institutional_memory_timeout_seconds == _DEFAULT_TIMEOUT + + def test_reads_env_override(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv( + "ROBOCO_INSTITUTIONAL_MEMORY_TIMEOUT_SECONDS", str(_OVERRIDE_TIMEOUT) + ) + assert Settings().institutional_memory_timeout_seconds == _OVERRIDE_TIMEOUT