mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Fix: agent workflow hardening (#70)
* 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>
This commit is contained in:
committed by
GitHub
co-authored by
Renn F
parent
97533f769b
commit
06682f33c6
@@ -77,13 +77,21 @@ async def fanout_setup(db_session: AsyncSession) -> AsyncIterator[dict]:
|
||||
assigned_cell=Team.UX_UI,
|
||||
created_by=system.id,
|
||||
)
|
||||
be_project = ProjectTable(
|
||||
id=uuid4(),
|
||||
name="BE",
|
||||
slug=f"be-{uuid4().hex[:6]}",
|
||||
git_url="https://example.com/be.git",
|
||||
assigned_cell=Team.BACKEND,
|
||||
created_by=system.id,
|
||||
)
|
||||
product = ProductTable(
|
||||
id=uuid4(),
|
||||
name="Prod",
|
||||
slug=f"prod-{uuid4().hex[:6]}",
|
||||
created_by=system.id,
|
||||
)
|
||||
db_session.add_all([fe_project, ux_project, product])
|
||||
db_session.add_all([fe_project, ux_project, be_project, product])
|
||||
await db_session.flush()
|
||||
|
||||
svc = TaskService(db_session)
|
||||
@@ -105,6 +113,7 @@ async def fanout_setup(db_session: AsyncSession) -> AsyncIterator[dict]:
|
||||
"fe_dev_id": fe_dev.id,
|
||||
"fe_project_id": fe_project.id,
|
||||
"ux_project_id": ux_project.id,
|
||||
"be_project_id": be_project.id,
|
||||
"product_id": product.id,
|
||||
}
|
||||
|
||||
@@ -224,3 +233,145 @@ async def test_dev_subtask_held_until_ux_dependency_resolves(
|
||||
assert dev_subtask.id in pending_after_ids, (
|
||||
"dev subtask must become dispatchable once UX reaches a terminal state"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_dependent_cell_sequence_follows_upstream_ux(
|
||||
fanout_setup: dict,
|
||||
) -> None:
|
||||
"""The frontend cell task sorts after its UX upstream: wiring the
|
||||
dependency also bumps its sequence to the UX task's sequence + 1, so
|
||||
list ordering and the panel show UX before the work it gates."""
|
||||
svc: TaskService = fanout_setup["svc"]
|
||||
tree = await _build_product_fanout(fanout_setup)
|
||||
ux_row = await svc.get(tree["ux_cell"].id)
|
||||
fe_row = await svc.get(tree["fe_cell"].id)
|
||||
assert ux_row is not None and fe_row is not None
|
||||
assert fe_row.sequence == (ux_row.sequence or 0) + 1, (
|
||||
"the dependent frontend task must sort one step after its UX upstream"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_backend_cell_also_depends_on_ux(fanout_setup: dict) -> None:
|
||||
"""UX/UI design defines the API contracts the backend builds against, so a
|
||||
backend cell task in the same fan-out also waits on the UX cell task and
|
||||
sorts after it."""
|
||||
svc: TaskService = fanout_setup["svc"]
|
||||
choreo: Choreographer = fanout_setup["choreo"]
|
||||
tree = await _build_product_fanout(fanout_setup)
|
||||
root = tree["root"]
|
||||
ux_cell = tree["ux_cell"]
|
||||
|
||||
be_cell = await svc.create_subtask(
|
||||
TaskCreateRequest(
|
||||
title="Backend implementation for the feature",
|
||||
description="a real backend cell task description over twenty chars",
|
||||
acceptance_criteria=["endpoints satisfy the contract"],
|
||||
team=Team.BACKEND,
|
||||
created_by=fanout_setup["creator"],
|
||||
project_id=fanout_setup["be_project_id"],
|
||||
product_id=fanout_setup["product_id"],
|
||||
parent_task_id=root.id,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.MEDIUM,
|
||||
)
|
||||
)
|
||||
# Forward order: the backend cell is delegated after the UX cell exists.
|
||||
await choreo._wire_ux_frontend_dependency(be_cell, root)
|
||||
await svc.session.flush()
|
||||
|
||||
be_row = await svc.get(be_cell.id)
|
||||
ux_row = await svc.get(ux_cell.id)
|
||||
assert be_row is not None and ux_row is not None
|
||||
assert ux_cell.id in be_row.dependency_ids, (
|
||||
"backend cell task must depend on the UX cell task"
|
||||
)
|
||||
assert be_row.sequence == (ux_row.sequence or 0) + 1, (
|
||||
"the backend task must sort one step after its UX upstream"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pending_impl_cells_retrowired_when_ux_arrives_later(
|
||||
fanout_setup: dict,
|
||||
) -> None:
|
||||
"""When the UX cell task is delegated AFTER still-pending frontend and
|
||||
backend siblings, both are retro-wired onto UX and sorted after it — the
|
||||
'either delegation order' guarantee, for both implementation cells."""
|
||||
svc: TaskService = fanout_setup["svc"]
|
||||
choreo: Choreographer = fanout_setup["choreo"]
|
||||
|
||||
root = await svc.create(
|
||||
TaskCreateRequest(
|
||||
title="Build the feature (board fan-out)",
|
||||
description="a real coordination task description over twenty chars",
|
||||
acceptance_criteria=["delegated to frontend + backend + ux_ui cells"],
|
||||
team=Team.BOARD,
|
||||
created_by=fanout_setup["creator"],
|
||||
project_id=None,
|
||||
product_id=fanout_setup["product_id"],
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.NON_TECHNICAL,
|
||||
estimated_complexity=Complexity.HIGH,
|
||||
)
|
||||
)
|
||||
fe_cell = await svc.create_subtask(
|
||||
TaskCreateRequest(
|
||||
title="Frontend implementation for the feature",
|
||||
description="a real frontend cell task description over twenty chars",
|
||||
acceptance_criteria=["UI matches the design"],
|
||||
team=Team.FRONTEND,
|
||||
created_by=fanout_setup["creator"],
|
||||
project_id=fanout_setup["fe_project_id"],
|
||||
product_id=fanout_setup["product_id"],
|
||||
parent_task_id=root.id,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.MEDIUM,
|
||||
)
|
||||
)
|
||||
be_cell = await svc.create_subtask(
|
||||
TaskCreateRequest(
|
||||
title="Backend implementation for the feature",
|
||||
description="a real backend cell task description over twenty chars",
|
||||
acceptance_criteria=["endpoints satisfy the contract"],
|
||||
team=Team.BACKEND,
|
||||
created_by=fanout_setup["creator"],
|
||||
project_id=fanout_setup["be_project_id"],
|
||||
product_id=fanout_setup["product_id"],
|
||||
parent_task_id=root.id,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.MEDIUM,
|
||||
)
|
||||
)
|
||||
# UX is delegated LAST — both pending implementation cells must be wired.
|
||||
ux_cell = await svc.create_subtask(
|
||||
TaskCreateRequest(
|
||||
title="UX/UI design for the feature",
|
||||
description="a real ux design task description over twenty chars",
|
||||
acceptance_criteria=["wireframes approved"],
|
||||
team=Team.UX_UI,
|
||||
created_by=fanout_setup["creator"],
|
||||
project_id=fanout_setup["ux_project_id"],
|
||||
product_id=fanout_setup["product_id"],
|
||||
parent_task_id=root.id,
|
||||
task_type=TaskType.DESIGN,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.MEDIUM,
|
||||
)
|
||||
)
|
||||
await choreo._wire_ux_frontend_dependency(ux_cell, root)
|
||||
await svc.session.flush()
|
||||
|
||||
fe_row = await svc.get(fe_cell.id)
|
||||
be_row = await svc.get(be_cell.id)
|
||||
ux_row = await svc.get(ux_cell.id)
|
||||
assert fe_row is not None and be_row is not None and ux_row is not None
|
||||
assert ux_cell.id in fe_row.dependency_ids, "frontend must retro-wire onto UX"
|
||||
assert ux_cell.id in be_row.dependency_ids, "backend must retro-wire onto UX"
|
||||
expected_sequence = (ux_row.sequence or 0) + 1
|
||||
assert fe_row.sequence == expected_sequence
|
||||
assert be_row.sequence == expected_sequence
|
||||
|
||||
@@ -120,6 +120,10 @@ class _StubGit:
|
||||
del branch_name, actor_agent_id
|
||||
return ("ok", 0)
|
||||
|
||||
async def push_task_branch(self, agent_id: UUID, task_id: UUID) -> int:
|
||||
del agent_id, task_id
|
||||
return 0
|
||||
|
||||
async def create_pr(
|
||||
self,
|
||||
branch_name: str,
|
||||
|
||||
@@ -124,6 +124,10 @@ class _StubGit:
|
||||
del branch_name, actor_agent_id
|
||||
return ("ok", 0)
|
||||
|
||||
async def push_task_branch(self, agent_id: UUID, task_id: UUID) -> int:
|
||||
del agent_id, task_id
|
||||
return 0
|
||||
|
||||
async def create_pr(
|
||||
self,
|
||||
branch_name: str,
|
||||
|
||||
@@ -322,3 +322,46 @@ async def test_all_three_dev_paths_gate_then_release(dep_gate_setup: dict) -> No
|
||||
assert (
|
||||
await choreo._run_claim_guards(agent_id=fe_dev_db_id, task=released) is None
|
||||
), "claim guard must allow once UX is terminal"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_claimed_dependency_blocked_task_is_released_to_pending(
|
||||
dep_gate_setup: dict,
|
||||
) -> None:
|
||||
"""A CLAIMED task whose dependency is unmet is released back to pending.
|
||||
|
||||
Unlike the pre-assigned-but-pending dev subtask, a cell task can reach
|
||||
CLAIMED with an unfinished dependency (the PM claims it before the upstream
|
||||
resolves). Left claimed, the orchestrator's respawn loop churns its
|
||||
assignee. The claim guard now releases it to pending — ``claimed -> blocked``
|
||||
is not a legal transition, so pending (held by the dependency filter) is the
|
||||
lifecycle-correct resting state, and ``_unblock_dependents`` re-dispatches it
|
||||
once the upstream completes.
|
||||
"""
|
||||
svc: TaskService = dep_gate_setup["svc"]
|
||||
choreo: Choreographer = dep_gate_setup["choreo"]
|
||||
fe_dev_db_id = dep_gate_setup["fe_dev_db_id"]
|
||||
|
||||
tree = await _seed_dev_subtask_with_unmet_dep(dep_gate_setup)
|
||||
dev_subtask = tree["dev_subtask"]
|
||||
|
||||
# Force the held task to CLAIMED (the state a respawn loop churns on).
|
||||
dev_subtask.status = TaskStatus.CLAIMED
|
||||
dev_subtask.branch_name = "feature/frontend/DEVLEAF01"
|
||||
await svc.session.flush()
|
||||
|
||||
held = await svc.get(dev_subtask.id)
|
||||
guard = await choreo._run_claim_guards(agent_id=fe_dev_db_id, task=held)
|
||||
assert guard is not None, "claim guard must still reject while UX is unmet"
|
||||
assert guard.error == "invalid_state"
|
||||
|
||||
after = await svc.get(dev_subtask.id)
|
||||
assert after is not None
|
||||
assert after.status == TaskStatus.PENDING, (
|
||||
"a claimed dependency-blocked task must be released to pending"
|
||||
)
|
||||
assert after.assigned_to is None, "release clears the assignee"
|
||||
assert after.branch_name is None, (
|
||||
"release clears branch_name so the re-claim cuts fresh off the current "
|
||||
"integration tip (which by then includes the upstream's work)"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user