mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Merge 'master' into 'dogfood feature branch' (smoke test run) (#82)
* Fix: human surface lifecycle hardening (#81) * fix: harden agent-idle, redis loop, git errors, escalation audit - i_am_idle no longer 500s when auto-pausing a task whose commits are stored as dicts: tolerate dict-or-object commit refs and run the synthetic-checkpoint computation inside the swallowing try block. - The stream event loop no longer logs an idle redis read-timeout as an ERROR every cycle; the blocking-read timeout is treated as a normal idle. - Git command failures surface git's own (secret-scrubbed) stderr in the error message instead of a bare 'Command failed', so push/fetch rejections are diagnosable; the injected PAT is redacted. - The escalate-to-pool redirect emits the task.pending audit event, closing a status mutation that previously skipped the audit log. * fix: let privileged operators set task status via an audited override The task update route silently dropped a 'status' field in the request body, so a CEO/admin could not transition a task wedged in a state with no valid in-band move (e.g. a blocked task whose work merged out-of-band) — the panel returned 200 while nothing changed. Add 'status' to the update schema and apply it through a new audited 'admin_set_status' that bypasses the strict transition validator but always records the audit event. The override requires elevated permissions; ordinary field updates are unchanged. * fix: stop human chat sessions from expiring between messages Messaging sessions fell back to a hardcoded 300s idle timeout, shorter than a normal pause in a human conversation: the sweeper closed the session and the next message opened a new one, so a person could not hold a continuous chat. Make the idle timeout configurable (session_idle_timeout_seconds, default 3600) and resolve an unset timeout to it at every session-creation path instead of the 300s column fallback. * fix: resolve doubled doc paths and stop the indexer warning flood The doc-path resolver returned absolute paths verbatim, so a documenter path that doubled the base segment (/app/docs/docs/...) never resolved on disk and the docs never indexed into RAG. Reduce an absolute path under the docs base to a relative one before normalizing, leaving truly-external absolute paths for the indexer to skip. The indexer now skips non-markdown source files and logs a missing/non-doc source at debug instead of warning on every pass. * fix: reject project repo URLs that point at a protected repository Add a configurable denylist (protected_git_urls) enforced in the project create and update paths, so a project cannot be registered against a repository that must not receive agent commits or merges (e.g. the roboco source repo during a smoke run). Empty by default (no behavior change); operators set it to sandbox smoke-test projects. * fix: let an agent release a blocked task back to the pool A developer (or QA/doc) trapped on a 'blocked' task had no legal forward move — every verb rejected from that state — so the dispatcher kept respawning it with nothing to do. Allow 'unclaim' to release a blocked task the agent owns back to pending (assignment cleared, work session abandoned, audited), so the cell PM can re-delegate it instead of the agent churning. * fix: keep blocked-dev churn out and cell tasks out of board hands - The dispatcher no longer respawns the owner of a blocked task: from blocked the owner has no legal move, so respawning only churns; it is revived on unblock or released via unclaim. - Escalation no longer hands a cell (backend/frontend/ux_ui) coordination task to a board/advisory role — such an escalation is diverted to the cell pool, matching the existing executable-task guard. main_pm targets are unaffected. * chore: add an opt-in full clean-slate to the reset script FULL_RESET=1 wipes everything under the roboco data root except the persistent service stores (ollama/postgres/redis) and clears the persisted agent Claude session dirs (ROBOCO_CLAUDE_STATE_DIRS), which otherwise replay across runs. Default off — the existing DB/Redis wipe + workspace git-reset is unchanged. * ++ --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com> * fix: read the real commit key (hash) and audit the restore-unblock path - The auto-pause checkpoint and _extract_first_commit_sha read commit dicts by key 'sha', but persisted commits are keyed 'hash' (CommitRef.hash) — the prior change stopped the crash but silently dropped every ref. Read 'hash' (sha fallback) at both sites; the test now uses the production dict shape so the regression can't hide. - unblock_with_restore set status directly and skipped the audit log; emit the status-transition audit there too, like the other direct-set paths. --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user