From c34e978f9ef3fb787443d47ccbde54918553b6ad Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 21:23:56 +0200 Subject: [PATCH] [F101] enforce PR-open state gate on gateway open_pr (parity with HTTP path) --- roboco/foundation/policy/lifecycle.py | 41 +++++++++ roboco/services/git.py | 21 ++--- tests/foundation/test_lifecycle_spec.py | 106 ++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 12 deletions(-) diff --git a/roboco/foundation/policy/lifecycle.py b/roboco/foundation/policy/lifecycle.py index cd24d5b6..ffe2e0cc 100644 --- a/roboco/foundation/policy/lifecycle.py +++ b/roboco/foundation/policy/lifecycle.py @@ -951,6 +951,46 @@ PRECONDITION_NON_TERMINAL = Precondition( ) +# The set of states from which a PR may be opened — the lifecycle-owned canon. +# The HTTP PR-create path (GitService._assert_pr_create_allowed) and the gateway +# ``open_pr`` intent must agree on this, so it lives here (the policy layer) as +# the single source and the service derives its str set from it. A PR opens +# during active dev (in_progress / verifying), the doc phase +# (awaiting_documentation), QA review (awaiting_qa), or a rework cycle +# (needs_revision) — never from claim/pause/block/terminal, which the HTTP path +# already blocked but the gateway ``open_pr`` (composes=() → no source-status +# gate) historically did not (F101). +PR_OPEN_STATES: frozenset[Status] = frozenset( + { + Status.IN_PROGRESS, + Status.VERIFYING, + Status.AWAITING_QA, + Status.AWAITING_DOCUMENTATION, + Status.NEEDS_REVISION, + } +) + + +def _p_pr_open_state(task: Any, _agent: Any, _ctx: Any) -> bool: + """True iff the task is in a PR-open-eligible state (see ``PR_OPEN_STATES``).""" + status = getattr(task, "status", None) + value = status.value if isinstance(status, Status) else str(status) + return value in {s.value for s in PR_OPEN_STATES} + + +PRECONDITION_PR_OPEN_STATE = Precondition( + key="pr_open_state", + check=_p_pr_open_state, + remediate=( + "open_pr is only valid during active dev states " + "(in_progress / verifying / awaiting_qa / awaiting_documentation / " + "needs_revision); move the task into one of those first" + ), + missing_token="pr_open_state", + rejection_kind="invalid_state", +) + + _INTENT_VERBS: dict[str, IntentSpec] = { # Phase 1: developer verbs "give_me_work": IntentSpec( @@ -1022,6 +1062,7 @@ _INTENT_VERBS: dict[str, IntentSpec] = { composes=(), extra_preconditions=( PRECONDITION_OWNERSHIP, + PRECONDITION_PR_OPEN_STATE, PRECONDITION_COMMITS, PRECONDITION_NO_PR, ), diff --git a/roboco/services/git.py b/roboco/services/git.py index 157d4111..21d69a41 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio import base64 +import contextlib import json import os import re @@ -47,6 +48,7 @@ from roboco.exceptions import ( GitTimeoutError, MergeConflictError, ) +from roboco.foundation.policy import lifecycle from roboco.models.base import AgentRole, TaskStatus from roboco.services.base import ( BaseService, @@ -141,12 +143,10 @@ def _remove_stale_git_locks(workspace: Path) -> None: return try: for lock in git_dir.rglob("*.lock"): - try: + # A lock a real process just grabbed, or a permission issue — + # leave it. The TTL/next-op path is the backstop. + with contextlib.suppress(OSError): lock.unlink() - except OSError: - # A lock a real process just grabbed, or a permission issue — - # leave it. The TTL/next-op path is the backstop. - pass except OSError: return @@ -2509,14 +2509,11 @@ class GitService(BaseService): "updated_fields": updated, } + # PR-open-eligible states — the lifecycle policy owns the canon + # (``PR_OPEN_STATES``); the HTTP path derives its str set from it so the + # gateway ``open_pr`` spec gate and this HTTP gate can never drift. _PR_OPEN_STATES: ClassVar[frozenset[str]] = frozenset( - { - TaskStatus.IN_PROGRESS.value, - TaskStatus.VERIFYING.value, - TaskStatus.AWAITING_QA.value, - TaskStatus.AWAITING_DOCUMENTATION.value, - TaskStatus.NEEDS_REVISION.value, - } + s.value for s in lifecycle.PR_OPEN_STATES ) @staticmethod diff --git a/tests/foundation/test_lifecycle_spec.py b/tests/foundation/test_lifecycle_spec.py index e489d263..e1d959c7 100644 --- a/tests/foundation/test_lifecycle_spec.py +++ b/tests/foundation/test_lifecycle_spec.py @@ -566,6 +566,112 @@ def test_can_invoke_intent_developer_open_pr_no_commits_tracing_gap() -> None: assert "commits>=1" in d.missing +# --------------------------------------------------------------------------- # +# F101: open_pr must enforce the PR-open state gate (parity with the HTTP path) +# --------------------------------------------------------------------------- # + + +def _owned_task(**overrides: Any) -> SimpleNamespace: + """A task owned by ``actor`` with commits and no prior PR — only the state + gate can fail, isolating the PR-open-state precondition.""" + actor = overrides.pop("actor_id", uuid4()) + return _stub_task( + assigned_to=actor, + commits=["abc123"], + pr_number=None, + **overrides, + ) + + +def test_open_pr_rejected_on_claimed_task() -> None: + """F101: ``open_pr`` has ``composes=()`` so the spec gate applied NO + source-status check — a dev could open a PR from ``claimed`` (before + ``in_progress``), skipping the active-dev state the HTTP path's + ``_assert_pr_create_allowed`` enforces. The state gate now rejects it.""" + actor = uuid4() + d = spec.can_invoke_intent( + spec.Role.DEVELOPER, + "open_pr", + _owned_task(status="claimed", actor_id=actor), + context=spec.Context(actor_id=actor), + ) + assert d.allowed is False + assert d.rejection_kind == "invalid_state" + + +def test_open_pr_rejected_on_paused_task() -> None: + actor = uuid4() + d = spec.can_invoke_intent( + spec.Role.DEVELOPER, + "open_pr", + _owned_task(status="paused", actor_id=actor), + context=spec.Context(actor_id=actor), + ) + assert d.allowed is False + assert d.rejection_kind == "invalid_state" + + +def test_open_pr_rejected_on_blocked_task() -> None: + actor = uuid4() + d = spec.can_invoke_intent( + spec.Role.DEVELOPER, + "open_pr", + _owned_task(status="blocked", actor_id=actor), + context=spec.Context(actor_id=actor), + ) + assert d.allowed is False + assert d.rejection_kind == "invalid_state" + + +def test_open_pr_rejected_on_completed_task() -> None: + """A completed task is terminal — opening a PR on it is nonsensical.""" + actor = uuid4() + d = spec.can_invoke_intent( + spec.Role.DEVELOPER, + "open_pr", + _owned_task(status="completed", actor_id=actor), + context=spec.Context(actor_id=actor), + ) + assert d.allowed is False + assert d.rejection_kind == "invalid_state" + + +@pytest.mark.parametrize( + "status", + [ + "in_progress", + "verifying", + "awaiting_qa", + "awaiting_documentation", + "needs_revision", + ], +) +def test_open_pr_allowed_in_pr_open_states(status: str) -> None: + """Regression guard: every PR-open-eligible state still lets the owner open + a PR — the new state gate must not over-restrict the legitimate path.""" + actor = uuid4() + d = spec.can_invoke_intent( + spec.Role.DEVELOPER, + "open_pr", + _owned_task(status=status, actor_id=actor), + context=spec.Context(actor_id=actor), + ) + assert d.allowed is True, f"open_pr should be allowed from {status}" + + +def test_open_pr_state_gate_takes_priority_over_unowned() -> None: + """A non-owner in a wrong state: ownership (not_authorized) is checked + before state, mirroring the HTTP path's assignee-first ordering.""" + d = spec.can_invoke_intent( + spec.Role.DEVELOPER, + "open_pr", + _owned_task(status="claimed", actor_id=uuid4()), + context=spec.Context(actor_id=uuid4()), # different actor -> not owner + ) + assert d.allowed is False + assert d.rejection_kind == "not_authorized" + + def test_escalate_up_rejected_on_completed_task() -> None: """F043: a PM must not resurrect a COMPLETED task via escalate_up.