mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(run-hardening): park the workforce on a session-limit + PR-review verdict colour (#249)
* fix(orchestrator): park the provider on a Claude session-limit 429, not crash-loop
When the org Claude usage ("5-hour") session limit is hit, an agent container
exits non-zero with a 0-token 429 rejection. The provider-unavailable break only
recognized 5xx overload signatures (529/500/503), so a session-limit crash fell
through to the normal crash-retry path — the orchestrator respawned the agent
straight back into the limit, fleet-wide, until the window reset.
Add a sibling detector _provider_rate_limit_park_target that matches the
session-limit markers ("hit your session limit", "five_hour") in the dead
container's output and parks the provider with kind="rate_limited" (a longer
probe cadence), checked before the overload path in _handle_stopped_container.
Reuses the existing park-and-probe machinery, so the background probe loop
revives the parked tasks when the quota resets — no churn. Gated by the same
overload_break_enabled flag.
Also backfills the CHANGELOG Fixed entry for the orchestrator self-call auth fix
(merged in #248 without one).
* fix(panel): PR Reviewer Notes card colour reflects the verdict
The card was hardcoded teal/green regardless of the review verdict, so a Failed
review sat inside a green card and read as passing at a glance. Derive the card
background from the verdict (red on failed, green on approved/passed, amber on
changes-requested, neutral teal before a verdict) — mirroring the QA Notes card.
---------
Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -10,6 +10,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
|
||||
|
||||
- **MegaTask — describe several tasks in one intake chat and ship them as one sequenced batch.** When the CEO wants several pieces of work at once — even across projects that don't share a codebase (e.g. a SaaS app, its open-source core engine, and a framework adapter) — the intake modal now offers a third scope, **MegaTask**, beside Single cell and Board-led. You pick the repos it spans; the intake agent reads them all and proposes the whole batch in one hand-off (the new `propose_batch` tool), one draft per task, each carrying its own project plus a collision surface (which files it touches, whether it adds a migration, whether it edits a widely-shared component). A deterministic analyzer (`SequencingService`) turns those surfaces into conflict-free **waves** — file-overlap and migration-adding tasks are serialized, a shared-surface edit runs after what it overlaps, independent tasks run in parallel — and the Board reviews the batch once. On confirm RoboCo creates a branchless **umbrella** task (the Main PM's coordination + board-review + CEO-approve unit) over N **root-subtasks**, each a real coordination root with its own project, branch, and PR, wired with the analyzer's dependencies so the existing dependency-gate dispatches the waves in order. The umbrella assembles no PR of its own, is exempt from the branch gate, and completes only when every root-subtask is terminal (then it escalates to the CEO). On the Board route the root-subtasks are held until the umbrella is approved, then released. Surfaced as a core capability — no feature flag — branded "MegaTask" across the panel, prompts, and docs; internal names stay technical (`batch_id`, `SequencingService`). Adds `tasks.batch_id` + the three collision-surface columns (migration 046), `confirm_live_batch` + `POST /prompter/live/{session}/confirm-batch`, multi-project intake spawn (`project_ids`), the `propose_batch` tool on both intake runtimes (Claude SDK driver + grok CLI server), and the panel's MegaTask scope + Review-MegaTask card.
|
||||
|
||||
### Fixed
|
||||
|
||||
- **The orchestrator's own recovery actions now actually run.** Its background dispatcher made internal HTTP calls to its own API without an agent identity, so every self-`PATCH` to a task — auto-blocking a task with missing prerequisites, auto-resuming a PM's paused parent, auto-recovering a stale-blocked parent, annotating an SLA breach — was rejected with `401 Missing X-Agent-ID` and silently dropped. The visible effect was paused/blocked parent tasks staying wedged and their dependent work stranded (with the dispatcher logging a "respawning assignee" loop). Header propagation was inconsistent across the orchestrator's separate HTTP-client call-sites — only the main dispatch loop sent the identity. The system identity is now hoisted into one shared constant and applied to every API-facing dispatcher client (the external provider-recovery probe is intentionally excluded); the `system` role holds the permission required for the audited status-override path those routes use.
|
||||
|
||||
- **Hitting the Claude session limit now parks the workforce instead of crash-looping it.** When the org's Claude usage ("5-hour") limit is reached, each agent container exits with a 429 rejection; the orchestrator was treating that like any crash and immediately respawning the agent straight back into the limit, over and over, across the whole fleet. It already parks the provider on a persistent server *overload* (529/500/503) and revives the parked work once it recovers — but that detection only matched the overload signatures, not the session-limit 429. The same park-and-resume break now also recognizes the session limit: the provider is parked, dispatch goes quiet, and the background probe loop brings the agents back automatically when the window resets — no churn, no wasted respawns.
|
||||
|
||||
- **A failed PR review no longer looks green.** On a task's detail page, the "PR Reviewer Notes" card was painted a fixed teal/green background regardless of the review verdict, so a `Failed` review — red badge and all — sat inside a green card and could read as passing at a glance. The card background now mirrors the verdict the way the QA Notes card already does: red on a failed review, green on approved/passed, amber on changes-requested, and neutral before a verdict is in.
|
||||
|
||||
## [0.10.0] - 2026-06-23
|
||||
|
||||
### Added
|
||||
|
||||
@@ -61,6 +61,30 @@ function prReviewBadge(task: Task): React.ReactNode {
|
||||
return <Badge className={`ml-2 ${v.cls} text-white`}>{v.label}</Badge>;
|
||||
}
|
||||
|
||||
// The card background mirrors the PR reviewer's verdict, so a FAILED review reads
|
||||
// as red — not the neutral teal that made a failure look green/passing at a glance.
|
||||
function prReviewCardBg(task: Task): string {
|
||||
const verdict = (
|
||||
task.notes_structured as
|
||||
| { pr_review?: { verdict?: string } }
|
||||
| null
|
||||
| undefined
|
||||
)?.pr_review?.verdict;
|
||||
const map: Record<string, string> = {
|
||||
approved:
|
||||
"bg-green-50 dark:bg-green-950 border border-green-200 dark:border-green-800",
|
||||
passed:
|
||||
"bg-green-50 dark:bg-green-950 border border-green-200 dark:border-green-800",
|
||||
changes_requested:
|
||||
"bg-amber-50 dark:bg-amber-950 border border-amber-200 dark:border-amber-800",
|
||||
failed: "bg-red-50 dark:bg-red-950 border border-red-200 dark:border-red-800",
|
||||
};
|
||||
return (
|
||||
(verdict ? map[verdict] : undefined) ??
|
||||
"bg-teal-50 dark:bg-teal-950 border border-teal-200 dark:border-teal-800"
|
||||
);
|
||||
}
|
||||
|
||||
interface NoteCardProps {
|
||||
task: Task;
|
||||
field: NoteField;
|
||||
@@ -332,7 +356,7 @@ export function TabNotes({ task }: TabNotesProps) {
|
||||
title="PR Reviewer Notes"
|
||||
icon={<GitPullRequest className="h-5 w-5" />}
|
||||
badge={prReviewBadge(task)}
|
||||
bgClass="bg-teal-50 dark:bg-teal-950 border border-teal-200 dark:border-teal-800"
|
||||
bgClass={prReviewCardBg(task)}
|
||||
/>
|
||||
|
||||
{/* Auditor Notes */}
|
||||
|
||||
@@ -124,6 +124,22 @@ _ANTHROPIC_OVERLOAD_MARKERS: tuple[str, ...] = (
|
||||
"error 503",
|
||||
)
|
||||
|
||||
# Session / usage-limit parking (HTTP 429). The Claude session ("5-hour") limit
|
||||
# crashes the agent container with a 0-token rejection that is NOT a 5xx
|
||||
# overload, so without its own markers it falls through to crash-respawn —
|
||||
# straight back into the limit until the window resets. Park the provider like a
|
||||
# 429 instead and let the probe-resume loop revive the parked tasks once the
|
||||
# quota clears. Markers are specific to how the session limit surfaces (matched
|
||||
# lowercased, substring) so they can't false-match an agent writing about
|
||||
# limits; the probe (which also hits the same limit) keeps the park until reset.
|
||||
# Reuses the longer overload retry cadence — probing a multi-hour window every
|
||||
# few seconds is wasteful, and each probe is itself a rejected call.
|
||||
_RATE_LIMIT_RETRY_AFTER_S = 300.0
|
||||
_ANTHROPIC_RATE_LIMIT_MARKERS: tuple[str, ...] = (
|
||||
"hit your session limit",
|
||||
"five_hour",
|
||||
)
|
||||
|
||||
# The intake (prompter) agent: a single seeded, board-adjacent interviewer.
|
||||
# Unlike delivery agents it is never dispatched and runs ONE persistent
|
||||
# container at a time (single CEO → one live chat). See the INTAKE section
|
||||
@@ -5220,6 +5236,30 @@ Start by:
|
||||
await self._park_grok_rate_limited(agent_id, instance)
|
||||
return
|
||||
graceful = exit_code == 0
|
||||
# Session/usage-limit parking: the Claude session ("5-hour") limit is a
|
||||
# 429 the SDK does not retry — the container exits non-zero with a
|
||||
# 0-token rejection. Detect it in the dead container's output and park
|
||||
# the provider (instead of crash-respawning straight back into the
|
||||
# limit); the probe-resume loop revives the task when the quota resets.
|
||||
if not graceful:
|
||||
rate_limited_provider = await self._provider_rate_limit_park_target(
|
||||
agent_id, instance
|
||||
)
|
||||
if rate_limited_provider is not None:
|
||||
logger.warning(
|
||||
"Session/usage limit detected in agent output; parking provider",
|
||||
agent_id=agent_id,
|
||||
provider=rate_limited_provider,
|
||||
task_id=instance.current_task_id,
|
||||
)
|
||||
await self._park_provider_unavailable(
|
||||
agent_id,
|
||||
instance,
|
||||
provider=rate_limited_provider,
|
||||
retry_after=_RATE_LIMIT_RETRY_AFTER_S,
|
||||
kind="rate_limited",
|
||||
)
|
||||
return
|
||||
# Server-overload parking: a persistent 529/500/503 from the model API
|
||||
# kills the run (the SDK already retries transient ones). Detect the
|
||||
# overload marker in the dead container's output and park the provider —
|
||||
@@ -5874,6 +5914,30 @@ Start by:
|
||||
return ModelProvider.ANTHROPIC.value
|
||||
return None
|
||||
|
||||
async def _provider_rate_limit_park_target(
|
||||
self, agent_id: str, instance: Any
|
||||
) -> str | None:
|
||||
"""Provider to park if this dead run hit a session/usage limit, else None.
|
||||
|
||||
Mirrors ``_provider_overload_park_target`` but matches the Claude session
|
||||
("5-hour") limit, which surfaces as a 429 the SDK does not retry — the
|
||||
container exits with a 0-token rejection rather than an overload. Without
|
||||
this it would crash-respawn straight back into the limit. Gated by the
|
||||
same flag so a misfire is toggle-able without a redeploy.
|
||||
"""
|
||||
if not settings.overload_break_enabled:
|
||||
return None
|
||||
from roboco.models.base import ModelProvider
|
||||
|
||||
provider_type = instance.config.provider_type if instance.config else None
|
||||
if provider_type not in (None, ModelProvider.ANTHROPIC.value):
|
||||
return None
|
||||
tail = await self._tail_container_logs(f"roboco-agent-{agent_id}")
|
||||
lowered = tail.lower()
|
||||
if any(marker in lowered for marker in _ANTHROPIC_RATE_LIMIT_MARKERS):
|
||||
return ModelProvider.ANTHROPIC.value
|
||||
return None
|
||||
|
||||
async def _park_provider_unavailable(
|
||||
self,
|
||||
agent_id: str,
|
||||
|
||||
@@ -16,6 +16,7 @@ from roboco.config import settings
|
||||
from roboco.models.runtime import AgentInstance
|
||||
from roboco.runtime.orchestrator import (
|
||||
_OVERLOAD_RETRY_AFTER_S,
|
||||
_RATE_LIMIT_RETRY_AFTER_S,
|
||||
AgentOrchestrator,
|
||||
AgentState,
|
||||
)
|
||||
@@ -25,6 +26,11 @@ _OVERLOAD_LOG = (
|
||||
'"message":"Overloaded"}}'
|
||||
)
|
||||
_CLEAN_LOG = "be-dev-1 finished editing src/app.py; all checks passed"
|
||||
_SESSION_LIMIT_LOG = (
|
||||
'{"type":"result","is_error":true,"api_error_status":429,'
|
||||
'"result":"You have hit your session limit - resets 1am (UTC)",'
|
||||
'"rate_limit_info":{"rateLimitType":"five_hour"}}'
|
||||
)
|
||||
|
||||
|
||||
def _instance(provider_type: str | None = "anthropic") -> AgentInstance:
|
||||
@@ -155,6 +161,9 @@ async def test_stopped_container_parks_on_overload(
|
||||
park = AsyncMock()
|
||||
spawn = AsyncMock()
|
||||
monkeypatch.setattr(orch, "_is_grok_rate_limit_exit", lambda _i, _e: False)
|
||||
monkeypatch.setattr(
|
||||
orch, "_provider_rate_limit_park_target", AsyncMock(return_value=None)
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
orch, "_provider_overload_park_target", AsyncMock(return_value="anthropic")
|
||||
)
|
||||
@@ -182,6 +191,9 @@ async def test_stopped_container_crash_retries_when_not_overload(
|
||||
inst.error_count = 0
|
||||
spawn = AsyncMock()
|
||||
monkeypatch.setattr(orch, "_is_grok_rate_limit_exit", lambda _i, _e: False)
|
||||
monkeypatch.setattr(
|
||||
orch, "_provider_rate_limit_park_target", AsyncMock(return_value=None)
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
orch, "_provider_overload_park_target", AsyncMock(return_value=None)
|
||||
)
|
||||
@@ -192,3 +204,74 @@ async def test_stopped_container_crash_retries_when_not_overload(
|
||||
|
||||
# Not an overload → the normal crash-retry path runs.
|
||||
spawn.assert_awaited_once()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Session/usage-limit (429) parking — the same break for a different signal
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_detects_session_limit_marker_for_anthropic(
|
||||
orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
monkeypatch.setattr(settings, "overload_break_enabled", True)
|
||||
monkeypatch.setattr(
|
||||
orch, "_tail_container_logs", AsyncMock(return_value=_SESSION_LIMIT_LOG)
|
||||
)
|
||||
assert (
|
||||
await orch._provider_rate_limit_park_target("be-dev-1", _instance())
|
||||
== "anthropic"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_clean_output_is_not_a_session_limit(
|
||||
orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
monkeypatch.setattr(settings, "overload_break_enabled", True)
|
||||
monkeypatch.setattr(
|
||||
orch, "_tail_container_logs", AsyncMock(return_value=_CLEAN_LOG)
|
||||
)
|
||||
assert await orch._provider_rate_limit_park_target("be-dev-1", _instance()) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_session_limit_disabled_flag_never_parks(
|
||||
orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
monkeypatch.setattr(settings, "overload_break_enabled", False)
|
||||
tail = AsyncMock(return_value=_SESSION_LIMIT_LOG)
|
||||
monkeypatch.setattr(orch, "_tail_container_logs", tail)
|
||||
assert await orch._provider_rate_limit_park_target("be-dev-1", _instance()) is None
|
||||
tail.assert_not_awaited() # short-circuits before reading logs
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_stopped_container_parks_on_session_limit(
|
||||
orch: AgentOrchestrator, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
inst = _instance()
|
||||
park = AsyncMock()
|
||||
spawn = AsyncMock()
|
||||
overload = AsyncMock(return_value=None)
|
||||
monkeypatch.setattr(orch, "_is_grok_rate_limit_exit", lambda _i, _e: False)
|
||||
monkeypatch.setattr(
|
||||
orch, "_provider_rate_limit_park_target", AsyncMock(return_value="anthropic")
|
||||
)
|
||||
monkeypatch.setattr(orch, "_provider_overload_park_target", overload)
|
||||
monkeypatch.setattr(orch, "_park_provider_unavailable", park)
|
||||
monkeypatch.setattr(orch, "_finalize_spawn_session", AsyncMock())
|
||||
monkeypatch.setattr(orch, "spawn_agent", spawn)
|
||||
|
||||
await orch._handle_stopped_container("be-dev-1", inst, exit_code=1)
|
||||
|
||||
park.assert_awaited_once_with(
|
||||
"be-dev-1",
|
||||
inst,
|
||||
provider="anthropic",
|
||||
retry_after=_RATE_LIMIT_RETRY_AFTER_S,
|
||||
kind="rate_limited",
|
||||
)
|
||||
spawn.assert_not_awaited() # crash-retry short-circuited
|
||||
overload.assert_not_awaited() # session-limit checked before the overload path
|
||||
|
||||
Reference in New Issue
Block a user