diff --git a/.gitignore b/.gitignore index ab231381..2585d8ad 100644 --- a/.gitignore +++ b/.gitignore @@ -98,4 +98,3 @@ panel/.env.local panel/.env.*.local # Internal-only: strategy/scratch/reference dumps — never publish docs/internal/ -SMOKE_FINDINGS_*.md diff --git a/roboco/api/routes/tasks.py b/roboco/api/routes/tasks.py index c83de488..356c8124 100644 --- a/roboco/api/routes/tasks.py +++ b/roboco/api/routes/tasks.py @@ -526,12 +526,35 @@ async def update_task( # Transform input data for database storage updates = transform_update_data(data) + # `status` is not a free-form field — it is an audited admin override so a + # privileged operator can recover a task wedged in a state with no valid + # in-band transition. Pop it out of the generic field update and apply it + # through the audited path, gated on elevated permissions. + new_status = updates.pop("status", None) + task = await service.update(task_id, **updates) if not task: raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Task update failed unexpectedly", ) + if new_status is not None and new_status != task.status: + if not has_higher_perms: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only privileged roles may override task status.", + ) + task = await service.admin_set_status( + task_id, + new_status, + actor_id=agent.agent_id, + actor_role=getattr(agent, "role", None), + ) + if not task: + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Task status override failed unexpectedly", + ) await db.commit() return task_to_response(task) diff --git a/roboco/api/schemas/tasks.py b/roboco/api/schemas/tasks.py index 1ffdfd38..81f7a8eb 100644 --- a/roboco/api/schemas/tasks.py +++ b/roboco/api/schemas/tasks.py @@ -218,6 +218,11 @@ class TaskUpdate(BaseModel): auditor_notes: str | None = None quick_context: str | None = None + # Lifecycle override — privileged/admin only. Applied by the route as an + # audited force-transition (so an operator can recover a task wedged in a + # state with no valid in-band move), never as a free-form field set. + status: TaskStatus | None = None + @model_validator(mode="before") @classmethod def _reject_explicit_blank_acceptance_criteria(cls, data: Any) -> Any: diff --git a/roboco/config.py b/roboco/config.py index e76a5ee1..67c8471a 100644 --- a/roboco/config.py +++ b/roboco/config.py @@ -279,6 +279,25 @@ class Settings(BaseSettings): ), ) + session_idle_timeout_seconds: int = Field( + default=3600, + ge=30, + description=( + "Idle seconds before a messaging session is swept closed. The " + "previous 300s default was shorter than a human conversation pause, " + "so a person's chat session expired and reopened between messages." + ), + ) + + protected_git_urls: list[str] = Field( + default_factory=list, + description=( + "Repo URL substrings a project may not point at (e.g. the roboco " + "source repo). Blocks agent commits/merges from reaching a protected " + "repository; set this to sandbox smoke-test projects." + ), + ) + # ========================================================================== # Agent Guardrails (per-session budgets, loop detection, SLAs) # ========================================================================== diff --git a/roboco/events/stream_bus.py b/roboco/events/stream_bus.py index 34611fb7..90188f7f 100644 --- a/roboco/events/stream_bus.py +++ b/roboco/events/stream_bus.py @@ -15,6 +15,7 @@ from typing import Any import redis.asyncio as redis import structlog from redis.exceptions import ResponseError +from redis.exceptions import TimeoutError as RedisTimeoutError from roboco.config import settings from roboco.models.events import Event, EventType @@ -219,6 +220,12 @@ class StreamEventBus: if await self._handle_response_error(e, streams): continue await asyncio.sleep(1) + except (RedisTimeoutError, TimeoutError): + # An idle XREADGROUP(block=...) hits the client read-timeout when + # no new message arrives within the block window. This is the + # normal idle path, not an error — re-block on the next iteration + # without logging or back-off. + continue except Exception as e: logger.error("Error in stream event loop", error=str(e)) await asyncio.sleep(1) diff --git a/roboco/exceptions.py b/roboco/exceptions.py index ece4b911..8bfa36b0 100644 --- a/roboco/exceptions.py +++ b/roboco/exceptions.py @@ -5,6 +5,7 @@ Structured exception hierarchy for the AI Agents Company system. All exceptions include context for debugging and logging. """ +import re from typing import Any, ClassVar from uuid import UUID @@ -435,16 +436,41 @@ class GitError(ServiceError): ) +def _scrub_git_secrets(text: str) -> str: + """Redact credentials a git command may echo into stderr. + + Push/fetch run with the PAT injected via a URL or an ``http.extraheader`` + Basic header; never surface those verbatim in an error message or log. + """ + if not text: + return text + text = re.sub(r"(://)[^/@\s]+@", r"\1***@", text) + text = re.sub(r"(?i)(authorization:\s*basic\s+)\S+", r"\1***", text) + text = re.sub(r"(?i)(extraheader=\S*?basic\s+)\S+", r"\1***", text) + text = re.sub( + r"\b(gh[pousr]_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,})\b", "***", text + ) + return text + + class GitCommandError(GitError): """Git command execution failed.""" def __init__(self, command: str, stderr: str) -> None: + scrubbed = _scrub_git_secrets(stderr or "") + # Surface a short, secret-free tail of git's own stderr in the message so + # the real reason (403, non-fast-forward, ...) is visible to callers that + # only render ``.message`` instead of swallowing it as "Command failed". + tail = " ".join(scrubbed.split())[-300:] + message = f"Command failed: {command}" + if tail: + message = f"{message} — {tail}" super().__init__( - message=f"Command failed: {command}", - details={"command": command, "stderr": stderr}, + message=message, + details={"command": command, "stderr": scrubbed}, ) self.command = command - self.stderr = stderr + self.stderr = scrubbed class GitTimeoutError(GitError): diff --git a/roboco/models/llm_catalog.py b/roboco/models/llm_catalog.py index fc2d2671..58c733f8 100644 --- a/roboco/models/llm_catalog.py +++ b/roboco/models/llm_catalog.py @@ -118,7 +118,7 @@ OLLAMA_ROLE_DEFAULTS: dict[str, str] = { } # The Ollama model picked for "pure Ollama" mode's GLOBAL row when the -# caller doesn't override. Kimi K2.6 wins as the generalist because it has +# caller doesn't override. Minimax M3 wins as the generalist because it has # the strongest reasoning/tool-use profile and can fall back to coding/writing # adequately if a role ends up mapped to the global default. -OLLAMA_DEFAULT_MODEL: str = "kimi-k2.6:cloud" +OLLAMA_DEFAULT_MODEL: str = "minimax-m3:cloud" diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 6c70799d..e2c9f397 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -4808,11 +4808,16 @@ Never `commit`, never write code, never run `git`. PMs coordinate. async def _handle_dev_existing_owner( self, task: dict[str, Any], status: str, agent_slug: str ) -> None: - """Respawn existing dev for needs_revision / in_progress / claimed / blocked.""" + """Respawn existing dev for needs_revision / in_progress / claimed.""" + # A `blocked` task is waiting for its blocker to clear (PM / dependency); + # the owner has no legal move from `blocked`, so respawning it does + # nothing but churn. It is revived only when unblocked back to + # in_progress, or released to the pool (unclaim) for re-delegation. + if status == "blocked": + return if status in ( "in_progress", "claimed", - "blocked", ) and not self._is_agent_active(agent_slug): logger.info( "Respawning agent for orphaned task", diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 5502850a..4f37cde7 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -1503,14 +1503,14 @@ class Choreographer: @staticmethod def _extract_first_commit_sha(t: Any) -> str | None: - """Read the first commit sha off the task, dict or model alike.""" + """Read the first commit hash off the task, dict or model alike.""" commits: list[Any] = list(getattr(t, "commits", []) or []) if not commits: return None first = commits[0] if isinstance(first, dict): - return first.get("sha") - return getattr(first, "sha", None) + return first.get("hash") or first.get("sha") + return getattr(first, "hash", None) or getattr(first, "sha", None) @staticmethod def _already_addressed_criteria(existing_status: list[dict[str, Any]]) -> set[str]: @@ -2707,11 +2707,21 @@ class Choreographer: Failure is logged and swallowed — the pause already happened and the caller must not be affected by a checkpoint DB error. """ - commit_refs = [c.sha for c in (task.commits or [])[-3:]] - commit_count = len(task.commits or []) - state_summary = f"auto-paused on i_am_idle (commits: {commit_count})" - remaining_work = commit_refs if commit_refs else ["no commits yet"] try: + commits = task.commits or [] + # commits may be hydrated as CommitRef objects or as plain dicts + # (JSON column round-trip); the identifier field is `hash` (a stray + # `sha` only ever appears on a gateway return value, never persisted). + commit_refs = [ + (c.get("hash") or c.get("sha")) + if isinstance(c, dict) + else (getattr(c, "hash", None) or getattr(c, "sha", None)) + for c in commits[-3:] + ] + commit_refs = [ref for ref in commit_refs if ref] + commit_count = len(commits) + state_summary = f"auto-paused on i_am_idle (commits: {commit_count})" + remaining_work = commit_refs if commit_refs else ["no commits yet"] await self.task.add_checkpoint( task_id=task.id, agent_id=agent_id, diff --git a/roboco/services/messaging.py b/roboco/services/messaging.py index 9b7f3a61..f9293e1f 100644 --- a/roboco/services/messaging.py +++ b/roboco/services/messaging.py @@ -20,6 +20,7 @@ from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import joinedload, selectinload +from roboco.config import settings from roboco.db.tables import ( ChannelTable, GroupTable, @@ -378,6 +379,19 @@ class MessagingService(BaseService): # SESSION OPERATIONS (TASK-015) # ========================================================================= + @staticmethod + def _resolve_session_timeout(requested: int | None) -> int: + """Resolve a session's idle-timeout, defaulting to the configurable value. + + An unset timeout previously fell through to the column default of 300s, + which is shorter than a human conversation pause — the session was swept + between messages and a new one opened on the next post. Resolve it to + ``session_idle_timeout_seconds`` so human-paced chats stay on one session. + """ + if requested is not None: + return requested + return settings.session_idle_timeout_seconds + async def create_session(self, req: SessionCreateRequest) -> SessionTable: """ Create a new session in a group. @@ -404,7 +418,7 @@ class MessagingService(BaseService): group_id=req.group_id, max_message_count=req.max_message_count, max_content_length=req.max_content_length, - timeout_seconds=req.timeout_seconds, + timeout_seconds=self._resolve_session_timeout(req.timeout_seconds), status=SessionStatus.ACTIVE, scope=req.scope, ) @@ -643,7 +657,7 @@ class MessagingService(BaseService): max_time_window=(_minutes_to_timedelta(request.max_time_window_minutes)), max_message_count=request.max_message_count, max_content_length=request.max_content_length, - timeout_seconds=request.timeout_seconds, + timeout_seconds=self._resolve_session_timeout(request.timeout_seconds), status=SessionStatus.ACTIVE, ) self.session.add(new_session) @@ -1141,7 +1155,11 @@ class MessagingService(BaseService): group_id=cast("UUID", group.id), max_message_count=(req.config.max_message_count if req.config else None), max_content_length=(req.config.max_content_length if req.config else None), - timeout_seconds=(req.config.timeout_seconds if req.config else 300), + timeout_seconds=( + req.config.timeout_seconds + if req.config and req.config.timeout_seconds is not None + else settings.session_idle_timeout_seconds + ), scope=req.scope, ) diff --git a/roboco/services/optimal_brain/indexes/docs.py b/roboco/services/optimal_brain/indexes/docs.py index 8b3c9f8b..2d9c3c00 100644 --- a/roboco/services/optimal_brain/indexes/docs.py +++ b/roboco/services/optimal_brain/indexes/docs.py @@ -85,8 +85,16 @@ class DocsIndexPlugin(BaseIndexPlugin): ] return md_files + txt_files if source_path.exists(): - return [source_path] - logger.warning(f"Source not found: {source}") + # Only markdown/text files are docs; a recorded path to a source + # file (e.g. a .tsx) is not indexable and is skipped quietly. + if source_path.suffix.lower() in {".md", ".txt"}: + return [source_path] + logger.debug(f"Skipping non-doc source path: {source}") + return [] + # A documenter may record a path for a doc that was not written under + # the docs root; this is not actionable at index time, so log at debug + # instead of flooding a warning on every pass. + logger.debug(f"Doc source not found, skipping: {source}") return [] def _read_file_record( diff --git a/roboco/services/project.py b/roboco/services/project.py index ef5eab0c..98256e6f 100644 --- a/roboco/services/project.py +++ b/roboco/services/project.py @@ -11,7 +11,9 @@ from uuid import UUID from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession +from roboco.config import settings from roboco.db.tables import ProjectTable +from roboco.exceptions import ValidationError from roboco.models.base import Team from roboco.models.project import ProjectCreate, ProjectUpdate from roboco.services.base import BaseService, ConflictError, NotFoundError @@ -35,6 +37,22 @@ class ProjectService(BaseService): # CRUD OPERATIONS # ========================================================================= + def _assert_git_url_allowed(self, git_url: str | None) -> None: + """Reject a project repo URL that matches a protected (denylisted) repo. + + Keeps agent commits/merges out of a repository that must not receive + them — e.g. the roboco source repo during a smoke run. + """ + if not git_url: + return + for protected in settings.protected_git_urls: + if protected and protected in git_url: + raise ValidationError( + "Project git_url may not point at a protected repository " + f"('{protected}').", + field="git_url", + ) + async def create( self, data: ProjectCreate, @@ -61,6 +79,8 @@ class ProjectService(BaseService): resource_type="project", ) + self._assert_git_url_allowed(data.git_url) + # Encrypt git token if provided encrypted_token = None if data.git_token: @@ -141,6 +161,8 @@ class ProjectService(BaseService): if not project: return None + self._assert_git_url_allowed(data.git_url) + # Handle git_token specially (empty string clears, None leaves unchanged) token_updated = False if data.git_token is not None: diff --git a/roboco/services/task.py b/roboco/services/task.py index cb4f51de..013afd3b 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -87,6 +87,12 @@ _DESCENDANT_EXECUTABLE_TASK_TYPES: frozenset[str] = frozenset( {TaskType.CODE.value, TaskType.DOCUMENTATION.value, TaskType.DESIGN.value} ) +# Implementation-cell teams. A board/advisory role must never own a cell task — +# including the cell's own coordination/planning task (which carries a cell team +# but not a CODE/DOC/DESIGN type), so escalating one toward a board role is +# diverted to the cell pool instead of handing ownership up. +_CELL_TEAMS: frozenset[str] = frozenset({"backend", "frontend", "ux_ui"}) + def _is_descendant_executable_task(task: TaskTable) -> bool: """True for a child task that does cell-executed work (#14 guard). @@ -108,6 +114,19 @@ def _is_descendant_executable_task(task: TaskTable) -> bool: return str(type_value) in _DESCENDANT_EXECUTABLE_TASK_TYPES +def _is_cell_team_task(task: TaskTable) -> bool: + """True for a descendant task owned by an implementation cell. + + Complements ``_is_descendant_executable_task``: a cell's coordination / + planning task carries a cell ``team`` but not a CODE/DOC/DESIGN ``task_type``, + so the executable-type check alone would let it escalate onto a board role. + """ + if task.parent_task_id is None: + return False + team_value = getattr(task.team, "value", task.team) + return str(team_value) in _CELL_TEAMS + + # Notes fields (dev_notes, qa_notes, quick_context) are append-only — # every revision cycle adds more. Cap total size so a task that cycles # dozens of times doesn't grow into megabytes. When we exceed the cap, @@ -977,6 +996,47 @@ class TaskService(BaseService): ) return task + async def admin_set_status( + self, + task_id: UUID, + new_status: TaskStatus, + *, + actor_id: str | UUID | None = None, + actor_role: str | None = None, + ) -> TaskTable | None: + """Privileged override: set a task's status directly, always audited. + + Bypasses the strict transition validator so an operator can recover a + task wedged in a state with no valid in-band move (e.g. a ``blocked`` + task whose work already merged out-of-band). The change is recorded in + the audit log like any other transition — no status change may skip it. + """ + task = await self.get(task_id) + if not task: + return None + from_status = ( + task.status.value + if isinstance(task.status, TaskStatus) + else str(task.status) + ) + task.status = new_status + await self.session.flush() + self._emit_status_transition_audit( + task, + from_status=from_status, + to_status=new_status.value, + agent_role=actor_role, + audit_agent_id=actor_id, + ) + self.log.info( + "Task status set via admin override", + task_id=str(task_id), + from_status=from_status, + to_status=new_status.value, + actor=str(actor_id) if actor_id else None, + ) + return task + async def delete(self, task_id: UUID) -> bool: """Delete a task and all its descendants.""" task = await self.get(task_id) @@ -1738,8 +1798,16 @@ class TaskService(BaseService): from roboco.services.docs import DOCS_BASE_PATH path = Path(rel_path) + # An absolute path already rooted at the docs base may double the + # segment (``/app/docs/docs/...``) or simply be re-anchored here; reduce + # it to a path relative to the base so the normalization below applies + # uniformly. An absolute path OUTSIDE the docs root (e.g. a workspace + # source file) is returned as-is for the indexer to skip. if path.is_absolute(): - return str(path) + try: + path = path.relative_to(DOCS_BASE_PATH) + except ValueError: + return str(path) parts = path.parts if parts and parts[0] == DOCS_BASE_PATH.name: path = Path(*parts[1:]) if len(parts) > 1 else Path() @@ -2241,6 +2309,37 @@ class TaskService(BaseService): task.active_claimant_id = cast("Any", None) await self.session.flush() return task + # A task the agent owns but cannot advance — it is `blocked` (a blocker + # it cannot self-resolve, or a dependency block) — is otherwise a trap: + # from `blocked` the agent has no legal forward verb and the dispatcher + # keeps respawning it. Releasing the claim returns the task to the pool + # for the cell PM to re-delegate. Audited; the active WorkSession is + # abandoned so a re-claim does not trip the uniqueness constraint. + if task.status == TaskStatus.BLOCKED: + pre_status = ( + task.status.value + if isinstance(task.status, TaskStatus) + else str(task.status) + ) + prior_owner = cast("Any", task.claimed_by or task.assigned_to) + if task.work_session_id: + await self._abandon_work_session_best_effort( + task.work_session_id, reason="agent-unclaim-from-blocked" + ) + task.work_session_id = cast("Any", None) + task.status = TaskStatus.PENDING + task.assigned_to = cast("Any", None) + task.claimed_by = cast("Any", None) + task.active_claimant_id = cast("Any", None) + await self.session.flush() + self._emit_status_transition_audit( + task, + from_status=pre_status, + to_status=TaskStatus.PENDING.value, + agent_role=None, + audit_agent_id=prior_owner, + ) + return task if task.status not in (TaskStatus.CLAIMED, TaskStatus.IN_PROGRESS): return None @@ -3419,9 +3518,9 @@ class TaskService(BaseService): primitive — so both the gateway ``escalate`` verb and the HTTP escalate route are covered. """ - if _is_descendant_executable_task(task) and await self._is_board_advisory_agent( - target_agent_id - ): + if ( + _is_descendant_executable_task(task) or _is_cell_team_task(task) + ) and await self._is_board_advisory_agent(target_agent_id): await self._release_code_task_to_pool( task=task, escalator_slug=escalator_slug, @@ -5547,6 +5646,12 @@ class TaskService(BaseService): except ValueError: return await self.unblock(task_id, agent_role="cell_pm") + pre_status = ( + task.status.value + if isinstance(task.status, TaskStatus) + else str(task.status) + ) + restored_owner = cast("Any", task.pre_block_assignee or task.claimed_by) task.status = restored_status if task.pre_block_assignee: task.assigned_to = cast("Any", task.pre_block_assignee) @@ -5557,6 +5662,16 @@ class TaskService(BaseService): task.blocker_resolver_type = None task.blocker_raised_by = None await self.session.flush() + # This restore path sets the status directly (bypassing the strict + # transition validator), so emit the audit explicitly — no status + # change may skip the audit log. + self._emit_status_transition_audit( + task, + from_status=pre_status, + to_status=restored_status.value, + agent_role=None, + audit_agent_id=restored_owner, + ) return task async def cell_pm_complete( @@ -5659,6 +5774,12 @@ class TaskService(BaseService): re-dispatch source), and appends an audit note explaining why the board hand-off was refused. """ + pre_status = ( + task.status.value + if isinstance(task.status, TaskStatus) + else str(task.status) + ) + prior_owner = cast("Any", task.claimed_by or task.assigned_to) task.assigned_to = cast("Any", None) task.claimed_by = cast("Any", None) task.active_claimant_id = cast("Any", None) @@ -5673,6 +5794,16 @@ class TaskService(BaseService): ) task.dev_notes = existing_notes + note await self.session.flush() + # This path sets PENDING directly (bypassing the strict transition + # validator), so emit the task.pending audit explicitly — no status + # change may skip the audit log. + self._emit_status_transition_audit( + task, + from_status=pre_status, + to_status=TaskStatus.PENDING.value, + agent_role=None, + audit_agent_id=prior_owner, + ) self.log.info( "Descendant executable task released to pool instead of board escalation", task_id=str(task.id), diff --git a/scripts/reset_runtime_state.sh b/scripts/reset_runtime_state.sh index 057e5176..5e62b0d6 100755 --- a/scripts/reset_runtime_state.sh +++ b/scripts/reset_runtime_state.sh @@ -22,6 +22,11 @@ # if none exist). # SKIP_WORKSPACE_RESET=1 — skip the workspace cleanup step entirely # (DB + Redis only). +# FULL_RESET=1 — opt-in aggressive clean-slate: after the DB + Redis wipe, +# delete everything under the roboco data root EXCEPT ollama/postgres/redis +# (overridable via ROBOCO_DATA_ROOT) and clear the persisted agent Claude +# session dirs listed in ROBOCO_CLAUDE_STATE_DIRS (space-separated). Skips +# the per-workspace git reset (the clones are removed and re-cloned). set -euo pipefail @@ -71,6 +76,50 @@ if $DOCKER ps --format '{{.Names}}' | grep -q '^roboco-redis$'; then $DOCKER exec roboco-redis redis-cli FLUSHDB | sed 's/^/ /' fi +# Optional full clean-slate (opt-in via FULL_RESET=1): wipe everything under the +# roboco data root EXCEPT the persistent service stores (ollama / postgres / +# redis), and clear any persisted agent Claude session state that would +# otherwise replay across runs. Default OFF — the workspace git-reset below is +# the usual path. The data root is resolved from the workspaces root's parent +# (or ROBOCO_DATA_ROOT); the Claude-state paths come from ROBOCO_CLAUDE_STATE_DIRS +# (space-separated) so nothing is guessed. +if [ "${FULL_RESET:-0}" = "1" ]; then + DATA_ROOT="${ROBOCO_DATA_ROOT:-}" + if [ -z "$DATA_ROOT" ]; then + for candidate in /volume1/roboco/data /data; do + if [ -d "$candidate/workspaces" ]; then + DATA_ROOT="$candidate" + break + fi + done + fi + if [ -n "$DATA_ROOT" ] && [ -d "$DATA_ROOT" ]; then + echo ">>> FULL_RESET: wiping $DATA_ROOT/* except ollama/postgres/redis ..." + for entry in "$DATA_ROOT"/*; do + [ -e "$entry" ] || continue + case "$(basename "$entry")" in + ollama | postgres | redis) + echo " keep $(basename "$entry")" + ;; + *) + echo " wipe $(basename "$entry")" + rm -rf "$entry" + ;; + esac + done + else + echo ">>> FULL_RESET: no data root resolved — skipping data wipe." + fi + for cdir in ${ROBOCO_CLAUDE_STATE_DIRS:-}; do + if [ -e "$cdir" ]; then + echo " clearing Claude session state $cdir" + rm -rf "$cdir" + fi + done + echo ">>> FULL_RESET done." + exit 0 +fi + # Workspace reset — each agent has a private git clone at # {root}/{project}/{team}/{agent}. Leftover staged/untracked edits and # feature branches from a previous run will fail the claim→start diff --git a/tests/integration/test_messaging_service.py b/tests/integration/test_messaging_service.py index 7f94d1fe..a5c73355 100644 --- a/tests/integration/test_messaging_service.py +++ b/tests/integration/test_messaging_service.py @@ -11,6 +11,7 @@ from uuid import uuid4 as _u import pytest import pytest_asyncio +from roboco.config import settings from roboco.db.tables import AgentTable, MessageTable, ProjectTable, TaskTable from roboco.db.tables import AgentTable as _AgentTable from roboco.enforcement.channel_access import ChannelAccessDeniedError @@ -2705,3 +2706,14 @@ async def test_post_to_channel_permitted_agent_succeeds( ) assert msg.content == "hello cell" assert msg.task_id == task.id + + +def test_resolve_session_timeout_uses_configurable_default() -> None: + """An unset session timeout resolves to the configurable default instead of + the old 300s that swept human chats between messages.""" + explicit = settings.session_idle_timeout_seconds + 60 + assert MessagingService._resolve_session_timeout(explicit) == explicit + assert ( + MessagingService._resolve_session_timeout(None) + == settings.session_idle_timeout_seconds + ) diff --git a/tests/integration/test_project_service.py b/tests/integration/test_project_service.py index 6c1650b9..61098131 100644 --- a/tests/integration/test_project_service.py +++ b/tests/integration/test_project_service.py @@ -12,7 +12,9 @@ from uuid import uuid4 import pytest import pytest_asyncio +from roboco.config import settings from roboco.db.tables import AgentTable +from roboco.exceptions import ValidationError from roboco.models import AgentRole, AgentStatus, Team from roboco.models.project import ProjectCreate, ProjectUpdate from roboco.services.base import ConflictError, NotFoundError @@ -89,6 +91,19 @@ async def test_create_project_duplicate_slug_raises(project_setup: dict) -> None await svc.create(payload, project_setup["creator_id"]) +@pytest.mark.asyncio +async def test_create_project_rejects_protected_git_url( + project_setup: dict, monkeypatch: pytest.MonkeyPatch +) -> None: + """A project may not point at a denylisted repo (keeps agent merges out of it).""" + monkeypatch.setattr(settings, "protected_git_urls", ["github.com/owner/roboco"]) + svc = project_setup["svc"] + payload_dict = _project_payload(uuid4().hex[:6]).model_dump() + payload_dict["git_url"] = "https://github.com/owner/roboco.git" + with pytest.raises(ValidationError): + await svc.create(ProjectCreate(**payload_dict), project_setup["creator_id"]) + + @pytest.mark.asyncio async def test_get_returns_project(project_setup: dict) -> None: svc = project_setup["svc"] diff --git a/tests/integration/test_task_service_basics.py b/tests/integration/test_task_service_basics.py index 7fbf4408..39ef8b60 100644 --- a/tests/integration/test_task_service_basics.py +++ b/tests/integration/test_task_service_basics.py @@ -1246,6 +1246,24 @@ async def test_unclaim_for_agent_releases_claim( assert result.status == TaskStatus.PENDING +@pytest.mark.asyncio +async def test_unclaim_for_agent_releases_blocked_to_pool( + task_setup: dict, db_session: AsyncSession +) -> None: + """An agent trapped on a `blocked` task can release it back to the pool + (returns to pending, assignment cleared) instead of churning with no move.""" + svc = task_setup["svc"] + task = await svc.create(_req(task_setup)) + task.status = TaskStatus.BLOCKED + task.assigned_to = task_setup["agent_id"] + task.claimed_by = task_setup["agent_id"] + await db_session.flush() + result = await svc.unclaim_for_agent(task.id, agent_id=task_setup["agent_id"]) + assert result is not None + assert result.status == TaskStatus.PENDING + assert result.assigned_to is None + + @pytest.mark.asyncio async def test_unclaim_for_reaper_resets( task_setup: dict, db_session: AsyncSession diff --git a/tests/integration/test_tasks_routes.py b/tests/integration/test_tasks_routes.py index d30d9f3a..e9df0939 100644 --- a/tests/integration/test_tasks_routes.py +++ b/tests/integration/test_tasks_routes.py @@ -264,6 +264,23 @@ async def test_update_task(task_client: dict) -> None: assert response.status_code in (HTTPStatus.OK, HTTPStatus.UNPROCESSABLE_ENTITY) +@pytest.mark.asyncio +async def test_update_task_status_override_recovers_blocked(task_client: dict) -> None: + """A privileged PATCH with ``status`` is applied as an audited override, so an + operator can recover a task wedged in ``blocked`` (which ``/complete`` refuses) + instead of the status being silently dropped.""" + client = task_client["client"] + task = _seed_task(task_client, status=TaskStatus.BLOCKED) + await task_client["db"].flush() + response = await client.patch( + f"/api/tasks/{task.id}", + json={"status": "completed"}, + headers=_HDR, + ) + assert response.status_code == HTTPStatus.OK + assert response.json()["status"] == "completed" + + @pytest.mark.asyncio async def test_delete_task(task_client: dict) -> None: client = task_client["client"] diff --git a/tests/unit/gateway/test_auto_pause_checkpoint.py b/tests/unit/gateway/test_auto_pause_checkpoint.py index 566ac405..1df13d89 100644 --- a/tests/unit/gateway/test_auto_pause_checkpoint.py +++ b/tests/unit/gateway/test_auto_pause_checkpoint.py @@ -144,7 +144,10 @@ async def test_i_am_idle_with_commits_includes_last_three_in_remaining_work() -> agent_id = uuid4() task_id = uuid4() - commits = [MagicMock(sha=f"sha{i}") for i in range(5)] + # Production shape: task.commits is a JSON list[dict] keyed by `hash` + # (CommitRef.hash) — NOT `sha`. A prior fix read `sha` and silently lost + # every ref; this test uses the real shape so the regression can't hide. + commits = [{"hash": f"hash{i}", "message": f"c{i}"} for i in range(5)] task_obj = MagicMock() task_obj.id = task_id task_obj.status = "in_progress" @@ -163,12 +166,10 @@ async def test_i_am_idle_with_commits_includes_last_three_in_remaining_work() -> call_kwargs = task_svc.add_checkpoint.await_args remaining = call_kwargs.kwargs.get("remaining_work", []) - # Last 3 commit SHAs should appear somewhere in remaining_work entries - last_3_shas = {c.sha for c in commits[-3:]} - mentioned_shas = {entry for entry in remaining if isinstance(entry, str)} - assert last_3_shas & mentioned_shas or any( - sha in str(remaining) for sha in last_3_shas - ) + # Last 3 commit hashes (the real persisted key) must appear in remaining_work. + last_3 = {c["hash"] for c in commits[-3:]} + mentioned = {entry for entry in remaining if isinstance(entry, str)} + assert last_3 & mentioned or any(h in str(remaining) for h in last_3) @pytest.mark.asyncio diff --git a/tests/unit/runtime/test_blocker_and_claimed_dispatch.py b/tests/unit/runtime/test_blocker_and_claimed_dispatch.py index f2c86397..d3e1ce46 100644 --- a/tests/unit/runtime/test_blocker_and_claimed_dispatch.py +++ b/tests/unit/runtime/test_blocker_and_claimed_dispatch.py @@ -293,3 +293,24 @@ async def test_dispatch_claimed_without_agent_releases_unknown_without_spending_ expected_releases = 2 # both ghost claims released assert release.await_count == expected_releases spawn.assert_awaited_once() # then one known assignee respawned + + +@pytest.mark.asyncio +async def test_handle_dev_existing_owner_skips_blocked() -> None: + """A blocked task's owner is not respawned — it has no legal move from + blocked, so respawning it only churns; it waits for unblock or release.""" + orch = _orch() + orch._respawn_dev_if_inactive = AsyncMock() + orch._is_agent_active = MagicMock(return_value=False) + await orch._handle_dev_existing_owner({"id": "t1"}, "blocked", "be-dev-1") + orch._respawn_dev_if_inactive.assert_not_called() + + +@pytest.mark.asyncio +async def test_handle_dev_existing_owner_respawns_in_progress() -> None: + """An in_progress task whose owner is inactive is still respawned.""" + orch = _orch() + orch._respawn_dev_if_inactive = AsyncMock() + orch._is_agent_active = MagicMock(return_value=False) + await orch._handle_dev_existing_owner({"id": "t1"}, "in_progress", "be-dev-1") + orch._respawn_dev_if_inactive.assert_awaited_once() diff --git a/tests/unit/services/test_escalation_board_guard.py b/tests/unit/services/test_escalation_board_guard.py index 6f525821..60aeb0a9 100644 --- a/tests/unit/services/test_escalation_board_guard.py +++ b/tests/unit/services/test_escalation_board_guard.py @@ -18,7 +18,11 @@ from uuid import uuid4 import pytest from roboco.models.base import AgentRole, TaskStatus, TaskType, Team -from roboco.services.task import TaskService, _is_descendant_executable_task +from roboco.services.task import ( + TaskService, + _is_cell_team_task, + _is_descendant_executable_task, +) def _bind(svc: TaskService, name: str, value: object) -> None: @@ -41,6 +45,26 @@ def test_descendant_code_task_is_flagged() -> None: assert _is_descendant_executable_task(task) is True +def test_descendant_cell_team_task_is_flagged() -> None: + # A cell's own coordination task carries a cell team but a non-executable + # type; it must still not be handed to a board role on escalation. + task = MagicMock( + parent_task_id=uuid4(), team=Team.FRONTEND, task_type=TaskType.PLANNING + ) + assert _is_cell_team_task(task) is True + + +def test_root_cell_team_task_is_not_flagged() -> None: + # A root task can legitimately escalate up the chain (the CEO reviews it). + task = MagicMock(parent_task_id=None, team=Team.FRONTEND) + assert _is_cell_team_task(task) is False + + +def test_non_cell_team_task_is_not_flagged() -> None: + task = MagicMock(parent_task_id=uuid4(), team=Team.BOARD) + assert _is_cell_team_task(task) is False + + def test_descendant_documentation_task_is_flagged() -> None: # #14 broaden: documentation is cell-executed (documenter), not board work. task = MagicMock(parent_task_id=uuid4(), task_type=TaskType.DOCUMENTATION) @@ -369,3 +393,32 @@ async def test_apply_escalation_emits_blocked_audit_event() -> None: assert kwargs["event_type"] == "task.blocked" assert kwargs["details"]["from_status"] == "in_progress" assert kwargs["details"]["to_status"] == "blocked" + + +@pytest.mark.asyncio +async def test_unblock_with_restore_emits_audit_event() -> None: + """The PM restore path sets status directly (bypassing the validated + transition) and used to skip the audit log; it must record the transition.""" + svc = _service() + task = MagicMock( + id=uuid4(), + status=TaskStatus.BLOCKED, + pre_block_state="in_progress", + pre_block_assignee=None, + claimed_by=uuid4(), + ) + _bind(svc, "get", AsyncMock(return_value=task)) + audit_mock = MagicMock(log_task_event=AsyncMock()) + + with patch("roboco.services.audit.get_audit_service", return_value=audit_mock): + await svc.unblock_with_restore(uuid4(), uuid4(), restore=True) + pending = list(svc._background_tasks) + if pending: + await asyncio.gather(*pending, return_exceptions=True) + + assert task.status == TaskStatus.IN_PROGRESS + audit_mock.log_task_event.assert_awaited_once() + kwargs = audit_mock.log_task_event.await_args.kwargs + assert kwargs["event_type"] == "task.in_progress" + assert kwargs["details"]["from_status"] == "blocked" + assert kwargs["details"]["to_status"] == "in_progress" diff --git a/tests/unit/services/test_task.py b/tests/unit/services/test_task.py index 86f3dba1..96d8ce9b 100644 --- a/tests/unit/services/test_task.py +++ b/tests/unit/services/test_task.py @@ -706,8 +706,27 @@ def test_resolve_doc_abspath_keeps_plain_relative_path() -> None: def test_resolve_doc_abspath_passes_absolute_path_through() -> None: - """An already-absolute path is trusted as-is (no re-rooting).""" + """An absolute path already correctly rooted under the base is unchanged.""" assert ( TaskService._resolve_doc_abspath("/app/docs/design/spec.md") == "/app/docs/design/spec.md" ) + + +def test_resolve_doc_abspath_collapses_doubled_absolute_docs() -> None: + """An absolute path that doubled the base segment is collapsed. + + The documenter sometimes records `/app/docs/docs/...`; previously it was + returned verbatim and never resolved on disk (the recurring "Source not + found" warning). + """ + assert ( + TaskService._resolve_doc_abspath("/app/docs/docs/backend/api/prompter.md") + == "/app/docs/backend/api/prompter.md" + ) + + +def test_resolve_doc_abspath_leaves_external_absolute_path() -> None: + """An absolute path outside the docs root is left as-is for the indexer to skip.""" + external = "/data/workspaces/panel/frontend/fe-dev-1/src/page.tsx" + assert TaskService._resolve_doc_abspath(external) == external diff --git a/tests/unit/test_exceptions.py b/tests/unit/test_exceptions.py index f354f2a0..264b79e5 100644 --- a/tests/unit/test_exceptions.py +++ b/tests/unit/test_exceptions.py @@ -294,6 +294,29 @@ def test_git_command_error() -> None: assert err.details["command"] == "git push" +def test_git_command_error_surfaces_stderr_in_message() -> None: + err = GitCommandError( + command="push -u origin feature/x", + stderr="remote: Permission to owner/repo.git denied.\nfatal: unable to access", + ) + # The real reason must reach callers that only render ``.message``. + assert "Command failed: push -u origin feature/x" in err.message + assert "Permission to owner/repo.git denied" in err.message + + +def test_git_command_error_scrubs_credentials() -> None: + leaky = ( + "fatal: unable to access " + "'https://x-access-token:ghp_AbC123456789012345678901234567890@github.com/o/r.git'" + ) + err = GitCommandError(command="push", stderr=leaky) + # The injected PAT must never survive into the message, stderr, or details. + assert "ghp_" not in err.message + assert "ghp_" not in err.stderr + assert "ghp_" not in err.details["stderr"] + assert "https://***@github.com" in err.stderr + + def test_git_timeout_error() -> None: err = GitTimeoutError(command="git fetch", timeout=_TIMEOUT_SECONDS) assert err.command == "git fetch"