From c8bb16e63470b5beffa2867f5b0b1cab2a920295 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 21:41:49 +0200 Subject: [PATCH] [F104] fail-closed on conventions resolution errors (block gate no longer silently disabled) --- roboco/services/git.py | 19 ++-- .../test_git_conventions_check_fail_closed.py | 93 +++++++++++++++++++ 2 files changed, 106 insertions(+), 6 deletions(-) create mode 100644 tests/unit/services/test_git_conventions_check_fail_closed.py diff --git a/roboco/services/git.py b/roboco/services/git.py index 2c465dda..b38da26d 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -4304,10 +4304,13 @@ class GitService(BaseService): """Run the conventions validator on a task's changed files. Resolves the acting agent's workspace + the branch's changed files and - runs ``python -m roboco.conventions`` over them. Fail-open on a - resolution error (returns no findings, ``could_not_run=False``); the - validator's OWN fail-loud (exit 3) sets ``could_not_run=True`` so the - gate blocks rather than passing on an unanalyzable diff. + runs ``python -m roboco.conventions`` over them. A resolution error + (workspace missing, diff failed) fails CLOSED — returns + ``could_not_run=True`` so the block gate refuses the submit instead of + silently passing on an unanalyzable diff (the validator's OWN fail-loud + exit-3 philosophy). The two empty-result paths stay fail-open: a + branchless task (no ``branch_name``) and a task with no changed files + genuinely have nothing to validate, so the gate correctly passes. """ try: branch = task.branch_name @@ -4319,8 +4322,12 @@ class GitService(BaseService): changed = await self.list_changed_files( branch_name=branch, actor_agent_id=actor_agent_id ) - except Exception: - return {"findings": [], "could_not_run": False} + except Exception as exc: + return { + "findings": [], + "could_not_run": True, + "reason": f"resolution failed: {exc}"[:300], + } if not changed: return {"findings": [], "could_not_run": False} return await self._run_conventions_validator(workspace, changed) diff --git a/tests/unit/services/test_git_conventions_check_fail_closed.py b/tests/unit/services/test_git_conventions_check_fail_closed.py new file mode 100644 index 00000000..2d85878e --- /dev/null +++ b/tests/unit/services/test_git_conventions_check_fail_closed.py @@ -0,0 +1,93 @@ +"""conventions_check_for_task must fail CLOSED on a resolution error. + +The conventions block gate (i_am_done / pr_pass) treats +``could_not_run=False`` + no findings as a clean PASS. A workspace or diff +resolution error that returns ``could_not_run=False`` therefore silently +disables the block gate — the opposite of the validator's OWN fail-loud +philosophy (exit 3 → ``could_not_run=True`` → gate blocks, never silently +passes). A raised exception during workspace/diff resolution is a real error, +not an empty result, so it must fail closed: ``could_not_run=True``. + +The two LEGITIMATE fail-open paths stay fail-open: +- no ``branch_name`` (a branchless/coordination task has nothing to validate) +- no changed files (nothing changed → nothing to validate) +These are empty-result cases, not errors, so the gate correctly passes. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.services.git import GitService + + +def _service() -> GitService: + return GitService(MagicMock()) + + +def _bind(svc: GitService, name: str, value: object) -> None: + object.__setattr__(svc, name, value) + + +def _task(branch_name: str) -> MagicMock: + return MagicMock(branch_name=branch_name) + + +@pytest.mark.asyncio +async def test_workspace_resolution_error_fails_closed() -> None: + """``_workspace_for_branch`` raising → could_not_run=True (block), not + False (silent pass).""" + svc = _service() + _bind( + svc, + "_workspace_for_branch", + AsyncMock(side_effect=RuntimeError("workspace clone missing")), + ) + result = await svc.conventions_check_for_task(uuid4(), _task("feature/backend/abc")) + assert result["could_not_run"] is True + assert result["findings"] == [] + # The reason is informational (for logs/debug), capped like the validator path. + assert isinstance(result.get("reason"), str) and result["reason"] + + +@pytest.mark.asyncio +async def test_diff_resolution_error_fails_closed() -> None: + """``list_changed_files`` raising → could_not_run=True (block), not a + silent pass. The workspace resolved fine, but the diff itself failed.""" + svc = _service() + _bind(svc, "_workspace_for_branch", AsyncMock(return_value=Path("/tmp/ws"))) + _bind( + svc, + "list_changed_files", + AsyncMock(side_effect=RuntimeError("git diff errored")), + ) + result = await svc.conventions_check_for_task(uuid4(), _task("feature/backend/abc")) + assert result["could_not_run"] is True + assert result["findings"] == [] + + +@pytest.mark.asyncio +async def test_no_branch_still_fails_open() -> None: + """A branchless/coordination task (no branch_name) has nothing to + validate — the gate correctly passes (could_not_run=False). This is NOT + an error; it must stay fail-open.""" + svc = _service() + result = await svc.conventions_check_for_task(uuid4(), _task("")) + assert result["could_not_run"] is False + assert result["findings"] == [] + + +@pytest.mark.asyncio +async def test_no_changed_files_still_fails_open() -> None: + """A task whose branch resolved but has no changed files has nothing to + validate — the gate correctly passes (could_not_run=False). This is an + empty result, not an error; it must stay fail-open.""" + svc = _service() + _bind(svc, "_workspace_for_branch", AsyncMock(return_value=Path("/tmp/ws"))) + _bind(svc, "list_changed_files", AsyncMock(return_value=[])) + result = await svc.conventions_check_for_task(uuid4(), _task("feature/backend/abc")) + assert result["could_not_run"] is False + assert result["findings"] == []