mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
* fix(gateway): push the branch before QA handoff so reviewers see the latest commits The commit content tool commits locally without pushing; only open_pr pushed the branch. On the first submission that was fine, but a fix committed while addressing needs_revision never reached origin (open_pr is skipped once the PR exists), so QA — which reviews the remote PR branch — re-reviewed the stale remote and re-failed the task on every cycle, a loop that never converged. i_am_done now pushes the task branch (idempotent; a no-op when nothing is unpushed) as part of the shared submit gate, covering both the normal and resume-from-verifying paths. A push failure blocks the handoff with a clear remediation rather than parking the task in awaiting_qa with commits that exist only in the developer's local workspace. * fix(orchestrator): don't reap a stale claim while the agent's container is alive The stale-claim reaper released any claimed/in_progress task whose last_heartbeat_at exceeded the TTL. The heartbeat only updates on certain gateway calls, so a developer deep in a long edit/test cycle outran the TTL and had its claim reaped mid-work — churning the task and risking a double spawn against the still-running container. The reaper now skips a task whose assignee still holds a live (ACTIVE) agent instance, trusting container liveness — the ground truth — over the heartbeat proxy. The check is defensive on missing fields so a heartbeat-only caller (and the reaper's existing unit tests) behave exactly as before. * fix(gateway): refuse to unblock a task while a dependency is unfinished A PM unblock on a dependency-gated task moved it straight to in_progress, overriding the dependency — letting a dependent proceed without its upstream's work (e.g. a frontend task built before its UX design lands). A dependency block is meant to clear on its own via _unblock_dependents the moment the upstream reaches a terminal state. unblock now refuses while any dependency is still non-terminal, returning a clear remediation that the block resolves automatically. Manual unblock remains available for genuine, non-dependency blockers. * fix(gateway): release a dependency-blocked claim to pending instead of looping A task that reached claimed/in_progress with an unfinished dependency was left in that state when the claim guard rejected, so the orchestrator's respawn loop kept reviving its assignee — which could make no progress — burning work for nothing. The claim guard now releases such a task back to pending. claimed -> blocked is not a legal transition, so pending — held by the dispatch dependency filter — is the lifecycle-correct resting state: the respawn loop ignores pending tasks, and _unblock_dependents re-dispatches it once the upstream reaches a terminal state. release_dependency_blocked_claim shares a _force_unclaim_to_pending core with unclaim_for_reaper so both record a truthful work-session abandon reason. * feat(security): warn at startup in header-trust mode + document the auth posture When ROBOCO_AGENT_AUTH_REQUIRED is not enabled the API accepts the X-Agent-Id / X-Agent-Role headers without a signed token, so any client that can reach it may act as any role (including 'ceo'). The API now logs a clear warning at startup in this mode, and the README gains a Security section documenting the auth posture and how to harden it. Acceptable only on a trusted private network — do not expose the API to untrusted networks. * fix(workspace): scope the refresh fetch to current + default branch ensure_workspace's healthy short-circuit ran an all-refs 'git fetch origin' to keep every origin/<branch> ref current. On a monorepo with many accumulated feature/* branches that exceeds the refresh timeout, the fetch silently fails, and the workspace keeps a stale base — so an agent builds on an out-of-date branch. The refresh now fetches only the workspace's current branch and the repo's default branch (resolved via origin/HEAD), with --no-tags --prune: it transfers near-nothing and can't time out. Readers need their own branch and the default; the integration branch is refreshed at branch-creation time. * fix(git): refresh a dependency-blocked task's branch off the current integration tip A cross-cell dependent (e.g. a frontend task waiting on the UX design) was branched off a base captured before its upstream merged into the integration branch, and the branch was never re-synced — so the agent built on a stale snapshot with none of the upstream's work. Two changes close the gap: - release_dependency_blocked_claim now clears branch_name, so the re-claim (after the dependency clears) re-runs branch creation. - create_branch, when the branch is already on disk with no commits of its own, resets it onto the freshly-pulled base — the dependent now builds on the current integration tip. A branch carrying real commits is left untouched, so no work is discarded; the cell->leaf cascade carries the upstream down to the dev branch automatically. * refactor(gateway): drop the sibling-sequence claim guard Sibling sequence no longer gates a claim. Cross-cell ordering is enforced by task dependencies — a cell task that depends on another is held until its upstream reaches a terminal state, a stronger, status-aware gate than the sequence-number check. That check was dormant in practice anyway: every fan-out child carries sequence 0, on which the guard short-circuited. `sequence` stays a sibling-ordering / dispatch-priority field (list_pending ordering and the panel). Removes sibling_sequence_guard and its _earlier_blocking_sibling helper, the now-unused skip_sequence parameter threaded through the claim verbs, and the sibling fetch that fed it. * feat(gateway): sort a cross-cell dependent after its upstream When the frontend cell task is wired to depend on its UX/UI sibling, set its sequence to the upstream's sequence + 1 so it sorts after the design it waits on — list_pending ordering and the panel now show UX ahead of the implementation it gates, in either delegation order. Adds TaskService.set_sequence (the sibling-ordering field is a service write; it carries no claim-gating semantics — dependencies gate claims). * feat(gateway): make the backend cell depend on UX too UX/UI design defines the screens and API contracts both implementation cells build against, so the backend cell — not just the frontend — waits on the UX/UI cell task in a product fan-out and sorts after it. Wires in either delegation order: a backend task delegated after UX gets the dependency directly; a UX task delegated after a still-pending backend sibling retro-wires it. Mirrors the existing frontend wiring (_depend_backend_on_ux and _depend_pending_backends_on_ux). Backend is held by the same dependency gate, so it costs no extra dispatch churn. * fix(websocket): forward notification acks instead of logging them incomplete The bridge handler serves both notification.sent and notification.acked, but acked events carry `agent_id` (the acking agent) rather than `recipient_id`, so every acknowledgement tripped the missing-field guard and logged "Incomplete notification event" instead of reaching the panel. Accept either field as the recipient. * feat(api): hint the full UUID when a truncated task id fails validation Agents copy the 8-character task prefix the system shows them (the commit prefix, task summaries) and send it as task_id, which fails UUID validation with an opaque "invalid length" 422 and wastes a call. The request-validation handler now detects a task_id UUID error and attaches a `remediate` hint telling the agent to retry with the full 36-character UUID from its task envelope. * fix(audit): record the blocked transition when a task is escalated Escalation sets a task to blocked by writing task.status directly, which bypassed the validated transition helper and so never emitted a task.blocked audit row — the lifecycle moved but the Auditor saw nothing. Extract the audit emit from the central transition helper into _emit_status_transition_audit and call it from the escalate path, capturing the prior status and outgoing owner before reassignment so the row is attributed correctly. * fix(docs): stop doubling the docs path so design specs index into RAG The documenter sometimes hands a doc path already rooted at docs/, and joining it onto DOCS_BASE_PATH (/app/docs) produced /app/docs/docs/..., so the file was never found and the spec never indexed — the frontend cell could not retrieve the UX design over RAG. Normalize the path before joining: trust an absolute path, otherwise strip a single redundant leading docs/ segment. * feat(security): let the control panel authenticate in secure mode With ROBOCO_AGENT_AUTH_REQUIRED=true every request must carry a valid HMAC token, which locked the human control panel out — it sends role headers but no token. nginx, the only trusted hop between the browser and the API, now injects the CEO token on /api and /ws, so the browser never holds the signing secret. The injected value is just the existing per-agent token issued for the CEO identity (issue_panel_token), so the token-verification path is unchanged. An empty value (dev/header-trust mode) renders to no header. `make panel-token` prints the value; set it as ROBOCO_PANEL_AGENT_TOKEN in .env before enabling secure mode. .env.example and the README Security section document the flow. * chore(compose): consolidate the two compose files into one docker-compose.yml and docker-compose.yaml had diverged: .yml — the file Docker actually uses — carried ROBOCO_PUBLIC_BASE_URL but was missing the /app/manifests bind-mount, while .yaml had the manifests mount but not the base URL. Merge the union into docker-compose.yml and delete the duplicate so there is one source of truth and no "multiple config files" warning. This activates the manifests mount in the deployed file: without it the orchestrator writes per-agent tool manifests to its ephemeral container fs, they never reach the host for the daemon to bind-mount, and agents fall back to all-verbs registration. Drop the stale .yaml reference from the config.py docstring, the labeler, and the CI path filters. --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
277 lines
9.0 KiB
Python
277 lines
9.0 KiB
Python
"""api.middleware coverage — pure-function status mapping + handlers."""
|
|
|
|
from __future__ import annotations
|
|
|
|
from http import HTTPStatus
|
|
|
|
# UUID annotates a Pydantic model field below, so it must stay a runtime import
|
|
# (Pydantic resolves the annotation when building the model) despite `from
|
|
# __future__ import annotations` making it look type-checking-only to ruff.
|
|
from uuid import UUID # noqa: TC003
|
|
|
|
from fastapi import FastAPI, HTTPException
|
|
from fastapi.testclient import TestClient
|
|
from pydantic import BaseModel
|
|
from roboco.api.middleware import (
|
|
_uuid_field_remediation,
|
|
get_status_code,
|
|
setup_middleware,
|
|
)
|
|
from roboco.exceptions import (
|
|
AuthenticationError,
|
|
InvalidStateError,
|
|
NotFoundError,
|
|
PermissionDeniedError,
|
|
RobocoError,
|
|
ValidationError,
|
|
)
|
|
from roboco.services.base import (
|
|
ConflictError as ServiceConflictError,
|
|
)
|
|
from roboco.services.base import (
|
|
NotFoundError as ServiceNotFoundError,
|
|
)
|
|
from roboco.services.base import (
|
|
UnauthorizedError as ServiceUnauthorizedError,
|
|
)
|
|
from roboco.services.base import (
|
|
ValidationError as ServiceValidationError,
|
|
)
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# get_status_code
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_get_status_code_for_not_found() -> None:
|
|
assert get_status_code(NotFoundError("Task", "abc")) == HTTPStatus.NOT_FOUND
|
|
|
|
|
|
def test_get_status_code_for_validation() -> None:
|
|
assert get_status_code(ValidationError("x")) == HTTPStatus.UNPROCESSABLE_ENTITY
|
|
|
|
|
|
def test_get_status_code_for_invalid_state() -> None:
|
|
assert (
|
|
get_status_code(InvalidStateError("pending", "complete")) == HTTPStatus.CONFLICT
|
|
)
|
|
|
|
|
|
def test_get_status_code_for_permission() -> None:
|
|
assert get_status_code(PermissionDeniedError("x")) == HTTPStatus.FORBIDDEN
|
|
|
|
|
|
def test_get_status_code_for_auth() -> None:
|
|
assert get_status_code(AuthenticationError("x")) == HTTPStatus.UNAUTHORIZED
|
|
|
|
|
|
def test_get_status_code_for_generic() -> None:
|
|
"""Unknown RobocoError subclass defaults to 400."""
|
|
assert get_status_code(RobocoError("x", code="other")) == HTTPStatus.BAD_REQUEST
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Middleware integration via TestClient
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _make_app() -> FastAPI:
|
|
app = FastAPI()
|
|
|
|
@app.get("/ok")
|
|
async def _ok():
|
|
return {"status": "ok"}
|
|
|
|
@app.get("/raise")
|
|
async def _raise():
|
|
raise RuntimeError("boom")
|
|
|
|
@app.get("/notfound")
|
|
async def _nf():
|
|
raise NotFoundError("Resource", "abc")
|
|
|
|
@app.get("/http-error")
|
|
async def _he():
|
|
raise HTTPException(status_code=403, detail="nope")
|
|
|
|
# service-layer errors (parallel hierarchy from roboco.services.base)
|
|
@app.get("/svc-notfound")
|
|
async def _svc_nf():
|
|
raise ServiceNotFoundError("Channel", "main-pm")
|
|
|
|
@app.get("/svc-validation")
|
|
async def _svc_v():
|
|
raise ServiceValidationError("invalid input", field="title")
|
|
|
|
@app.get("/svc-conflict")
|
|
async def _svc_c():
|
|
raise ServiceConflictError("duplicate", resource_type="task")
|
|
|
|
@app.get("/svc-unauth")
|
|
async def _svc_u():
|
|
raise ServiceUnauthorizedError("merge_pr", reason="not your PR")
|
|
|
|
setup_middleware(app)
|
|
return app
|
|
|
|
|
|
def test_middleware_adds_correlation_id_header() -> None:
|
|
client = TestClient(_make_app())
|
|
response = client.get("/ok")
|
|
assert response.status_code == HTTPStatus.OK
|
|
assert "X-Correlation-ID" in response.headers
|
|
|
|
|
|
def test_middleware_uses_provided_correlation_id() -> None:
|
|
client = TestClient(_make_app())
|
|
cid = "test-correlation-12345"
|
|
response = client.get("/ok", headers={"X-Correlation-ID": cid})
|
|
assert response.headers["X-Correlation-ID"] == cid
|
|
|
|
|
|
def test_middleware_adds_response_time_header() -> None:
|
|
client = TestClient(_make_app())
|
|
response = client.get("/ok")
|
|
assert "X-Response-Time-Ms" in response.headers
|
|
|
|
|
|
def test_roboco_exception_translates_to_404() -> None:
|
|
client = TestClient(_make_app(), raise_server_exceptions=False)
|
|
response = client.get("/notfound")
|
|
assert response.status_code == HTTPStatus.NOT_FOUND
|
|
|
|
|
|
def test_http_exception_handler_returns_standardized_format() -> None:
|
|
client = TestClient(_make_app(), raise_server_exceptions=False)
|
|
response = client.get("/http-error")
|
|
assert response.status_code == HTTPStatus.FORBIDDEN
|
|
body = response.json()
|
|
assert "error" in body
|
|
|
|
|
|
# `roboco.services.base.ServiceError` is a parallel exception hierarchy
|
|
# (it does NOT inherit from RobocoError), so a separate handler maps it
|
|
# to clean 4xx codes instead of letting the generic 500 handler eat it.
|
|
|
|
|
|
def test_service_notfound_translates_to_404() -> None:
|
|
client = TestClient(_make_app(), raise_server_exceptions=False)
|
|
response = client.get("/svc-notfound")
|
|
assert response.status_code == HTTPStatus.NOT_FOUND
|
|
body = response.json()
|
|
assert body["error"] == "NotFoundError"
|
|
assert "main-pm" in body["message"]
|
|
|
|
|
|
def test_service_validation_translates_to_422() -> None:
|
|
client = TestClient(_make_app(), raise_server_exceptions=False)
|
|
response = client.get("/svc-validation")
|
|
assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY
|
|
body = response.json()
|
|
assert body["error"] == "ValidationError"
|
|
|
|
|
|
def test_service_conflict_translates_to_409() -> None:
|
|
client = TestClient(_make_app(), raise_server_exceptions=False)
|
|
response = client.get("/svc-conflict")
|
|
assert response.status_code == HTTPStatus.CONFLICT
|
|
|
|
|
|
def test_service_unauthorized_translates_to_403() -> None:
|
|
client = TestClient(_make_app(), raise_server_exceptions=False)
|
|
response = client.get("/svc-unauth")
|
|
assert response.status_code == HTTPStatus.FORBIDDEN
|
|
|
|
|
|
def test_service_handler_carries_correlation_id() -> None:
|
|
client = TestClient(_make_app(), raise_server_exceptions=False)
|
|
cid = "test-svc-correlation-987"
|
|
response = client.get("/svc-notfound", headers={"X-Correlation-ID": cid})
|
|
body = response.json()
|
|
assert body["details"]["correlation_id"] == cid
|
|
|
|
|
|
def test_generic_exception_returns_500() -> None:
|
|
client = TestClient(_make_app(), raise_server_exceptions=False)
|
|
response = client.get("/raise")
|
|
assert response.status_code == HTTPStatus.INTERNAL_SERVER_ERROR
|
|
body = response.json()
|
|
assert "error" in body
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# _uuid_field_remediation + truncated-task_id 422 remediation
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def test_uuid_field_remediation_hits_truncated_task_id() -> None:
|
|
errors = [{"loc": ("body", "task_id"), "type": "uuid_parsing", "msg": "bad"}]
|
|
hint = _uuid_field_remediation(errors)
|
|
assert hint is not None
|
|
assert "full" in hint.lower()
|
|
assert "uuid" in hint.lower()
|
|
|
|
|
|
def test_uuid_field_remediation_ignores_other_field_errors() -> None:
|
|
errors = [{"loc": ("body", "title"), "type": "string_too_short", "msg": "x"}]
|
|
assert _uuid_field_remediation(errors) is None
|
|
|
|
|
|
def test_uuid_field_remediation_ignores_non_uuid_task_id_errors() -> None:
|
|
errors = [{"loc": ("body", "task_id"), "type": "missing", "msg": "required"}]
|
|
assert _uuid_field_remediation(errors) is None
|
|
|
|
|
|
class _TaskIdBody(BaseModel):
|
|
task_id: UUID
|
|
|
|
|
|
def _make_uuid_app() -> FastAPI:
|
|
app = FastAPI()
|
|
|
|
@app.post("/needs-uuid")
|
|
async def _need(body: _TaskIdBody) -> dict:
|
|
return {"task_id": str(body.task_id)}
|
|
|
|
setup_middleware(app)
|
|
return app
|
|
|
|
|
|
def test_truncated_task_id_422_carries_remediation() -> None:
|
|
"""An 8-char task_id (the recurring agent mistake) returns 422 + remediate."""
|
|
client = TestClient(_make_uuid_app(), raise_server_exceptions=False)
|
|
response = client.post("/needs-uuid", json={"task_id": "cee99ecc"})
|
|
assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY
|
|
body = response.json()
|
|
assert "remediate" in body
|
|
assert "full" in body["remediate"].lower()
|
|
|
|
|
|
def test_other_validation_422_omits_remediation() -> None:
|
|
"""A non-task_id validation error keeps the standard 422 shape (no remediate)."""
|
|
client = TestClient(_make_uuid_app(), raise_server_exceptions=False)
|
|
response = client.post("/needs-uuid", json={}) # missing task_id entirely
|
|
assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY
|
|
assert "remediate" not in response.json()
|
|
|
|
|
|
def test_request_validation_handler_returns_422_with_details() -> None:
|
|
"""request_validation_handler logs + returns 422 with errors+body (251-260)."""
|
|
|
|
class _Body(BaseModel):
|
|
name: str
|
|
|
|
app = FastAPI()
|
|
setup_middleware(app)
|
|
|
|
@app.post("/validate")
|
|
async def _v(_data: _Body):
|
|
return {"ok": True}
|
|
|
|
client = TestClient(app, raise_server_exceptions=False)
|
|
response = client.post("/validate", json={"wrong_field": "x"})
|
|
assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY
|
|
body = response.json()
|
|
assert "detail" in body
|
|
assert "body" in body
|