mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): bounded fail-open evidence assembly + PM decision transient-failure bypass
Evidence-assembly git legs (diff, changed-files, branch fetch, advisory conventions run) ran unbounded inside claim_review / claim_doc_task / claim_gate_review / evidence() / i_am_done's envelope build, so a slow clone turned the whole verb into a silent 120s FlowVerbTimeout 504. Each leg now runs through run_bounded_leg under a shared LegBudget (evidence_assembly_timeout_seconds, 45s total): a timed-out leg — both asyncio TimeoutError and git's own GitTimeoutError — degrades into an evidence_gaps note on the envelope instead of hanging the verb, while non-timeout git errors still propagate. The advisory conventions run gets an inner-only timeout (conventions_validator_advisory_timeout_ seconds, 30s) threaded down to the subprocess so it is never orphaned by an outer cancel; the fail-closed i_am_done/pr_pass conventions gates keep their hardcoded 120s. _ensure_pm_decision now reports a PmDecisionOutcome: a transient DB failure recording the PM's decision journal (e.g. lock timeout under load) no longer launders into a journal:decision gate rejection that escalates and BLOCKS the task — the verb's own rationale satisfies the gate with a structured warning, across all seven PM verbs. Also: repo-wide ruff realignment to the lockfile-pinned ruff (8 format-only diffs, 14 UP038 isinstance conversions) that a transiently newer venv ruff had masked. Gate: 15474 passed, 459 skipped; ruff/mypy/xenon/vulture/bandit/ pip-audit/deptry/import-linter/foundation-check all green.
This commit is contained in:
@@ -49,7 +49,6 @@ async def test_get_bool_parses_and_defaults() -> None:
|
||||
async def test_apply_overrides_stored_flags_only(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
|
||||
# Baseline env defaults.
|
||||
monkeypatch.setattr(cfg, "external_pr_enabled", False)
|
||||
monkeypatch.setattr(cfg, "research_enabled", True)
|
||||
@@ -73,7 +72,6 @@ async def test_apply_overrides_stored_flags_only(
|
||||
async def test_effective_values_use_env_default_when_unset(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
|
||||
monkeypatch.setattr(cfg, "strategy_engine_enabled", True)
|
||||
|
||||
async def fake_get(_self: SettingsService, _key: str) -> str | None:
|
||||
|
||||
@@ -149,3 +149,80 @@ async def test_validator_timeout_fails_closed_and_reaps(
|
||||
assert "timed out" in (result.get("reason") or "")
|
||||
fake_proc.kill.assert_called_once()
|
||||
fake_proc.wait.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_conventions_validator_timeout_override_used_over_hardcoded(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""An explicit ``timeout`` kwarg wins over the module-level hardcoded
|
||||
default — the advisory claim_review path's shorter budget must actually
|
||||
reach the subprocess wait, not the fail-closed 120s cap. Sets the module
|
||||
constant to something LONG (would never fire in the test's real time
|
||||
budget) so a failure here would prove the override was ignored, not a
|
||||
coincidence of both values being short."""
|
||||
fake_proc = MagicMock()
|
||||
fake_proc.returncode = None
|
||||
|
||||
async def _communicate() -> tuple[bytes, bytes]:
|
||||
await asyncio.sleep(30)
|
||||
return (b"", b"")
|
||||
|
||||
fake_proc.communicate = _communicate
|
||||
fake_proc.kill = MagicMock()
|
||||
fake_proc.wait = AsyncMock(return_value=-9)
|
||||
|
||||
async def _fake_exec(*_args: object, **_kwargs: object) -> object:
|
||||
return fake_proc
|
||||
|
||||
monkeypatch.setattr(asyncio, "create_subprocess_exec", _fake_exec)
|
||||
monkeypatch.setattr(git_module, "_CONVENTIONS_VALIDATOR_TIMEOUT_SECONDS", 300)
|
||||
|
||||
svc = _service()
|
||||
result = await svc._run_conventions_validator(tmp_path, ["a.py"], timeout=0.01)
|
||||
assert result["could_not_run"] is True
|
||||
assert "timed out after 0.01s" in (result.get("reason") or "")
|
||||
|
||||
|
||||
def _task_with_id(branch_name: str) -> MagicMock:
|
||||
"""Like ``_task`` but with a real UUID id — ``conventions_check_for_task``
|
||||
calls ``require_uuid(task.id)`` outside the resolution try/except, so a
|
||||
bare MagicMock id would raise before reaching the validator call."""
|
||||
return MagicMock(branch_name=branch_name, id=uuid4())
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_conventions_check_for_task_forwards_timeout_override() -> None:
|
||||
"""``conventions_check_for_task``'s ``timeout`` kwarg must reach
|
||||
``_run_conventions_validator`` — the seam ``claim_review`` uses to pin
|
||||
the ADVISORY (shorter) budget instead of the fail-closed default."""
|
||||
svc = _service()
|
||||
_bind(svc, "_workspace_for_branch", AsyncMock(return_value=Path("/tmp/ws")))
|
||||
_bind(svc, "list_changed_files", AsyncMock(return_value=["a.py"]))
|
||||
_bind(svc, "_worktree_for_task", MagicMock(return_value=Path("/tmp/wt")))
|
||||
_bind(svc, "_ensure_worktree_for_commit", AsyncMock(return_value=None))
|
||||
validator = AsyncMock(return_value={"findings": [], "could_not_run": False})
|
||||
_bind(svc, "_run_conventions_validator", validator)
|
||||
|
||||
await svc.conventions_check_for_task(
|
||||
uuid4(), _task_with_id("feature/backend/abc"), timeout=30.0
|
||||
)
|
||||
validator.assert_awaited_once_with(Path("/tmp/wt"), ["a.py"], timeout=30.0)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_conventions_check_for_task_default_timeout_is_none() -> None:
|
||||
"""The fail-closed callers (i_am_done's ``_conventions_gate``, pr_pass's
|
||||
``_conventions_guard``) never pass ``timeout`` — confirming the default
|
||||
forwards ``None`` so ``_run_conventions_validator`` falls back to its
|
||||
hardcoded fail-closed cap, unchanged."""
|
||||
svc = _service()
|
||||
_bind(svc, "_workspace_for_branch", AsyncMock(return_value=Path("/tmp/ws")))
|
||||
_bind(svc, "list_changed_files", AsyncMock(return_value=["a.py"]))
|
||||
_bind(svc, "_worktree_for_task", MagicMock(return_value=Path("/tmp/wt")))
|
||||
_bind(svc, "_ensure_worktree_for_commit", AsyncMock(return_value=None))
|
||||
validator = AsyncMock(return_value={"findings": [], "could_not_run": False})
|
||||
_bind(svc, "_run_conventions_validator", validator)
|
||||
|
||||
await svc.conventions_check_for_task(uuid4(), _task_with_id("feature/backend/abc"))
|
||||
validator.assert_awaited_once_with(Path("/tmp/wt"), ["a.py"], timeout=None)
|
||||
|
||||
@@ -158,7 +158,7 @@ async def test_conventions_check_runs_validator_in_worktree_not_clone() -> None:
|
||||
captured: list[Path] = []
|
||||
|
||||
async def _capture_validator(
|
||||
workspace: Path, _files: list[str]
|
||||
workspace: Path, _files: list[str], **_kwargs: object
|
||||
) -> dict[str, object]:
|
||||
captured.append(Path(workspace))
|
||||
return {"findings": [], "could_not_run": False}
|
||||
|
||||
@@ -169,7 +169,6 @@ async def test_create_rejects_cell_pm_assignee_plus_code() -> None:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_allows_cell_pm_assignee_plus_planning() -> None:
|
||||
|
||||
be_pm_uuid = AGENTS["be-pm"].uuid
|
||||
svc = TaskService(
|
||||
MagicMock(add=MagicMock(), flush=AsyncMock(), execute=AsyncMock())
|
||||
|
||||
@@ -0,0 +1,132 @@
|
||||
"""fetch_branch_for_inspection's subprocess_timeout override (adversarial-review
|
||||
round-2 fix 3): the fetch subprocess must self-bound near the caller's own leg
|
||||
budget instead of running up to workspace_clone_timeout (300s) on the shared
|
||||
DEFAULT asyncio executor (asyncio.to_thread, not git.py's dedicated
|
||||
_GIT_EXECUTOR) after the caller has already given up waiting on it. A
|
||||
timeout there now raises GitTimeoutError (mirroring _run_git's own
|
||||
TimeoutExpired -> GitTimeoutError conversion in git.py), not a raw
|
||||
subprocess.TimeoutExpired, so a bounded caller (run_bounded_leg) catches it
|
||||
the same way as every other git-touching leg.
|
||||
|
||||
The clone-CREATION step (ensure_workspace, stubbed out here) is untouched by
|
||||
this fix and keeps its own workspace_clone_timeout (300s) unconditionally —
|
||||
these tests isolate the FETCH subprocess only.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import subprocess
|
||||
from typing import TYPE_CHECKING, Any
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.config import settings
|
||||
from roboco.exceptions import GitTimeoutError
|
||||
from roboco.services.workspace import WorkspaceService
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from pathlib import Path
|
||||
|
||||
# Named constant to satisfy ruff PLR2004 (magic value in comparison).
|
||||
_EXPECTED_TIMEOUT_SECONDS = 5
|
||||
|
||||
|
||||
def _service() -> WorkspaceService:
|
||||
session = MagicMock()
|
||||
session.execute = AsyncMock()
|
||||
return WorkspaceService(session)
|
||||
|
||||
|
||||
def _bind(svc: WorkspaceService, name: str, value: object) -> None:
|
||||
"""Stub `name` on `svc` without tripping mypy's method-assign check."""
|
||||
object.__setattr__(svc, name, value)
|
||||
|
||||
|
||||
def _wire_resolution(svc: WorkspaceService, workspace: Path) -> None:
|
||||
"""Stub the resolution steps ahead of the fetch subprocess so only the
|
||||
fetch itself is under test."""
|
||||
_bind(svc, "_resolve_branch_to_project_slug", AsyncMock(return_value="roboco"))
|
||||
_bind(svc, "ensure_workspace", AsyncMock(return_value=workspace))
|
||||
|
||||
|
||||
def _no_project_service_patch() -> Any:
|
||||
"""No git token (project=None skips the decrypt path entirely)."""
|
||||
return patch(
|
||||
"roboco.services.project.get_project_service",
|
||||
return_value=MagicMock(get_by_slug=AsyncMock(return_value=None)),
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_default_subprocess_timeout_is_workspace_clone_timeout(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Every EXISTING caller omits subprocess_timeout — behavior byte-for-byte
|
||||
unchanged: the fetch subprocess keeps the 300s workspace_clone_timeout."""
|
||||
svc = _service()
|
||||
_wire_resolution(svc, tmp_path)
|
||||
captured: list[object] = []
|
||||
|
||||
def _fake_run(*_args: object, **kwargs: object) -> subprocess.CompletedProcess[str]:
|
||||
captured.append(kwargs.get("timeout"))
|
||||
return subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="")
|
||||
|
||||
with (
|
||||
patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run),
|
||||
patch("roboco.services.workspace._ensure_agent_owned"),
|
||||
_no_project_service_patch(),
|
||||
):
|
||||
await svc.fetch_branch_for_inspection(agent_id=uuid4(), branch_name="feature/x")
|
||||
|
||||
assert captured == [settings.workspace_clone_timeout]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_subprocess_timeout_override_reaches_the_fetch(tmp_path: Path) -> None:
|
||||
svc = _service()
|
||||
_wire_resolution(svc, tmp_path)
|
||||
captured: list[object] = []
|
||||
|
||||
def _fake_run(*_args: object, **kwargs: object) -> subprocess.CompletedProcess[str]:
|
||||
captured.append(kwargs.get("timeout"))
|
||||
return subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="")
|
||||
|
||||
with (
|
||||
patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run),
|
||||
patch("roboco.services.workspace._ensure_agent_owned"),
|
||||
_no_project_service_patch(),
|
||||
):
|
||||
await svc.fetch_branch_for_inspection(
|
||||
agent_id=uuid4(), branch_name="feature/x", subprocess_timeout=12.5
|
||||
)
|
||||
|
||||
assert captured == [12.5]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_timeout_expired_becomes_git_timeout_error(tmp_path: Path) -> None:
|
||||
"""Mirrors _run_git's own TimeoutExpired -> GitTimeoutError conversion
|
||||
(git.py) so run_bounded_leg catches this uniformly with every other
|
||||
git-touching leg, instead of a raw subprocess.TimeoutExpired propagating
|
||||
uncaught to the RobocoError handler."""
|
||||
svc = _service()
|
||||
_wire_resolution(svc, tmp_path)
|
||||
|
||||
def _fake_run(*_args: object, **kwargs: object) -> subprocess.CompletedProcess[str]:
|
||||
timeout = kwargs.get("timeout")
|
||||
raise subprocess.TimeoutExpired(
|
||||
cmd="git fetch",
|
||||
timeout=float(timeout) if isinstance(timeout, int | float) else 0,
|
||||
)
|
||||
|
||||
with (
|
||||
patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run),
|
||||
patch("roboco.services.workspace._ensure_agent_owned"),
|
||||
_no_project_service_patch(),
|
||||
pytest.raises(GitTimeoutError) as exc_info,
|
||||
):
|
||||
await svc.fetch_branch_for_inspection(
|
||||
agent_id=uuid4(), branch_name="feature/x", subprocess_timeout=5.0
|
||||
)
|
||||
assert exc_info.value.timeout == _EXPECTED_TIMEOUT_SECONDS
|
||||
Reference in New Issue
Block a user