From cf637efd39d9ef086e72445da4e1f5ba61e06ec2 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 22 Apr 2026 13:14:56 +0300 Subject: [PATCH 1/6] feat(scheduler): agent-owned pre-check hook (#454) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New optional contract: agents implement POST /api/pre-check in their container; scheduler calls it before firing a cron-triggered chat. Endpoint absent or any error → fire as usual (fail-open). fire=false records a skipped execution. fire=true with a message overrides the schedule.message for that invocation. - docker/base-image/agent_server/routers/pre_check.py: new router that dynamically loads /home/developer/.trinity/pre-check.py (template- supplied) and calls its check() function - agent-server main.py: mount pre_check_router - scheduler/agent_client.py: pre_check() method with fail-open semantics on 404/5xx/timeout/malformed-response - scheduler/service.py: _run_pre_check + pre-check branch in _execute_schedule_with_lock (cron only; manual triggers bypass) - tests/scheduler_tests/test_pre_check.py: 12 tests covering client- and service-level behavior; 161/161 scheduler suite passes Zero schema change — reuses existing ExecutionStatus.SKIPPED and create_skipped_execution. Closes the "wake agent on every cron tick" cost gap noted in docs/planning/PR_REVIEWER_AGENT.md. Co-Authored-By: Claude Opus 4.7 (1M context) --- docker/base-image/agent_server/main.py | 2 + .../agent_server/routers/__init__.py | 2 + .../agent_server/routers/pre_check.py | 104 +++++++++ src/scheduler/agent_client.py | 42 ++++ src/scheduler/service.py | 66 +++++- tests/scheduler_tests/test_pre_check.py | 199 ++++++++++++++++++ 6 files changed, 413 insertions(+), 2 deletions(-) create mode 100644 docker/base-image/agent_server/routers/pre_check.py create mode 100644 tests/scheduler_tests/test_pre_check.py diff --git a/docker/base-image/agent_server/main.py b/docker/base-image/agent_server/main.py index e511786d0..ffe88a4f6 100644 --- a/docker/base-image/agent_server/main.py +++ b/docker/base-image/agent_server/main.py @@ -24,6 +24,7 @@ dashboard_router, skills_router, snapshot_router, + pre_check_router, ) from .state import agent_state from .services.trinity_mcp import inject_trinity_mcp_if_configured @@ -60,6 +61,7 @@ app.include_router(dashboard_router) # Dashboard endpoint app.include_router(skills_router) # Skills/playbooks listing endpoint app.include_router(snapshot_router) # Snapshot/restore primitives (#384, S3) +app.include_router(pre_check_router) # Agent-owned pre-check hook (#454) # #389 S1a: auto-sync heartbeat loop (gated by GIT_SYNC_AUTO env var). schedule_auto_sync_if_enabled(app) diff --git a/docker/base-image/agent_server/routers/__init__.py b/docker/base-image/agent_server/routers/__init__.py index 49890897e..69c0cd962 100644 --- a/docker/base-image/agent_server/routers/__init__.py +++ b/docker/base-image/agent_server/routers/__init__.py @@ -11,6 +11,7 @@ from .dashboard import router as dashboard_router from .skills import router as skills_router from .snapshot import router as snapshot_router +from .pre_check import router as pre_check_router __all__ = [ "chat_router", @@ -23,4 +24,5 @@ "dashboard_router", "skills_router", "snapshot_router", + "pre_check_router", ] diff --git a/docker/base-image/agent_server/routers/pre_check.py b/docker/base-image/agent_server/routers/pre_check.py new file mode 100644 index 000000000..a98d4d988 --- /dev/null +++ b/docker/base-image/agent_server/routers/pre_check.py @@ -0,0 +1,104 @@ +""" +Pre-check endpoint (#454). + +Optional hook that lets an agent template deterministically decide whether a +scheduled invocation should actually fire, without waking Claude. The scheduler +calls this endpoint before dispatching a cron-triggered chat; if the endpoint +is absent the scheduler falls back to today's behavior (fire as usual). + +Template authors implement the gate by dropping a Python file at +``/home/developer/.trinity/pre-check.py`` with a top-level ``check()`` function +that returns:: + + {"fire": False, "reason": "..."} + # or + {"fire": True, "message": "optional chat message override"} + +If ``check()`` returns ``fire=False`` the scheduler records a skipped +execution and does not call ``/api/chat``. If it returns ``fire=True`` the +scheduler fires the chat, using the returned ``message`` if present. + +Fail-open by design: any exception or malformed response at this layer +propagates to the scheduler as "no decision", which falls back to the +default fire behavior. A broken pre-check must never silently suppress +scheduled work. +""" +from __future__ import annotations + +import asyncio +import importlib.util +import inspect +import logging +from pathlib import Path +from typing import Any, Dict + +from fastapi import APIRouter, HTTPException + +logger = logging.getLogger(__name__) +router = APIRouter() + +PRE_CHECK_PATH = Path("/home/developer/.trinity/pre-check.py") +MAX_MESSAGE_BYTES = 32_000 + + +def _load_check_callable(): + """Dynamically load ``check`` from the template's pre-check file.""" + if not PRE_CHECK_PATH.exists(): + return None + spec = importlib.util.spec_from_file_location( + "_trinity_pre_check", PRE_CHECK_PATH + ) + if spec is None or spec.loader is None: + return None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) # may raise — caller logs and returns 500 + fn = getattr(module, "check", None) + if not callable(fn): + return None + return fn + + +def _normalise_result(raw: Any) -> Dict[str, Any]: + if not isinstance(raw, dict) or "fire" not in raw: + raise ValueError( + "pre-check check() must return a dict with a 'fire' bool" + ) + fire = bool(raw.get("fire")) + out: Dict[str, Any] = {"fire": fire} + if not fire: + reason = raw.get("reason") + if reason is not None: + out["reason"] = str(reason)[:2000] + return out + message = raw.get("message") + if message is not None: + message = str(message) + if len(message.encode("utf-8")) > MAX_MESSAGE_BYTES: + logger.warning( + "[pre-check] message exceeds %d bytes — dropping override", + MAX_MESSAGE_BYTES, + ) + else: + out["message"] = message + return out + + +@router.post("/api/pre-check") +async def pre_check() -> Dict[str, Any]: + """Run the template's ``check()`` if present; 404 if absent (fail-open).""" + fn = _load_check_callable() + if fn is None: + raise HTTPException(status_code=404, detail="pre-check not implemented") + try: + if inspect.iscoroutinefunction(fn): + raw = await fn() + else: + raw = await asyncio.get_event_loop().run_in_executor(None, fn) + except Exception as e: + logger.exception("[pre-check] check() raised: %s", e) + raise HTTPException(status_code=500, detail=f"pre-check error: {e}") + try: + return _normalise_result(raw) + except Exception as e: + logger.warning("[pre-check] invalid return shape: %s", e) + raise HTTPException(status_code=500, detail=str(e)) diff --git a/src/scheduler/agent_client.py b/src/scheduler/agent_client.py index 61e927a30..5ea871b16 100644 --- a/src/scheduler/agent_client.py +++ b/src/scheduler/agent_client.py @@ -152,6 +152,48 @@ async def task( result = response.json() return self._parse_task_response(result) + async def pre_check(self, timeout: float = 60.0) -> Optional[Dict[str, Any]]: + """ + Ask the agent whether a scheduled invocation should actually fire (#454). + + Returns: + None if the endpoint is absent (404) or the call fails for any + reason. The scheduler treats this as "no decision" and falls back + to normal firing (fail-open). + + Otherwise a dict with at least ``fire: bool`` and optionally + ``reason`` (str) and ``message`` (str — chat override). + """ + try: + response = await self._request( + "POST", "/api/pre-check", timeout=timeout, json={} + ) + except AgentClientError as e: + logger.info( + f"[pre-check] agent {self.agent_name} unreachable: {e} — fail-open" + ) + return None + if response.status_code == 404: + return None + if response.status_code >= 400: + logger.warning( + f"[pre-check] agent {self.agent_name} returned {response.status_code} — fail-open" + ) + return None + try: + data = response.json() + except Exception as e: + logger.warning( + f"[pre-check] agent {self.agent_name} invalid JSON ({e}) — fail-open" + ) + return None + if not isinstance(data, dict) or "fire" not in data: + logger.warning( + f"[pre-check] agent {self.agent_name} malformed response — fail-open" + ) + return None + return data + async def health_check(self, timeout: float = 5.0) -> bool: """ Check if agent is healthy and responding. diff --git a/src/scheduler/service.py b/src/scheduler/service.py index 546d3d0aa..3602665ab 100644 --- a/src/scheduler/service.py +++ b/src/scheduler/service.py @@ -723,13 +723,57 @@ async def _execute_schedule_with_lock(self, schedule_id: str, triggered_by: str logger.info(f"Schedule {schedule_id} skipped: agent {schedule.agent_name} autonomy is disabled") return + # Agent-owned pre-check hook (#454). Only for cron-triggered invocations: + # manual triggers from the UI represent an explicit operator decision and + # must always fire. Fail-open: any None return means the agent has no + # pre-check or the call errored — fall through to normal firing. + effective_message = schedule.message + if triggered_by == "schedule": + decision = await self._run_pre_check(schedule.agent_name) + if decision is not None: + if not decision.get("fire", True): + reason = decision.get("reason") or "pre-check returned fire=false" + skipped = self.db.create_skipped_execution( + schedule_id=schedule.id, + agent_name=schedule.agent_name, + message=schedule.message, + triggered_by=triggered_by, + skip_reason=f"pre-check: {reason}", + ) + logger.info( + f"Schedule {schedule.name} skipped by pre-check: {reason}" + ) + now = datetime.utcnow() + next_run = self._get_next_run_time( + schedule.cron_expression, schedule.timezone + ) + self.db.update_schedule_run_times( + schedule.id, last_run_at=now, next_run_at=next_run + ) + await self._publish_event({ + "type": "schedule_execution_skipped", + "agent": schedule.agent_name, + "schedule_id": schedule.id, + "execution_id": skipped.id if skipped else None, + "schedule_name": schedule.name, + "reason": reason, + }) + return + override = decision.get("message") + if override and isinstance(override, str): + effective_message = override + logger.info( + f"Schedule {schedule.name} pre-check overrode message " + f"({len(override)} chars)" + ) + logger.info(f"Executing schedule: {schedule.name} for agent {schedule.agent_name} (triggered_by={triggered_by})") # Create execution record execution = self.db.create_execution( schedule_id=schedule.id, agent_name=schedule.agent_name, - message=schedule.message, + message=effective_message, triggered_by=triggered_by, model_used=schedule.model ) @@ -755,7 +799,7 @@ async def _execute_schedule_with_lock(self, schedule_id: str, triggered_by: str try: result = await self._call_backend_execute_task( agent_name=schedule.agent_name, - message=schedule.message, + message=effective_message, triggered_by=triggered_by, model=schedule.model, timeout_seconds=schedule.timeout_seconds, @@ -854,6 +898,24 @@ async def _execute_schedule_with_lock(self, schedule_id: str, triggered_by: str "error": error_msg if actual_status == ExecutionStatus.FAILED else None }) + async def _run_pre_check(self, agent_name: str) -> Optional[dict]: + """Call the agent's ``/api/pre-check`` hook (#454). + + Returns the agent's decision dict, or ``None`` if the agent has no + hook or the call fails. ``None`` signals the caller to fire the + schedule as usual (fail-open). + """ + from .agent_client import AgentClient + + try: + client = AgentClient(agent_name, timeout=60.0) + return await client.pre_check() + except Exception as e: + logger.warning( + f"[pre-check] unexpected error for {agent_name}: {e} — fail-open" + ) + return None + async def _call_backend_execute_task( self, agent_name: str, diff --git a/tests/scheduler_tests/test_pre_check.py b/tests/scheduler_tests/test_pre_check.py new file mode 100644 index 000000000..b6176a122 --- /dev/null +++ b/tests/scheduler_tests/test_pre_check.py @@ -0,0 +1,199 @@ +""" +Tests for the agent-owned pre-check hook (#454). + +Covers: +- AgentClient.pre_check returns dict on 200, None on 404, None on error +- SchedulerService skips execution when pre_check returns fire=False +- SchedulerService uses override message when pre_check returns fire=True with message +- SchedulerService falls through to normal fire when pre_check returns None (fail-open) +""" + +# Path setup must happen before scheduler imports +import sys +from pathlib import Path + +_this_file = Path(__file__).resolve() +_src_path = str(_this_file.parent.parent.parent / "src") +if _src_path not in sys.path: + sys.path.insert(0, _src_path) + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scheduler.agent_client import AgentClient, AgentNotReachableError +from scheduler.models import ExecutionStatus + + +# --------------------------------------------------------------------------- +# AgentClient.pre_check unit tests +# --------------------------------------------------------------------------- + + +class TestAgentClientPreCheck: + @pytest.mark.asyncio + async def test_returns_decision_on_200(self): + client = AgentClient("pr-reviewer") + response = MagicMock() + response.status_code = 200 + response.json.return_value = {"fire": True, "message": "review this"} + with patch.object(client, "_request", AsyncMock(return_value=response)): + result = await client.pre_check() + assert result == {"fire": True, "message": "review this"} + + @pytest.mark.asyncio + async def test_returns_none_on_404(self): + """Endpoint absent — scheduler should fall back to normal fire.""" + client = AgentClient("no-hook-agent") + response = MagicMock() + response.status_code = 404 + with patch.object(client, "_request", AsyncMock(return_value=response)): + result = await client.pre_check() + assert result is None + + @pytest.mark.asyncio + async def test_returns_none_on_5xx(self): + client = AgentClient("broken-agent") + response = MagicMock() + response.status_code = 500 + with patch.object(client, "_request", AsyncMock(return_value=response)): + result = await client.pre_check() + assert result is None + + @pytest.mark.asyncio + async def test_returns_none_on_unreachable(self): + client = AgentClient("unreachable-agent") + with patch.object( + client, + "_request", + AsyncMock(side_effect=AgentNotReachableError("timeout")), + ): + result = await client.pre_check() + assert result is None + + @pytest.mark.asyncio + async def test_returns_none_on_malformed_json(self): + client = AgentClient("sketchy-agent") + response = MagicMock() + response.status_code = 200 + response.json.side_effect = ValueError("not json") + with patch.object(client, "_request", AsyncMock(return_value=response)): + result = await client.pre_check() + assert result is None + + @pytest.mark.asyncio + async def test_returns_none_on_missing_fire_field(self): + client = AgentClient("wrong-shape-agent") + response = MagicMock() + response.status_code = 200 + response.json.return_value = {"message": "hi"} # no "fire" + with patch.object(client, "_request", AsyncMock(return_value=response)): + result = await client.pre_check() + assert result is None + + @pytest.mark.asyncio + async def test_returns_skip_decision(self): + client = AgentClient("pr-reviewer") + response = MagicMock() + response.status_code = 200 + response.json.return_value = {"fire": False, "reason": "no new PRs"} + with patch.object(client, "_request", AsyncMock(return_value=response)): + result = await client.pre_check() + assert result == {"fire": False, "reason": "no new PRs"} + + +# --------------------------------------------------------------------------- +# SchedulerService._execute_schedule_with_lock pre-check branch +# --------------------------------------------------------------------------- + + +@pytest.fixture +def mock_scheduler(db_with_data): + """Build a SchedulerService with mocked dependencies so we can drive the + pre-check branch of `_execute_schedule_with_lock` without real networking.""" + from scheduler.service import SchedulerService + + svc = SchedulerService.__new__(SchedulerService) + svc.db = db_with_data + svc.lock_manager = MagicMock() + svc._publish_event = AsyncMock() + svc._call_backend_execute_task = AsyncMock( + return_value={"status": "dispatched"} + ) + svc._get_next_run_time = MagicMock(return_value=None) + return svc + + +class TestSchedulerPreCheckBranch: + @pytest.mark.asyncio + async def test_skip_when_fire_false(self, mock_scheduler, db_with_data): + """pre-check fire=False → create skipped execution, no chat dispatch.""" + mock_scheduler._run_pre_check = AsyncMock( + return_value={"fire": False, "reason": "no new PRs"} + ) + + await mock_scheduler._execute_schedule_with_lock("schedule-1") + + mock_scheduler._call_backend_execute_task.assert_not_called() + + mock_scheduler._publish_event.assert_awaited() + event = mock_scheduler._publish_event.await_args.args[0] + assert event["type"] == "schedule_execution_skipped" + assert event["reason"] == "no new PRs" + + skipped = db_with_data.get_execution(event["execution_id"]) + assert skipped is not None + assert skipped.status == ExecutionStatus.SKIPPED + assert "no new PRs" in (skipped.error or "") + + @pytest.mark.asyncio + async def test_fire_with_override_message(self, mock_scheduler, db_with_data): + """pre-check fire=True with message → chat dispatched with override.""" + mock_scheduler._run_pre_check = AsyncMock( + return_value={"fire": True, "message": "Review abilityai/trinity#371"} + ) + + await mock_scheduler._execute_schedule_with_lock("schedule-1") + + mock_scheduler._call_backend_execute_task.assert_awaited_once() + kwargs = mock_scheduler._call_backend_execute_task.await_args.kwargs + assert kwargs["message"] == "Review abilityai/trinity#371" + + @pytest.mark.asyncio + async def test_fail_open_when_pre_check_returns_none( + self, mock_scheduler, db_with_data + ): + """pre-check returns None (e.g. 404) → fire as usual with schedule.message.""" + mock_scheduler._run_pre_check = AsyncMock(return_value=None) + + await mock_scheduler._execute_schedule_with_lock("schedule-1") + + mock_scheduler._call_backend_execute_task.assert_awaited_once() + kwargs = mock_scheduler._call_backend_execute_task.await_args.kwargs + assert kwargs["message"] == "Run morning report" + + @pytest.mark.asyncio + async def test_fire_true_without_message_uses_schedule_message( + self, mock_scheduler, db_with_data + ): + mock_scheduler._run_pre_check = AsyncMock(return_value={"fire": True}) + + await mock_scheduler._execute_schedule_with_lock("schedule-1") + + mock_scheduler._call_backend_execute_task.assert_awaited_once() + kwargs = mock_scheduler._call_backend_execute_task.await_args.kwargs + assert kwargs["message"] == "Run morning report" + + @pytest.mark.asyncio + async def test_manual_trigger_bypasses_pre_check( + self, mock_scheduler, db_with_data + ): + """Manual triggers are explicit operator intent — pre-check must not run.""" + mock_scheduler._run_pre_check = AsyncMock() + + await mock_scheduler._execute_schedule_with_lock( + "schedule-1", triggered_by="manual" + ) + + mock_scheduler._run_pre_check.assert_not_called() + mock_scheduler._call_backend_execute_task.assert_awaited_once() From d389920b455a43e92391c27a412a8e5e73a7062e Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 22 Apr 2026 15:45:00 +0300 Subject: [PATCH 2/6] docs(#454): scheduler pre-check feature flow + arch + requirements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - feature-flows/scheduler-pre-check.md: new flow doc with contract, fail-open semantics, error table, testing summary - architecture.md: add /api/pre-check to agent-server endpoint list and pre-check note to Scheduler Service row - requirements.md: SCHED-COND-001 entry under §10 (Scheduling & Execution) - feature-flows.md: index row - docs/planning/PR_REVIEWER_AGENT.md: design doc from which this feature was extracted — committed for traceability Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/memory/architecture.md | 3 +- docs/memory/feature-flows.md | 1 + .../feature-flows/scheduler-pre-check.md | 125 ++++++++ docs/memory/requirements.md | 15 + docs/planning/PR_REVIEWER_AGENT.md | 272 ++++++++++++++++++ 5 files changed, 415 insertions(+), 1 deletion(-) create mode 100644 docs/memory/feature-flows/scheduler-pre-check.md create mode 100644 docs/planning/PR_REVIEWER_AGENT.md diff --git a/docs/memory/architecture.md b/docs/memory/architecture.md index 3d8c46847..3ee6c95ba 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -362,6 +362,7 @@ docker exec trinity-vector sh -c "tail -50 /data/logs/agents.json" | jq . - `/api/chat/session` - Context window stats - `/api/files` - List workspace files (recursive tree structure) - `/api/files/download` - Download file content (100MB limit) +- `/api/pre-check` - Optional agent-owned gate for scheduled invocations (SCHED-COND-001). If the template ships `~/.trinity/pre-check.py` with a `check()` function, the scheduler calls this endpoint before firing a cron-triggered chat; `fire=False` → scheduler records a skipped execution and does NOT wake Claude. Endpoint absent → scheduler falls back to normal cron behavior (backward compat). **Persistent Chat:** - All chat messages automatically saved to SQLite (`chat_sessions`, `chat_messages`) @@ -395,7 +396,7 @@ Services that run continuously in the backend process: | **Operator Queue Sync** | `operator_queue_service.py` | Polls running agents every 5s, reads `~/.trinity/operator-queue.json`, syncs to DB, writes responses back. (OPS-001) | | **Sync Health Service** | `sync_health_service.py` | Polls git-enabled agents every 60s, upserts `agent_sync_state`, emits `sync_failing` operator-queue entries when consecutive_failures ≥ 3. (#389 S1) | | **Monitoring Service** | `monitoring_service.py` | Fleet-wide health checks on configurable interval. (MON-001) | -| **Scheduler Service** | `scheduler_service.py` | APScheduler-based cron job execution. Async fire-and-forget with DB polling for status. | +| **Scheduler Service** | `scheduler_service.py` | APScheduler-based cron job execution. Async fire-and-forget with DB polling for status. On each cron-triggered fire, optionally calls the target agent's `POST /api/pre-check` hook; `fire=False` records a skipped execution and does not invoke Claude (SCHED-COND-001, #454). | | **Capacity Maintenance** | `capacity_manager.py` | Calls `CapacityManager.run_maintenance()` every 60s — expires stale queued tasks (>24h) and drains orphans after restart. (BACKLOG-001 / CAPACITY-CONSOLIDATE #428) | The **agent server** also runs a 15-min `auto_sync` heartbeat loop (gated diff --git a/docs/memory/feature-flows.md b/docs/memory/feature-flows.md index 8332e90bd..35db77323 100644 --- a/docs/memory/feature-flows.md +++ b/docs/memory/feature-flows.md @@ -19,6 +19,7 @@ | 2026-04-25 | #487 | Telegram file upload Phase 2 — workspace delivery hardening: NFKC sanitizer with collision dedup, spec injection format `[File uploaded by {uploader}]: {name} ({size}) saved to {path}`, all-writes-failed channel error + abort. Same code path benefits Slack inbound. | [telegram-integration.md](feature-flows/telegram-integration.md), [slack-file-sharing.md](feature-flows/slack-file-sharing.md) | | 2026-04-23 | #476 | SQLite lexicographic cutoff bug fix — new `iso_cutoff(hours)` helper replaces `datetime('now', ...)` in 15 sites across rate-limit / dashboard / schedules; `max_retries` default flipped `1 → 0`; `cleanup_old_rate_limit_events` wired into `CleanupService` (phase 6, hourly) | [subscription-auto-switch.md](feature-flows/subscription-auto-switch.md), [cleanup-service.md](feature-flows/cleanup-service.md), [scheduler-service.md](feature-flows/scheduler-service.md) | | 2026-04-22 | #458 | `.gitignore` init fix — `initialize_git_in_container` now appends missing patterns instead of truncate-and-write; adds `.env`, `.env.*`, `.mcp.json` to the default list and runs for both `/home/developer` and legacy `/home/developer/workspace` (stops credential leak on first GitHub sync) | [github-repo-initialization.md](feature-flows/github-repo-initialization.md) | +| 2026-04-22 | SCHED-COND-001 (#454) | Conditional schedule pre-check — agent-owned `POST /api/pre-check` hook lets templates deterministically skip scheduled ticks (zero Claude tokens on empty polls); fail-open; reuses `ExecutionStatus.SKIPPED` (no schema change) | [scheduler-pre-check.md](feature-flows/scheduler-pre-check.md) | | 2026-04-21 | RELIABILITY-003 (#306) | WebSocket event bus on Redis Streams — replaces in-process broadcast with XADD/XREAD, adds reconnect replay via `?last-event-id=`, 3-failure eviction, MAXLEN trim (tunable) | [websocket-event-bus.md](feature-flows/websocket-event-bus.md) | | 2026-04-20 | #420 | Scheduler sync loop fix — `update_schedule_run_times` no longer bumps `updated_at`, stopping the self-triggering re-register of every schedule per tick | [scheduler-service.md](feature-flows/scheduler-service.md) | | 2026-04-20 | #418 | Inter-agent timeout honors per-agent `execution_timeout_seconds` — removed 600s hardcoded defaults in MCP `chat_with_agent`/`fan_out` tools and fan-out service; HTTP client ceiling bumped to platform max (7200s) | [fan-out.md](feature-flows/fan-out.md), [mcp-orchestration.md](feature-flows/mcp-orchestration.md), [parallel-headless-execution.md](feature-flows/parallel-headless-execution.md) | diff --git a/docs/memory/feature-flows/scheduler-pre-check.md b/docs/memory/feature-flows/scheduler-pre-check.md new file mode 100644 index 000000000..6974a5871 --- /dev/null +++ b/docs/memory/feature-flows/scheduler-pre-check.md @@ -0,0 +1,125 @@ +# Feature: Conditional Schedule Pre-Check (SCHED-COND-001 / Issue #454) + +## Overview +Optional agent-owned hook that lets a scheduled cron tick be skipped deterministically without waking Claude. The scheduler calls `POST /api/pre-check` on the target agent before firing a cron-triggered chat; `fire=False` records a skipped execution with zero Claude token cost. Eliminates per-tick token burn on poll-driven agents (PR reviewers, inbox monitors, alert routers). + +## User Story +As the author of a poll-driven agent template, I want my agent's own code to decide whether each scheduled tick actually needs to run, so that cheap deterministic checks (scan GitHub, list unread mail, compare cost threshold) replace a full Claude turn on every empty poll. + +## Entry Points +- **Agent container endpoint** (new): `POST http://agent-:8000/api/pre-check` — internal network only; called by the scheduler service. +- **Template contract**: drop `~/.trinity/pre-check.py` with a top-level `def check()` returning `{"fire": bool, "message": str?, "reason": str?}`. +- **No operator-facing API change**: schedule CRUD endpoints and the Schedules UI are unchanged. Operators create normal cron schedules; agents own the gate. + +## Frontend Layer +No UI change. Skipped executions appear in the existing schedule executions list alongside `success`/`failed` rows — visible immediately because the frontend already renders the `status` field as a badge. + +## Agent Server Layer +**Router**: `docker/base-image/agent_server/routers/pre_check.py` + +- Dynamically loads `/home/developer/.trinity/pre-check.py` via `importlib.util.spec_from_file_location`. No caching beyond Python's default `sys.modules` behavior. +- Returns 404 if the file is absent (fail-open signal to scheduler). +- Accepts both sync and async `check()` functions (`inspect.iscoroutinefunction` branch). +- Normalises the return value: clamps `reason` to 2000 chars; drops `message` if it exceeds 32 KB UTF-8; requires `fire` key or raises 500. +- Any exception in `check()` → 500 with the exception text. Scheduler treats 500 as fail-open. + +**Registration**: `docker/base-image/agent_server/routers/__init__.py` and `main.py` mount `pre_check_router` alongside existing routers (chat, files, git, skills, dashboard). + +## Scheduler Layer +**Client**: `src/scheduler/agent_client.py` + +New `AgentClient.pre_check(timeout=60.0) -> Optional[dict]`: +- 404 / 5xx / timeout / malformed JSON / missing `fire` field → returns `None`. +- Valid 200 → returns decision dict. +- `None` signals "no decision, fire as usual" (fail-open). + +**Service**: `src/scheduler/service.py` + +New `_run_pre_check(agent_name)` wraps the client call with a broad `except Exception` — any unexpected error also returns `None`. + +Intercept point: `_execute_schedule_with_lock` calls `_run_pre_check` only when `triggered_by == "schedule"` (cron). Manual triggers bypass entirely. + +```python +effective_message = schedule.message +if triggered_by == "schedule": + decision = await self._run_pre_check(schedule.agent_name) + if decision is not None: + if not decision.get("fire", True): + skipped = self.db.create_skipped_execution(...) + # publish event, update run times, return — do not create execution + return + override = decision.get("message") + if override and isinstance(override, str): + effective_message = override +``` + +`effective_message` is then threaded through `create_execution()` and `_call_backend_execute_task()`. + +## Data Layer +**Zero schema change.** Reuses existing: +- `ExecutionStatus.SKIPPED` (already defined in `src/scheduler/models.py` for Issue #46's max-instances drop handling). +- `SchedulerDatabase.create_skipped_execution(...)` (already written to record APScheduler-dropped runs). + +Skip rows carry `status='skipped'`, `error=f"pre-check: {reason}"`, `duration_ms=0`, `started_at == completed_at`. + +## WebSocket Layer +New event type: `schedule_execution_skipped` + +```json +{ + "type": "schedule_execution_skipped", + "agent": "pr-reviewer", + "schedule_id": "LidXcOwtDsDuFTFGvkUqCw", + "execution_id": "I2DWodfpYpuJbs1TTZ42Ig", + "schedule_name": "PR review poll", + "reason": "no new PRs" +} +``` + +## Side Effects +- Skipped execution row written to `schedule_executions` table (visible in UI immediately) +- `schedule.last_run_at` and `next_run_at` updated (so missed-schedule detection still works) +- WebSocket event broadcast to any subscribed UIs +- No `/api/internal/execute-task` call, no backend task creation, no Claude invocation, no backlog slot acquisition + +## Error Handling +| Condition | Scheduler behavior | +|---|---| +| Agent container unreachable | Fires as usual (fail-open); logs warning | +| `/api/pre-check` returns 404 | Fires as usual (backward compat for templates without a hook) | +| `/api/pre-check` returns 5xx | Fires as usual; logs warning | +| Response times out | Fires as usual; logs warning | +| Response missing `fire` field | Fires as usual; logs warning | +| `fire=false` | Records skipped execution, no chat dispatch | +| `fire=true` with `message` | Fires chat with override message | +| `fire=true` without `message` | Fires chat with `schedule.message` (existing behavior) | +| Manual trigger (`triggered_by != 'schedule'`) | Skips pre-check entirely — explicit operator intent always fires | + +## Security +- Pre-check runs inside the agent's container as `developer`, same sandbox as chat-mode tool calls. No new privilege. +- Stdout/message cap at 32 KB UTF-8 — oversized payloads are dropped with a warning, fall through to schedule.message. +- Fail-open policy means a malicious/broken pre-check cannot suppress scheduled invocations (worst case: wastes tokens — today's baseline). +- Invariant #5 ("Agent Server Mirrors Backend") preserved: agent exposes an HTTP contract, scheduler proxies to it. +- Invariant #1 ("Three-layer backend: router → service → db") preserved on the scheduler side — CLI wrapper → service → database. + +## Testing +**Unit** (`tests/scheduler_tests/test_pre_check.py`, 12 tests): +- `AgentClient.pre_check` — returns decision on 200, None on 404, None on 5xx, None on unreachable, None on malformed JSON, None on missing `fire` field, normal pass on fire-false, normal pass on fire-true-with-message. +- `SchedulerService._execute_schedule_with_lock` — skip records execution with correct reason, fire-true with message uses override, fail-open routes through backend, fire-true without message uses schedule.message, manual trigger bypasses pre-check. + +Full scheduler suite: 161/161 passing. + +**Live end-to-end** (verified 2026-04-22 in local Trinity): +- Empty scan on `dolho/pr-reviewer-agent` → `fire:false` → skip row in DB, `$0` cost, zero backend activity. +- Open PR on that repo → next tick `fire:true`, override message with PR list → Claude runs `/review`, posts comment. +- Subsequent tick with existing bot comment → `fire:false` again (stateless dedup via GitHub-hosted comment thread). + +## Related Flows +- `feature-flows/agent-event-subscriptions.md` — EVT-001, the event-driven analogue (other trigger source, same "non-cron agent invocation" theme). +- `feature-flows/scheduler-service.md` — base scheduler behavior this extends. +- `docs/planning/PR_REVIEWER_AGENT.md` — the motivating use case that drove this feature. + +## Migration / Rollout +- Zero migration required (no schema change). +- Existing schedules and agent templates behave identically after deploy (endpoint absent → fall back to today's fire semantics). +- Templates opt in by shipping `~/.trinity/pre-check.py`. No Trinity-side flag. diff --git a/docs/memory/requirements.md b/docs/memory/requirements.md index a86ee380d..24f139d8c 100644 --- a/docs/memory/requirements.md +++ b/docs/memory/requirements.md @@ -373,6 +373,21 @@ Trinity is autonomous agent orchestration and infrastructure — sovereign infra - Configurable `POLL_INTERVAL` env var (default 10s) - **Root Cause**: TCP connection drops after 15-30 min on long-running scheduled tasks, causing false `failed` status even though agent work completed successfully +### 10.6.1 Conditional Schedule Pre-Check (SCHED-COND-001) +- **Status**: ✅ Implemented (2026-04-22) +- **Requirement ID**: SCHED-COND-001 +- **GitHub Issue**: #454 +- **Description**: Optional agent-owned hook that lets a scheduled cron tick be skipped deterministically — scheduler calls `POST /api/pre-check` on the target agent before firing a chat and records a skipped execution when the agent returns `fire=false`. Eliminates Claude token cost on empty polls for poll-driven agents (PR reviewers, inbox monitors, alert routers, RSS watchers). +- **Key Features**: + - Contract: agent templates drop a `~/.trinity/pre-check.py` with a top-level `check()` returning `{"fire": bool, "message": str?, "reason": str?}`. Base-image router at `POST /api/pre-check` invokes it. + - Fail-open: endpoint absent (404), timeout, 5xx, malformed response → scheduler fires as usual. A broken pre-check never silently suppresses scheduled work. + - Message override: `fire=true` with a `message` field replaces `schedule.message` for that one invocation — lets the agent inject real work items (e.g. the PR list) into the chat prompt. + - Skip record: `fire=false` writes a row to `schedule_executions` with `status='skipped'`, reason, and zero cost — visible in the Trinity UI alongside successful runs. + - Manual triggers bypass pre-check entirely (explicit operator intent always fires). + - Zero DB schema change (reuses existing `ExecutionStatus.SKIPPED` + `create_skipped_execution`). +- **Test plan**: 12 unit tests covering client response shapes (200/404/5xx/timeout/malformed) + scheduler branch behaviors (skip, override, fail-open, manual-bypass). Full 161-test scheduler suite passes. +- **Root Cause**: No platform primitive for "deterministic gate before LLM invocation." Previously required per-template daemons backgrounded inside agent containers — invisible to Trinity UI, reimplemented per template, no skip metrics. + ### 10.7 Per-Agent Execution Timeout (TIMEOUT-001) - **Status**: ✅ Implemented (2026-03-12) - **Requirement ID**: TIMEOUT-001 diff --git a/docs/planning/PR_REVIEWER_AGENT.md b/docs/planning/PR_REVIEWER_AGENT.md new file mode 100644 index 000000000..d82e71987 --- /dev/null +++ b/docs/planning/PR_REVIEWER_AGENT.md @@ -0,0 +1,272 @@ +# PR Reviewer Agent — Planning Doc + +**Status**: Draft — pending review +**Date**: 2026-04-22 +**Goal**: Autonomous agent that watches a configurable set of GitHub repos, runs `/review` on every new PR, and posts the resulting review as a PR comment. Safety goal: the agent has **near-zero** direct ability to mutate any repo; all side effects go through a narrowly-scoped deterministic Python CLI. + +--- + +## 1. Trust Boundary + +| Layer | Can do | Cannot do | +|-------|--------|-----------| +| **Deterministic CLI** (`pr-reviewer`) | Read PR list, fetch diff, post issue comment, update local state DB | Close/merge PRs, push code, create branches, approve/request-changes, edit files, run arbitrary `gh` | +| **Agent (Claude)** | Call `pr-reviewer` subcommands on an allow-list, read fetched diffs, invoke the `/review` skill, write review markdown to a sandbox path | Talk to GitHub directly, run raw `gh`/`git`, use a PAT, see credential values | + +Concretely: the **GitHub PAT never lands in the agent's environment**. It lives in the CLI process's env, invoked as an out-of-process subprocess by the agent, and the CLI enforces the allow-list at its entry point. If the agent is ever jailbroken to try `gh pr merge` or `git push`, it has no token and no tool to do so. + +--- + +## 2. Architecture + +``` +┌──────────────────────────────────────────────────────────────────────┐ +│ Trinity Agent Container │ +│ │ +│ ┌─────────────────────────┐ │ +│ │ pr-reviewer daemon │ ◄── sleep(interval) loop, no Claude │ +│ │ - scan every N minutes │ │ +│ │ - if empty: sleep again │ │ +│ │ - if work: POST /api/chat to localhost Claude │ +│ └──────────┬──────────────┘ │ +│ │ (only when PRs found) │ +│ ▼ │ +│ ┌────────────────────────┐ subprocess ┌──────────────────────┐ │ +│ │ Claude Code (agent) │ ───────────► │ pr-reviewer CLI │ │ +│ │ - /review skill │ │ (same binary) │ │ +│ │ - no PAT in env │ ◄───stdout─── │ - GITHUB_TOKEN in env │ │ +│ └────────────────────────┘ └──────────┬───────────┘ │ +│ │ │ +│ ┌─────────────────────┼──────────┐ │ +│ │ ▼ │ │ +│ │ SQLite state GitHub API │ │ +│ │ (reviewed_prs) (fine-grained│ │ +│ │ PAT, RW on │ │ +│ │ PRs only) │ │ +│ └────────────────────────────────┘ │ +└──────────────────────────────────────────────────────────────────────┘ +``` + +### Trigger model — deterministic, zero-token-on-empty + +Trinity's cron fires a chat turn, which costs Claude tokens on every poll even when nothing to do. To avoid that, polling lives **outside** Claude: a sibling daemon in the same container runs the scan. Claude is only woken when there is real work. + +**Daemon loop** (in-container, no Claude invocations): +1. `pr-reviewer daemon --interval 900` starts at container boot, alongside `agent-server.py`. +2. Every 15 min: `pr-reviewer scan` → list of new PRs since last run. +3. **If empty** → sleep, no token cost, nothing happens. +4. **If non-empty** → `POST http://localhost:8000/api/chat` with a single batch message: `"Review the following PRs: abilityai/trinity#371, abilityai/abilities#42"`. + +**Claude loop** (only fires when daemon hands work off): +1. Agent receives the batch prompt via local chat. +2. For each PR: `pr-reviewer fetch #` → CLI writes `diff.md` + `meta.json` under `~/work///`. +3. Agent reads those files, invokes `/review`, writes `review.md`. +4. Agent calls `pr-reviewer post # --file review.md` → CLI posts the comment and marks `state.db`. +5. Agent reports per-PR status and exits the chat turn. + +**Why not Trinity scheduler directly?** Scheduler messages are chat messages — every fire is a Claude invocation. For a 15-min poll across quiet repos, that's ~96 empty turns/day × ~500 tokens = ~48k wasted tokens/day. The daemon sidesteps this by keeping polling deterministic and Python-only. + +**Fallback option** if we cannot add a daemon to the base image: use the Trinity scheduler with a minimal prompt (`"run pr-reviewer scan; if empty, reply DONE"`) — still costs tokens per poll but kept small. Not recommended unless image changes are blocked. + +--- + +## 3. The Deterministic CLI — `pr-reviewer` + +Single Python module, shipped with the agent. Uses `PyGithub` (or raw `httpx` against GitHub REST). Subcommands are the **only** way side effects happen. + +| Subcommand | Purpose | Writes? | +|------------|---------|---------| +| `scan` | List PRs needing review across configured repos. Returns JSON `[{repo, number, head_sha, title, author, url}, ...]`. | Reads only | +| `fetch #` | Download diff + metadata into `work///{diff.md, meta.json}`. | Local filesystem only | +| `post # --file ` | Post the file contents as a single PR issue comment. Prepends a bot-identity header. Refuses if PR already has a bot comment for current `head_sha`. | GitHub issue comment + `state.db` | +| `status` | Show recently reviewed PRs (from `state.db`). | Reads only | +| `config validate` | Lint the YAML config. | Reads only | +| `daemon --interval ` | Long-running loop: `scan` → if work, wake Claude via localhost `/api/chat`, else sleep. Never calls GitHub writes itself — only triggers the Claude loop. Started at container boot. | Local chat HTTP only | + +**Hard-coded restrictions inside the CLI** (not configurable, so the agent cannot loosen them): +- Only `POST /repos/{owner}/{repo}/issues/{number}/comments` is reachable for writes. +- No `PATCH`, no `MERGE`, no reviews API (`/pulls/{n}/reviews`), no branch/ref writes. +- Comment body capped (e.g. 60 KB) — hard rejection over that. +- Repo allow-list from config; any `repo` arg outside the list → error. +- Rate limit: max **N comments per hour per repo** (configurable, default 6) — CLI sleeps/fails rather than exceeding. +- Dry-run mode (`--dry-run` or `DRY_RUN=1`) that logs but never calls GitHub — default on for first deploy. + +--- + +## 4. Config Shape + +`~/work/config.yaml` in the agent workspace: + +```yaml +repos: + - owner: abilityai + repo: trinity + filters: + draft: false + labels_any: [] # empty = all + labels_none: [skip-bot-review] + authors_none: [dependabot[bot]] + - owner: abilityai + repo: abilities + +policy: + review_on: new_pr # new_pr | new_pr_and_push + max_comments_per_hour: 6 + comment_header: "🤖 **Trinity PR Reviewer**" + dry_run: true # flip to false after a week of dry-run output +``` + +--- + +## 5. State + +Local SQLite at `~/.pr-reviewer/state.db`, owned by the CLI: + +```sql +CREATE TABLE reviewed_prs ( + repo TEXT NOT NULL, + pr_number INTEGER NOT NULL, + head_sha TEXT NOT NULL, + reviewed_at TEXT NOT NULL, + comment_url TEXT, + comment_body_sha TEXT, -- for idempotency / detect drift + PRIMARY KEY (repo, pr_number, head_sha) +); +CREATE TABLE rate_limit ( + repo TEXT NOT NULL, + posted_at TEXT NOT NULL +); +``` + +This means **re-review** behavior is deterministic: a PR with a new head SHA reopens for review iff `policy.review_on == new_pr_and_push`, otherwise stays silent. + +--- + +## 6. PAT Scoping + +One fine-grained PAT issued by the bot's GitHub account: +- **Repositories**: explicit allow-list (same as config) +- **Permissions**: `pull_requests: read & write`, `contents: read`, `metadata: read` — **nothing else** (no workflows, no actions, no admin) +- Stored via Trinity's CRED-002 `.env` injection as `GITHUB_TOKEN` +- `.env` file readable only by the CLI process? In practice the agent container can `cat .env`, so this is belt-and-braces: the CLI's allow-list is the real gate, the narrow PAT is defense-in-depth. + +--- + +## 7. Trinity Platform Integration + +| Concern | How | +|---------|-----| +| **Deployment** | Custom agent template with `pr-reviewer` CLI pre-installed; create via `POST /api/agents` or Trinity UI. | +| **Credentials** | `GITHUB_TOKEN` (fine-grained PAT) injected via `POST /api/agents/{name}/credentials/inject`. No other secrets needed. | +| **Scheduling** | **No Trinity cron schedule.** `pr-reviewer daemon --interval 900` runs in-container next to `agent-server.py`, backgrounded from `~/.trinity/setup.sh` (which Trinity's `startup.sh` already invokes on every boot — no base-image change needed). The daemon does the polling; Claude is only invoked when the daemon POSTs work to localhost `/api/chat`. Zero Claude tokens burned on empty polls. | +| **Read-only mode** | Turn on `PUT /api/agents/{name}/read-only` — blocks accidental edits to source files; `work/` stays writable because read-only only gates source paths. | +| **Autonomy** | Enabled; no human in loop for routine reviews. | +| **Audit trail** | Each CLI invocation logs to stdout → Vector → `agents.json`. Every GitHub write the CLI makes is recorded with PR id + comment URL. Optionally emit Trinity events via MCP `emit_event` for a dashboard. | +| **Kill switch** | Flip `dry_run: true` in config OR disable the schedule via `PUT /api/agents/{name}/autonomy`. Either stops posts immediately. | + +--- + +## 8. Agent System Prompt (sketch) + +``` +You are the PR Reviewer agent. You review GitHub pull requests using the +/review skill and post the result as a comment. + +You have exactly ONE tool for GitHub interaction: the `pr-reviewer` CLI. +You MUST NOT use `gh`, `git push`, `curl`, or any other network tool +against GitHub. You do not have a GitHub token and these calls will fail. + +Loop per invocation: + 1. Run `pr-reviewer scan`. Parse JSON. + 2. For each entry: `pr-reviewer fetch #`, then read the + diff, run /review, write review.md, then + `pr-reviewer post # --file review.md`. + 3. If the CLI rejects a post (rate limit, duplicate, size), skip and + continue. Do not retry aggressively. + 4. Report per-PR status at the end. + +Never modify files outside `~/work/`. Never open shells against the +GitHub API directly. +``` + +--- + +## 9. V1 Scope + +In scope: +- Single top-level PR comment with the review markdown. +- Polling-based discovery via `scan` (no webhooks yet). +- SQLite state, dry-run default, rate limit, repo allow-list. +- One bot identity (one PAT), multi-repo. + +Out of scope (V2+): +- **Per-line code comments** via `/pulls/{n}/reviews` — needs another CLI subcommand with its own allow-list. Adds complexity; defer until V1 is stable. +- **Webhook-driven** (GitHub App or Cloudflare webhook → Trinity) for sub-minute latency. Polling is fine for V1. +- **Learning from reactions** (👎 on reviews → tune prompt). +- **PR approve/request-changes** — explicitly never, to preserve the "comment only" trust boundary. + +--- + +## 10. Security Review Checklist (pre-deploy) + +- [ ] PAT is fine-grained, scoped to configured repos only, `pull_requests:write` only +- [ ] CLI allow-list tested: attempting `pr-reviewer post` with a repo not in config fails closed +- [ ] CLI size-cap tested: 100 KB body rejected +- [ ] CLI rate-limit tested: 7th comment in an hour blocked +- [ ] Duplicate-comment guard tested: same head_sha twice → CLI refuses +- [ ] Dry-run default verified: fresh deploy posts nothing to GitHub +- [ ] Agent read-only mode enabled +- [ ] Audit log confirms all write attempts (successful and refused) + +--- + +## 11. Open Questions + +1. ~~Daemon launch mechanism~~ — **resolved**. No base-image change needed. `docker/base-image/startup.sh` already sources `/home/developer/.trinity/setup.sh` on every boot (used for restoring user packages). The template for this agent ships a `.trinity/setup.sh` that backgrounds the daemon: + + ```bash + nohup python3 /home/developer/bin/pr-reviewer daemon --interval 900 \ + > /home/developer/logs/daemon.log 2>&1 & + ``` + + Same `&`-backgrounding pattern Trinity already uses for `agent-server.py`. Daemon restarts with the container. Crash recovery: a `while true; do ...; done` wrapper or systemd-style restart policy in the daemon itself. +2. **Which review skill exactly** — the existing `/review` in `.claude/skills/review` (pre-landing PR review) applies to Trinity's own branch. For external repos we won't have a `main` to diff against in the agent container. Likely need a variant that works off the PR diff payload directly. Is this a new skill or a flag on the existing one? +3. **One PAT across repos or per-repo PATs?** One is simpler; per-repo is tighter blast radius if one repo's scope changes. +4. **Re-review on new pushes?** Default to single-review-per-PR (cheaper, less noise). Override per-repo in config if desired. +5. **Review latency target?** Polling every 15 min is cheap. Sub-5-min needs webhooks → adds Cloudflare Tunnel + HTTP endpoint to the agent. Defer? +6. **Handling draft PRs** — default skip, configurable? +7. **Bot identity** — dedicated GitHub user (recommended, clean audit trail) or run as a human account? + +--- + +## 12. Delivery Plan + +| Step | Artifact | Est. effort | +|------|----------|-------------| +| 1 | `pr-reviewer` CLI (Python, PyGithub, subcommands + allow-list + state DB + tests) | 1 day | +| 2 | Agent template (CLAUDE.md + system prompt + schedule + config) | 0.5 day | +| 3 | Fine-grained PAT issuance, credential injection | 0.5 day | +| 4 | Dry-run soak on abilityai/trinity + abilityai/abilities (observe, do not post) | 2–3 days | +| 5 | Flip `dry_run: false`, monitor first 20 reviews for quality + rate-limit behavior | ongoing | +| 6 | Follow-up issue for V2 (per-line comments, webhooks) | later | + +--- + +## 13. Failure Modes & Responses + +| Failure | Detection | Response | +|---------|-----------|----------| +| PAT revoked / expired | CLI 401 from GitHub | Agent reports; ops rotates PAT via credential injection endpoint | +| Agent posts spam / low-quality reviews | Human reviewer 👎 / issue report | Flip `dry_run: true` via config update + agent chat; disable schedule | +| Rate limit hit | CLI refuses | Already the intended behavior — safe | +| CLI bug writes wrong repo | Should be impossible (allow-list) — unit-tested | CLI tests gate the release | +| Agent tries direct `gh`/`git push` | No token → fails; logged to Vector | Audit log review, tune system prompt | + +--- + +## 14. Related + +- Trinity credential injection: `docs/memory/architecture.md` §Credentials (CRED-002) +- Scheduling: `docs/memory/architecture.md` §Background Services +- Existing review skill: `.claude/skills/review/` (needs variant for external-repo diffs — see Open Q #1) +- GitHub fine-grained PATs: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens#fine-grained-personal-access-tokens From d7a26e31b5ec792f167b49809fdd48e273cb6dec Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 22 Apr 2026 16:07:49 +0300 Subject: [PATCH 3/6] review: address PR #455 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pre_check.py: asyncio.get_event_loop() → get_running_loop() (deprecated in 3.10+) - pre_check.py: oversized message override no longer dropped silently — response now carries message_truncated="override dropped: N bytes exceeds 32000 cap" so scheduler/operator can see what happened; log escalated to ERROR with size+limit details - pre_check.py: module-level docstring expanded to note the security scope of check() (full Python interpreter access, same sandbox as chat tools — operators should review .trinity/pre-check.py like any executable template file) and the intentional no-cache behavior - tests/unit/test_pre_check_router.py: 15 new router/unit tests covering oversized-message drop path and non-dict return → 500 (both previously only exercised by inspection). Uses importlib to load pre_check.py directly, avoiding python-multipart requirement from sibling routers - feature-flows/scheduler-pre-check.md: document truncation behavior, security scope expectation, and updated test summary (12 scheduler + 15 router = 176 total passing) Lock-scope concern noted in review is not an issue: the skip path returns from _execute_schedule_with_lock, and the outer _execute_schedule holds the lock in a try/finally that covers the return. No leak. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../agent_server/routers/pre_check.py | 30 ++- .../feature-flows/scheduler-pre-check.md | 16 +- tests/unit/test_pre_check_router.py | 208 ++++++++++++++++++ 3 files changed, 245 insertions(+), 9 deletions(-) create mode 100644 tests/unit/test_pre_check_router.py diff --git a/docker/base-image/agent_server/routers/pre_check.py b/docker/base-image/agent_server/routers/pre_check.py index a98d4d988..8e7b49de6 100644 --- a/docker/base-image/agent_server/routers/pre_check.py +++ b/docker/base-image/agent_server/routers/pre_check.py @@ -22,6 +22,19 @@ propagates to the scheduler as "no decision", which falls back to the default fire behavior. A broken pre-check must never silently suppress scheduled work. + +Security note: ``check()`` is executed with the agent-server's full Python +interpreter access — templates can import ``subprocess``, open sockets, +etc. This is the same sandbox as chat-mode tool calls (non-root container, +dropped capabilities, isolated network), so it introduces no new privilege, +but operators reviewing templates should treat ``.trinity/pre-check.py`` +with the same scrutiny as ``CLAUDE.md`` and any skill files. + +Module-reload caching is intentionally absent: every request re-runs +``exec_module`` so template edits take effect on the next call without +a restart. Given the file is small and the poll cadence is minutes, the +cost is negligible. If polling tightens to sub-second, add mtime-based +caching — see PR #455 review thread for history. """ from __future__ import annotations @@ -73,11 +86,20 @@ def _normalise_result(raw: Any) -> Dict[str, Any]: message = raw.get("message") if message is not None: message = str(message) - if len(message.encode("utf-8")) > MAX_MESSAGE_BYTES: - logger.warning( - "[pre-check] message exceeds %d bytes — dropping override", + size = len(message.encode("utf-8")) + if size > MAX_MESSAGE_BYTES: + # Silently falling back to schedule.message would hide a real + # template bug. Log loudly and expose the reason in the response + # so the scheduler/operator can see what happened. + logger.error( + "[pre-check] message override %d bytes exceeds cap %d — " + "falling back to schedule.message", + size, MAX_MESSAGE_BYTES, ) + out["message_truncated"] = ( + f"override dropped: {size} bytes exceeds {MAX_MESSAGE_BYTES} cap" + ) else: out["message"] = message return out @@ -93,7 +115,7 @@ async def pre_check() -> Dict[str, Any]: if inspect.iscoroutinefunction(fn): raw = await fn() else: - raw = await asyncio.get_event_loop().run_in_executor(None, fn) + raw = await asyncio.get_running_loop().run_in_executor(None, fn) except Exception as e: logger.exception("[pre-check] check() raised: %s", e) raise HTTPException(status_code=500, detail=f"pre-check error: {e}") diff --git a/docs/memory/feature-flows/scheduler-pre-check.md b/docs/memory/feature-flows/scheduler-pre-check.md index 6974a5871..6a88a3a08 100644 --- a/docs/memory/feature-flows/scheduler-pre-check.md +++ b/docs/memory/feature-flows/scheduler-pre-check.md @@ -20,7 +20,8 @@ No UI change. Skipped executions appear in the existing schedule executions list - Dynamically loads `/home/developer/.trinity/pre-check.py` via `importlib.util.spec_from_file_location`. No caching beyond Python's default `sys.modules` behavior. - Returns 404 if the file is absent (fail-open signal to scheduler). - Accepts both sync and async `check()` functions (`inspect.iscoroutinefunction` branch). -- Normalises the return value: clamps `reason` to 2000 chars; drops `message` if it exceeds 32 KB UTF-8; requires `fire` key or raises 500. +- Normalises the return value: clamps `reason` to 2000 chars; requires `fire` key or raises 500. If `check()` returns a non-dict (e.g. `None`, `True`), the normaliser raises `ValueError` which surfaces as a 500 — scheduler treats that as fail-open. +- **Oversized `message` override**: if `message` exceeds 32 KB UTF-8, the router drops it from the response and sets a sibling `message_truncated: "override dropped: N bytes exceeds 32000 cap"` key. The scheduler sees no `message`, falls back to `schedule.message`, and operators can inspect the truncated-reason in agent-server logs (logged at `ERROR`). Templates must produce compact prompts or rely on `schedule.message`. - Any exception in `check()` → 500 with the exception text. Scheduler treats 500 as fail-open. **Registration**: `docker/base-image/agent_server/routers/__init__.py` and `main.py` mount `pre_check_router` alongside existing routers (chat, files, git, skills, dashboard). @@ -96,18 +97,23 @@ New event type: `schedule_execution_skipped` | Manual trigger (`triggered_by != 'schedule'`) | Skips pre-check entirely — explicit operator intent always fires | ## Security -- Pre-check runs inside the agent's container as `developer`, same sandbox as chat-mode tool calls. No new privilege. -- Stdout/message cap at 32 KB UTF-8 — oversized payloads are dropped with a warning, fall through to schedule.message. +- Pre-check runs inside the agent's container as `developer`, same sandbox as chat-mode tool calls. No new privilege over what chat-mode tool calls can already do. +- **Template review expectation**: `~/.trinity/pre-check.py` is executed with full Python interpreter access — it can `import subprocess`, open sockets, read files. This is intentional (operators already trust the template's `CLAUDE.md`, skills, and tool invocations), but `.trinity/pre-check.py` should be reviewed with the same scrutiny as any other executable file the template ships. +- Stdout/message cap at 32 KB UTF-8 — oversized payloads are dropped from the response and the drop is surfaced via a `message_truncated` field + `ERROR` log, not silently swallowed. - Fail-open policy means a malicious/broken pre-check cannot suppress scheduled invocations (worst case: wastes tokens — today's baseline). - Invariant #5 ("Agent Server Mirrors Backend") preserved: agent exposes an HTTP contract, scheduler proxies to it. - Invariant #1 ("Three-layer backend: router → service → db") preserved on the scheduler side — CLI wrapper → service → database. ## Testing -**Unit** (`tests/scheduler_tests/test_pre_check.py`, 12 tests): +**Scheduler-side** (`tests/scheduler_tests/test_pre_check.py`, 12 tests): - `AgentClient.pre_check` — returns decision on 200, None on 404, None on 5xx, None on unreachable, None on malformed JSON, None on missing `fire` field, normal pass on fire-false, normal pass on fire-true-with-message. - `SchedulerService._execute_schedule_with_lock` — skip records execution with correct reason, fire-true with message uses override, fail-open routes through backend, fire-true without message uses schedule.message, manual trigger bypasses pre-check. -Full scheduler suite: 161/161 passing. +**Agent-server router** (`tests/unit/test_pre_check_router.py`, 15 tests): +- `_normalise_result` — skip/fire/override cases; oversized message drops with `message_truncated` marker; reason clamped to 2000 chars; non-dict return raises `ValueError`; missing `fire` field raises. +- Router HTTP — 404 when `check()` absent, 200 on skip/fire, 500 when `check()` raises, 500 when `check()` returns non-dict, oversized-message fall-back path, async `check()` awaited correctly. + +Full scheduler + unit suite: 176/176 passing. **Live end-to-end** (verified 2026-04-22 in local Trinity): - Empty scan on `dolho/pr-reviewer-agent` → `fire:false` → skip row in DB, `$0` cost, zero backend activity. diff --git a/tests/unit/test_pre_check_router.py b/tests/unit/test_pre_check_router.py new file mode 100644 index 000000000..ff64be073 --- /dev/null +++ b/tests/unit/test_pre_check_router.py @@ -0,0 +1,208 @@ +""" +Unit tests for the agent-server pre-check router (#454, follow-up to review feedback). + +Covers response normalisation + router behavior without running a real +agent container. Pairs with ``tests/scheduler_tests/test_pre_check.py`` +which covers the scheduler-side integration. +""" +from __future__ import annotations + +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +_BASE_IMAGE = ( + Path(__file__).resolve().parent.parent.parent / "docker" / "base-image" +) +_BASE_IMAGE_STR = str(_BASE_IMAGE) +if _BASE_IMAGE_STR not in sys.path: + sys.path.insert(0, _BASE_IMAGE_STR) + +from fastapi import FastAPI # noqa: E402 +from fastapi.testclient import TestClient # noqa: E402 + +# Import the module directly rather than via the ``routers`` package so we +# don't pull in snapshot.py / git.py, which require python-multipart at +# import time. +import importlib.util # noqa: E402 + +_pre_check_path = _BASE_IMAGE / "agent_server" / "routers" / "pre_check.py" +_spec = importlib.util.spec_from_file_location( + "agent_server_pre_check_router", _pre_check_path +) +_pre_check_mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_pre_check_mod) + +MAX_MESSAGE_BYTES = _pre_check_mod.MAX_MESSAGE_BYTES +_normalise_result = _pre_check_mod._normalise_result +router = _pre_check_mod.router + + +# --------------------------------------------------------------------------- +# Response normalisation +# --------------------------------------------------------------------------- + + +class TestNormaliseResult: + def test_skip_decision_with_reason(self): + out = _normalise_result({"fire": False, "reason": "nothing to do"}) + assert out == {"fire": False, "reason": "nothing to do"} + + def test_fire_decision_with_message(self): + out = _normalise_result({"fire": True, "message": "Run review"}) + assert out == {"fire": True, "message": "Run review"} + + def test_fire_decision_without_message(self): + assert _normalise_result({"fire": True}) == {"fire": True} + + def test_oversized_message_dropped_with_truncation_marker(self): + """Review feedback: silent drop hides template bugs. Expose the drop.""" + oversized = "x" * (MAX_MESSAGE_BYTES + 100) + out = _normalise_result({"fire": True, "message": oversized}) + # Message must not pass through + assert "message" not in out + # Caller gets explicit signal that override was dropped + assert "message_truncated" in out + assert "exceeds" in out["message_truncated"] + assert str(MAX_MESSAGE_BYTES) in out["message_truncated"] + assert out["fire"] is True + + def test_message_exactly_at_cap_is_accepted(self): + msg = "x" * MAX_MESSAGE_BYTES + out = _normalise_result({"fire": True, "message": msg}) + assert out.get("message") == msg + + def test_reason_clamped_to_2000_chars(self): + out = _normalise_result({"fire": False, "reason": "z" * 5000}) + assert len(out["reason"]) == 2000 + + def test_non_dict_return_raises_value_error(self): + """check() returning None/True/etc. must be surfaced as a server + error, not silently normalised away.""" + for bad in (None, True, 42, "hello", [1, 2, 3]): + with pytest.raises(ValueError, match="must return a dict"): + _normalise_result(bad) + + def test_dict_missing_fire_key_raises_value_error(self): + with pytest.raises(ValueError, match="must return a dict"): + _normalise_result({"message": "no fire key"}) + + +# --------------------------------------------------------------------------- +# HTTP router behavior +# --------------------------------------------------------------------------- + + +@pytest.fixture +def app(): + app = FastAPI() + app.include_router(router) + return app + + +class TestRouter: + def test_returns_404_when_check_absent(self, app): + with patch.object( + _pre_check_mod, + "_load_check_callable", + return_value=None, + ): + with TestClient(app) as c: + r = c.post("/api/pre-check") + assert r.status_code == 404 + + def test_returns_200_on_fire_true_with_message(self, app): + def check(): + return {"fire": True, "message": "Review PR #1"} + + with patch.object( + _pre_check_mod, + "_load_check_callable", + return_value=check, + ): + with TestClient(app) as c: + r = c.post("/api/pre-check") + assert r.status_code == 200 + assert r.json() == {"fire": True, "message": "Review PR #1"} + + def test_returns_200_on_fire_false(self, app): + def check(): + return {"fire": False, "reason": "no new PRs"} + + with patch.object( + _pre_check_mod, + "_load_check_callable", + return_value=check, + ): + with TestClient(app) as c: + r = c.post("/api/pre-check") + assert r.status_code == 200 + assert r.json() == {"fire": False, "reason": "no new PRs"} + + def test_returns_500_when_check_raises(self, app): + def check(): + raise RuntimeError("template bug") + + with patch.object( + _pre_check_mod, + "_load_check_callable", + return_value=check, + ): + with TestClient(app) as c: + r = c.post("/api/pre-check") + assert r.status_code == 500 + assert "template bug" in r.json()["detail"] + + def test_returns_500_when_check_returns_non_dict(self, app): + """Review feedback: the ValueError path of _normalise_result was + previously untested end-to-end.""" + + def check(): + return None + + with patch.object( + _pre_check_mod, + "_load_check_callable", + return_value=check, + ): + with TestClient(app) as c: + r = c.post("/api/pre-check") + assert r.status_code == 500 + assert "must return a dict" in r.json()["detail"] + + def test_oversized_message_falls_back_without_override(self, app): + """End-to-end path for the truncation marker — scheduler sees + message_truncated but no `message` key, so falls back to + schedule.message.""" + + def check(): + return {"fire": True, "message": "x" * (MAX_MESSAGE_BYTES + 50)} + + with patch.object( + _pre_check_mod, + "_load_check_callable", + return_value=check, + ): + with TestClient(app) as c: + r = c.post("/api/pre-check") + assert r.status_code == 200 + body = r.json() + assert body["fire"] is True + assert "message" not in body + assert "message_truncated" in body + + def test_async_check_is_awaited(self, app): + async def check(): + return {"fire": True} + + with patch.object( + _pre_check_mod, + "_load_check_callable", + return_value=check, + ): + with TestClient(app) as c: + r = c.post("/api/pre-check") + assert r.status_code == 200 + assert r.json() == {"fire": True} From f3ea04bcd677e90ec3f048addb045329a589fb14 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 22 Apr 2026 16:35:11 +0300 Subject: [PATCH 4/6] refactor(#454): docker exec instead of agent-server HTTP endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #455 flagged that the HTTP-endpoint design introduced a new system edge (scheduler → agent-server direct) and a novel code- loading pattern (importlib in a router). Both broke with Trinity's established convention that all "run something in an agent container" flows go through `services/docker_service.execute_command_in_container` — the same primitive used by: - services/git_service.py (persistent-state allowlist, #384 S3) - services/ssh_service.py (key provisioning) - services/agent_service/terminal.py (web SSH) - routers/system_agent.py (admin exec) - adapters/message_router.py (Slack file ingest) - routers/voice.py, monitoring_service.py This commit swaps the design accordingly. Changes: - Delete docker/base-image/agent_server/routers/pre_check.py and its router registration. No new HTTP surface on agent-server. - Delete tests/unit/test_pre_check_router.py (router is gone). - Add src/backend/routers/internal.py → POST /api/internal/agents/{name}/pre-check. Runs the template-shipped `.trinity/pre-check.py` via execute_command_in_container. Two-step: `test -f` for existence, then `python3 .../pre-check.py`. Returns {hook_present, exit_code, stdout, stderr}. Gated by existing X-Internal-Secret header (C-003). - Rewrite src/scheduler/service.py::_run_pre_check to call the backend endpoint (scheduler no longer opens a direct edge to agent-server). Translates backend response to the same {fire, message, reason} shape so _execute_schedule_with_lock is unchanged. - Delete AgentClient.pre_check from src/scheduler/agent_client.py. - Rewrite tests/scheduler_tests/test_pre_check.py to mock the backend HTTP (httpx.AsyncClient) instead of the agent HTTP client. 13 tests covering translation: hook absent → None, non-zero exit → None, empty stdout → skip, non-empty stdout → fire with override, 404, 5xx, connection error, malformed JSON. - Update docs/memory/architecture.md §Agent Containers and §Background Services, docs/memory/requirements.md SCHED-COND-001, and docs/memory/feature-flows/scheduler-pre-check.md to reflect the new topology. feature-flows.md index updated. Template side (dolho/pr-reviewer-agent commit 4330d2a): .trinity/ pre-check.py rewritten as a standalone shebanged script that prints the chat prompt to stdout or exits 0 empty. Benefits: - Preserves "scheduler → backend → agent" topology (Invariant #11). - Uses Trinity's dominant pattern for agent-container command execution. - No importlib, no module caching, no pydantic contract — exit code + stdout is unambiguous. - Smaller code footprint: internal endpoint is ~50 lines, scheduler translation is ~50 lines, template script is ~50 lines. Test coverage: 162/162 scheduler suite passing (up from 161 — new case for malformed backend JSON). Co-Authored-By: Claude Opus 4.7 (1M context) --- docker/base-image/agent_server/main.py | 2 - .../agent_server/routers/__init__.py | 2 - .../agent_server/routers/pre_check.py | 126 ----------- docs/memory/architecture.md | 5 +- docs/memory/feature-flows.md | 2 +- .../feature-flows/scheduler-pre-check.md | 127 ++++++----- docs/memory/requirements.md | 14 +- src/backend/routers/internal.py | 76 +++++++ src/scheduler/agent_client.py | 42 ---- src/scheduler/service.py | 72 +++++- tests/scheduler_tests/test_pre_check.py | 160 ++++++++------ tests/unit/test_pre_check_router.py | 208 ------------------ 12 files changed, 313 insertions(+), 523 deletions(-) delete mode 100644 docker/base-image/agent_server/routers/pre_check.py delete mode 100644 tests/unit/test_pre_check_router.py diff --git a/docker/base-image/agent_server/main.py b/docker/base-image/agent_server/main.py index ffe88a4f6..e511786d0 100644 --- a/docker/base-image/agent_server/main.py +++ b/docker/base-image/agent_server/main.py @@ -24,7 +24,6 @@ dashboard_router, skills_router, snapshot_router, - pre_check_router, ) from .state import agent_state from .services.trinity_mcp import inject_trinity_mcp_if_configured @@ -61,7 +60,6 @@ app.include_router(dashboard_router) # Dashboard endpoint app.include_router(skills_router) # Skills/playbooks listing endpoint app.include_router(snapshot_router) # Snapshot/restore primitives (#384, S3) -app.include_router(pre_check_router) # Agent-owned pre-check hook (#454) # #389 S1a: auto-sync heartbeat loop (gated by GIT_SYNC_AUTO env var). schedule_auto_sync_if_enabled(app) diff --git a/docker/base-image/agent_server/routers/__init__.py b/docker/base-image/agent_server/routers/__init__.py index 69c0cd962..49890897e 100644 --- a/docker/base-image/agent_server/routers/__init__.py +++ b/docker/base-image/agent_server/routers/__init__.py @@ -11,7 +11,6 @@ from .dashboard import router as dashboard_router from .skills import router as skills_router from .snapshot import router as snapshot_router -from .pre_check import router as pre_check_router __all__ = [ "chat_router", @@ -24,5 +23,4 @@ "dashboard_router", "skills_router", "snapshot_router", - "pre_check_router", ] diff --git a/docker/base-image/agent_server/routers/pre_check.py b/docker/base-image/agent_server/routers/pre_check.py deleted file mode 100644 index 8e7b49de6..000000000 --- a/docker/base-image/agent_server/routers/pre_check.py +++ /dev/null @@ -1,126 +0,0 @@ -""" -Pre-check endpoint (#454). - -Optional hook that lets an agent template deterministically decide whether a -scheduled invocation should actually fire, without waking Claude. The scheduler -calls this endpoint before dispatching a cron-triggered chat; if the endpoint -is absent the scheduler falls back to today's behavior (fire as usual). - -Template authors implement the gate by dropping a Python file at -``/home/developer/.trinity/pre-check.py`` with a top-level ``check()`` function -that returns:: - - {"fire": False, "reason": "..."} - # or - {"fire": True, "message": "optional chat message override"} - -If ``check()`` returns ``fire=False`` the scheduler records a skipped -execution and does not call ``/api/chat``. If it returns ``fire=True`` the -scheduler fires the chat, using the returned ``message`` if present. - -Fail-open by design: any exception or malformed response at this layer -propagates to the scheduler as "no decision", which falls back to the -default fire behavior. A broken pre-check must never silently suppress -scheduled work. - -Security note: ``check()`` is executed with the agent-server's full Python -interpreter access — templates can import ``subprocess``, open sockets, -etc. This is the same sandbox as chat-mode tool calls (non-root container, -dropped capabilities, isolated network), so it introduces no new privilege, -but operators reviewing templates should treat ``.trinity/pre-check.py`` -with the same scrutiny as ``CLAUDE.md`` and any skill files. - -Module-reload caching is intentionally absent: every request re-runs -``exec_module`` so template edits take effect on the next call without -a restart. Given the file is small and the poll cadence is minutes, the -cost is negligible. If polling tightens to sub-second, add mtime-based -caching — see PR #455 review thread for history. -""" -from __future__ import annotations - -import asyncio -import importlib.util -import inspect -import logging -from pathlib import Path -from typing import Any, Dict - -from fastapi import APIRouter, HTTPException - -logger = logging.getLogger(__name__) -router = APIRouter() - -PRE_CHECK_PATH = Path("/home/developer/.trinity/pre-check.py") -MAX_MESSAGE_BYTES = 32_000 - - -def _load_check_callable(): - """Dynamically load ``check`` from the template's pre-check file.""" - if not PRE_CHECK_PATH.exists(): - return None - spec = importlib.util.spec_from_file_location( - "_trinity_pre_check", PRE_CHECK_PATH - ) - if spec is None or spec.loader is None: - return None - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) # may raise — caller logs and returns 500 - fn = getattr(module, "check", None) - if not callable(fn): - return None - return fn - - -def _normalise_result(raw: Any) -> Dict[str, Any]: - if not isinstance(raw, dict) or "fire" not in raw: - raise ValueError( - "pre-check check() must return a dict with a 'fire' bool" - ) - fire = bool(raw.get("fire")) - out: Dict[str, Any] = {"fire": fire} - if not fire: - reason = raw.get("reason") - if reason is not None: - out["reason"] = str(reason)[:2000] - return out - message = raw.get("message") - if message is not None: - message = str(message) - size = len(message.encode("utf-8")) - if size > MAX_MESSAGE_BYTES: - # Silently falling back to schedule.message would hide a real - # template bug. Log loudly and expose the reason in the response - # so the scheduler/operator can see what happened. - logger.error( - "[pre-check] message override %d bytes exceeds cap %d — " - "falling back to schedule.message", - size, - MAX_MESSAGE_BYTES, - ) - out["message_truncated"] = ( - f"override dropped: {size} bytes exceeds {MAX_MESSAGE_BYTES} cap" - ) - else: - out["message"] = message - return out - - -@router.post("/api/pre-check") -async def pre_check() -> Dict[str, Any]: - """Run the template's ``check()`` if present; 404 if absent (fail-open).""" - fn = _load_check_callable() - if fn is None: - raise HTTPException(status_code=404, detail="pre-check not implemented") - try: - if inspect.iscoroutinefunction(fn): - raw = await fn() - else: - raw = await asyncio.get_running_loop().run_in_executor(None, fn) - except Exception as e: - logger.exception("[pre-check] check() raised: %s", e) - raise HTTPException(status_code=500, detail=f"pre-check error: {e}") - try: - return _normalise_result(raw) - except Exception as e: - logger.warning("[pre-check] invalid return shape: %s", e) - raise HTTPException(status_code=500, detail=str(e)) diff --git a/docs/memory/architecture.md b/docs/memory/architecture.md index 3ee6c95ba..72ac5f4f5 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -362,7 +362,8 @@ docker exec trinity-vector sh -c "tail -50 /data/logs/agents.json" | jq . - `/api/chat/session` - Context window stats - `/api/files` - List workspace files (recursive tree structure) - `/api/files/download` - Download file content (100MB limit) -- `/api/pre-check` - Optional agent-owned gate for scheduled invocations (SCHED-COND-001). If the template ships `~/.trinity/pre-check.py` with a `check()` function, the scheduler calls this endpoint before firing a cron-triggered chat; `fire=False` → scheduler records a skipped execution and does NOT wake Claude. Endpoint absent → scheduler falls back to normal cron behavior (backward compat). + +**Template-supplied pre-check** (optional, SCHED-COND-001): if the template ships `~/.trinity/pre-check.py`, the backend's internal endpoint `POST /api/internal/agents/{name}/pre-check` executes it via `docker exec` before the scheduler fires a cron-triggered chat. The script's stdout becomes the chat message; empty stdout + exit 0 records a skipped execution. No HTTP endpoint is exposed on the agent-server for this — the primitive is the same `execute_command_in_container` already used by `services/git_service.py` (persistent-state allowlist), `ssh_service.py`, and the agent terminal. **Persistent Chat:** - All chat messages automatically saved to SQLite (`chat_sessions`, `chat_messages`) @@ -396,7 +397,7 @@ Services that run continuously in the backend process: | **Operator Queue Sync** | `operator_queue_service.py` | Polls running agents every 5s, reads `~/.trinity/operator-queue.json`, syncs to DB, writes responses back. (OPS-001) | | **Sync Health Service** | `sync_health_service.py` | Polls git-enabled agents every 60s, upserts `agent_sync_state`, emits `sync_failing` operator-queue entries when consecutive_failures ≥ 3. (#389 S1) | | **Monitoring Service** | `monitoring_service.py` | Fleet-wide health checks on configurable interval. (MON-001) | -| **Scheduler Service** | `scheduler_service.py` | APScheduler-based cron job execution. Async fire-and-forget with DB polling for status. On each cron-triggered fire, optionally calls the target agent's `POST /api/pre-check` hook; `fire=False` records a skipped execution and does not invoke Claude (SCHED-COND-001, #454). | +| **Scheduler Service** | `scheduler_service.py` | APScheduler-based cron job execution. Async fire-and-forget with DB polling for status. On each cron-triggered fire, optionally invokes the agent's `~/.trinity/pre-check.py` via the backend's `POST /api/internal/agents/{name}/pre-check` (which `docker exec`s into the agent container). Empty stdout + exit 0 records a skipped execution and does not invoke Claude (SCHED-COND-001, #454). | | **Capacity Maintenance** | `capacity_manager.py` | Calls `CapacityManager.run_maintenance()` every 60s — expires stale queued tasks (>24h) and drains orphans after restart. (BACKLOG-001 / CAPACITY-CONSOLIDATE #428) | The **agent server** also runs a 15-min `auto_sync` heartbeat loop (gated diff --git a/docs/memory/feature-flows.md b/docs/memory/feature-flows.md index 35db77323..26144d201 100644 --- a/docs/memory/feature-flows.md +++ b/docs/memory/feature-flows.md @@ -19,7 +19,7 @@ | 2026-04-25 | #487 | Telegram file upload Phase 2 — workspace delivery hardening: NFKC sanitizer with collision dedup, spec injection format `[File uploaded by {uploader}]: {name} ({size}) saved to {path}`, all-writes-failed channel error + abort. Same code path benefits Slack inbound. | [telegram-integration.md](feature-flows/telegram-integration.md), [slack-file-sharing.md](feature-flows/slack-file-sharing.md) | | 2026-04-23 | #476 | SQLite lexicographic cutoff bug fix — new `iso_cutoff(hours)` helper replaces `datetime('now', ...)` in 15 sites across rate-limit / dashboard / schedules; `max_retries` default flipped `1 → 0`; `cleanup_old_rate_limit_events` wired into `CleanupService` (phase 6, hourly) | [subscription-auto-switch.md](feature-flows/subscription-auto-switch.md), [cleanup-service.md](feature-flows/cleanup-service.md), [scheduler-service.md](feature-flows/scheduler-service.md) | | 2026-04-22 | #458 | `.gitignore` init fix — `initialize_git_in_container` now appends missing patterns instead of truncate-and-write; adds `.env`, `.env.*`, `.mcp.json` to the default list and runs for both `/home/developer` and legacy `/home/developer/workspace` (stops credential leak on first GitHub sync) | [github-repo-initialization.md](feature-flows/github-repo-initialization.md) | -| 2026-04-22 | SCHED-COND-001 (#454) | Conditional schedule pre-check — agent-owned `POST /api/pre-check` hook lets templates deterministically skip scheduled ticks (zero Claude tokens on empty polls); fail-open; reuses `ExecutionStatus.SKIPPED` (no schema change) | [scheduler-pre-check.md](feature-flows/scheduler-pre-check.md) | +| 2026-04-22 | SCHED-COND-001 (#454) | Conditional schedule pre-check — backend `docker exec`s template-supplied `~/.trinity/pre-check.py` before scheduler fires a cron chat; empty stdout + exit 0 records a skipped execution; fail-open; reuses `ExecutionStatus.SKIPPED` (no schema change, no HTTP edge from scheduler to agent) | [scheduler-pre-check.md](feature-flows/scheduler-pre-check.md) | | 2026-04-21 | RELIABILITY-003 (#306) | WebSocket event bus on Redis Streams — replaces in-process broadcast with XADD/XREAD, adds reconnect replay via `?last-event-id=`, 3-failure eviction, MAXLEN trim (tunable) | [websocket-event-bus.md](feature-flows/websocket-event-bus.md) | | 2026-04-20 | #420 | Scheduler sync loop fix — `update_schedule_run_times` no longer bumps `updated_at`, stopping the self-triggering re-register of every schedule per tick | [scheduler-service.md](feature-flows/scheduler-service.md) | | 2026-04-20 | #418 | Inter-agent timeout honors per-agent `execution_timeout_seconds` — removed 600s hardcoded defaults in MCP `chat_with_agent`/`fan_out` tools and fan-out service; HTTP client ceiling bumped to platform max (7200s) | [fan-out.md](feature-flows/fan-out.md), [mcp-orchestration.md](feature-flows/mcp-orchestration.md), [parallel-headless-execution.md](feature-flows/parallel-headless-execution.md) | diff --git a/docs/memory/feature-flows/scheduler-pre-check.md b/docs/memory/feature-flows/scheduler-pre-check.md index 6a88a3a08..9cd1cbb78 100644 --- a/docs/memory/feature-flows/scheduler-pre-check.md +++ b/docs/memory/feature-flows/scheduler-pre-check.md @@ -1,44 +1,47 @@ # Feature: Conditional Schedule Pre-Check (SCHED-COND-001 / Issue #454) ## Overview -Optional agent-owned hook that lets a scheduled cron tick be skipped deterministically without waking Claude. The scheduler calls `POST /api/pre-check` on the target agent before firing a cron-triggered chat; `fire=False` records a skipped execution with zero Claude token cost. Eliminates per-tick token burn on poll-driven agents (PR reviewers, inbox monitors, alert routers). +Optional template-supplied hook that lets a scheduled cron tick be skipped deterministically without waking Claude. Before firing a cron-triggered chat, the scheduler calls a **backend** internal endpoint, which `docker exec`s `~/.trinity/pre-check.py` inside the target agent container. Non-empty stdout becomes the chat prompt; empty stdout + exit 0 records a skipped execution. Eliminates per-tick token burn on poll-driven agents (PR reviewers, inbox monitors, alert routers). ## User Story -As the author of a poll-driven agent template, I want my agent's own code to decide whether each scheduled tick actually needs to run, so that cheap deterministic checks (scan GitHub, list unread mail, compare cost threshold) replace a full Claude turn on every empty poll. +As the author of a poll-driven agent template, I want a cheap deterministic gate to run before Claude wakes so empty polls (scan→no work) don't burn tokens. As a Trinity operator, I don't want to configure the gate per schedule — the agent template owns it, I just schedule the cadence. ## Entry Points -- **Agent container endpoint** (new): `POST http://agent-:8000/api/pre-check` — internal network only; called by the scheduler service. -- **Template contract**: drop `~/.trinity/pre-check.py` with a top-level `def check()` returning `{"fire": bool, "message": str?, "reason": str?}`. +- **Template contract**: ship `~/.trinity/pre-check.py` as a standalone executable script. Prints chat prompt to stdout when work is found; exits 0 with empty stdout to skip; exits non-zero on error (fail-open). +- **Backend endpoint** (internal, `X-Internal-Secret` auth): `POST /api/internal/agents/{name}/pre-check` — runs the script and returns stdout + exit code. Called only by `trinity-scheduler`. - **No operator-facing API change**: schedule CRUD endpoints and the Schedules UI are unchanged. Operators create normal cron schedules; agents own the gate. ## Frontend Layer -No UI change. Skipped executions appear in the existing schedule executions list alongside `success`/`failed` rows — visible immediately because the frontend already renders the `status` field as a badge. +No UI change. Skipped executions appear in the existing schedule executions list alongside `success`/`failed` rows — the frontend already renders the `status` field as a badge. -## Agent Server Layer -**Router**: `docker/base-image/agent_server/routers/pre_check.py` +## Backend Layer +**Router**: `src/backend/routers/internal.py` — `POST /api/internal/agents/{name}/pre-check` -- Dynamically loads `/home/developer/.trinity/pre-check.py` via `importlib.util.spec_from_file_location`. No caching beyond Python's default `sys.modules` behavior. -- Returns 404 if the file is absent (fail-open signal to scheduler). -- Accepts both sync and async `check()` functions (`inspect.iscoroutinefunction` branch). -- Normalises the return value: clamps `reason` to 2000 chars; requires `fire` key or raises 500. If `check()` returns a non-dict (e.g. `None`, `True`), the normaliser raises `ValueError` which surfaces as a 500 — scheduler treats that as fail-open. -- **Oversized `message` override**: if `message` exceeds 32 KB UTF-8, the router drops it from the response and sets a sibling `message_truncated: "override dropped: N bytes exceeds 32000 cap"` key. The scheduler sees no `message`, falls back to `schedule.message`, and operators can inspect the truncated-reason in agent-server logs (logged at `ERROR`). Templates must produce compact prompts or rely on `schedule.message`. -- Any exception in `check()` → 500 with the exception text. Scheduler treats 500 as fail-open. +Uses `services/docker_service.execute_command_in_container` (the same primitive as `services/git_service.py` persistent-state allowlist, `services/ssh_service.py` key provisioning, `services/agent_service/terminal.py`, `routers/system_agent.py`, `adapters/message_router.py` Slack ingest, etc.). -**Registration**: `docker/base-image/agent_server/routers/__init__.py` and `main.py` mount `pre_check_router` alongside existing routers (chat, files, git, skills, dashboard). +Two exec steps: +1. `test -f /home/developer/.trinity/pre-check.py` (5s timeout). If the file doesn't exist, return `{"hook_present": False}` immediately — scheduler treats as "no decision, fire as usual." +2. `python3 /home/developer/.trinity/pre-check.py` (60s timeout). Returns the output (capped at 32 KB) and exit code. -## Scheduler Layer -**Client**: `src/scheduler/agent_client.py` +Returns: +```json +{"hook_present": true, "exit_code": 0, "stdout": "Review PR #1\n", "stderr": ""} +``` -New `AgentClient.pre_check(timeout=60.0) -> Optional[dict]`: -- 404 / 5xx / timeout / malformed JSON / missing `fire` field → returns `None`. -- Valid 200 → returns decision dict. -- `None` signals "no decision, fire as usual" (fail-open). +## Scheduler Layer +**Service**: `src/scheduler/service.py` — `_run_pre_check(agent_name)` -**Service**: `src/scheduler/service.py` +Calls the backend's internal endpoint (not the agent directly — topology stays "scheduler → backend → agent"). Translates the backend response into a scheduler decision: -New `_run_pre_check(agent_name)` wraps the client call with a broad `except Exception` — any unexpected error also returns `None`. +| Backend response | Scheduler decision | +|---|---| +| `hook_present: false` | `None` → fire as usual (backward compat for templates without a hook) | +| `exit_code != 0` | `None` → fail-open + log stderr (broken hook must not suppress work) | +| `exit_code == 0`, empty stdout | `{"fire": False, "reason": "pre-check returned empty stdout"}` → record skipped execution | +| `exit_code == 0`, non-empty stdout | `{"fire": True, "message": stdout.strip()}` → fire with stdout as override | +| HTTP error / malformed JSON / timeout | `None` → fail-open + log | -Intercept point: `_execute_schedule_with_lock` calls `_run_pre_check` only when `triggered_by == "schedule"` (cron). Manual triggers bypass entirely. +Intercept point in `_execute_schedule_with_lock`: called only when `triggered_by == "schedule"` (cron). Manual triggers bypass entirely. ```python effective_message = schedule.message @@ -46,25 +49,24 @@ if triggered_by == "schedule": decision = await self._run_pre_check(schedule.agent_name) if decision is not None: if not decision.get("fire", True): - skipped = self.db.create_skipped_execution(...) - # publish event, update run times, return — do not create execution + self.db.create_skipped_execution(...) + self._publish_event({"type": "schedule_execution_skipped", ...}) return override = decision.get("message") - if override and isinstance(override, str): + if override: effective_message = override +# ... fires with effective_message ``` -`effective_message` is then threaded through `create_execution()` and `_call_backend_execute_task()`. - ## Data Layer **Zero schema change.** Reuses existing: -- `ExecutionStatus.SKIPPED` (already defined in `src/scheduler/models.py` for Issue #46's max-instances drop handling). -- `SchedulerDatabase.create_skipped_execution(...)` (already written to record APScheduler-dropped runs). +- `ExecutionStatus.SKIPPED` (already defined for Issue #46 — APScheduler max-instances drops). +- `SchedulerDatabase.create_skipped_execution(...)`. -Skip rows carry `status='skipped'`, `error=f"pre-check: {reason}"`, `duration_ms=0`, `started_at == completed_at`. +Skip rows carry `status='skipped'`, `error="pre-check: "`, `duration_ms=0`, `started_at == completed_at`. ## WebSocket Layer -New event type: `schedule_execution_skipped` +New event type: `schedule_execution_skipped`. ```json { @@ -73,59 +75,62 @@ New event type: `schedule_execution_skipped` "schedule_id": "LidXcOwtDsDuFTFGvkUqCw", "execution_id": "I2DWodfpYpuJbs1TTZ42Ig", "schedule_name": "PR review poll", - "reason": "no new PRs" + "reason": "pre-check: pre-check returned empty stdout" } ``` ## Side Effects -- Skipped execution row written to `schedule_executions` table (visible in UI immediately) +- Skipped execution row written to `schedule_executions` - `schedule.last_run_at` and `next_run_at` updated (so missed-schedule detection still works) -- WebSocket event broadcast to any subscribed UIs +- WebSocket event broadcast - No `/api/internal/execute-task` call, no backend task creation, no Claude invocation, no backlog slot acquisition ## Error Handling | Condition | Scheduler behavior | |---|---| -| Agent container unreachable | Fires as usual (fail-open); logs warning | -| `/api/pre-check` returns 404 | Fires as usual (backward compat for templates without a hook) | -| `/api/pre-check` returns 5xx | Fires as usual; logs warning | -| Response times out | Fires as usual; logs warning | -| Response missing `fire` field | Fires as usual; logs warning | -| `fire=false` | Records skipped execution, no chat dispatch | -| `fire=true` with `message` | Fires chat with override message | -| `fire=true` without `message` | Fires chat with `schedule.message` (existing behavior) | -| Manual trigger (`triggered_by != 'schedule'`) | Skips pre-check entirely — explicit operator intent always fires | +| Agent container doesn't exist | Backend returns 404 → fire as usual (schedule likely stale, let execute-task path handle the 404 surfacing) | +| `~/.trinity/pre-check.py` absent | `hook_present: false` → fire as usual (backward compat) | +| `python3 .../pre-check.py` exits non-zero | Fail-open — log stderr, fire with `schedule.message` | +| Exec timeout (>60s) | Backend returns non-zero exit → fail-open | +| Backend unreachable (connection error) | Fail-open — fire as usual, log warning | +| Backend 5xx / malformed JSON | Fail-open | +| Exit 0, empty stdout | Record skipped execution, no chat dispatch | +| Exit 0, non-empty stdout | Fire with stdout as chat message (overrides `schedule.message`) | +| Manual trigger (`triggered_by != 'schedule'`) | Skip pre-check entirely — explicit operator intent always fires | ## Security - Pre-check runs inside the agent's container as `developer`, same sandbox as chat-mode tool calls. No new privilege over what chat-mode tool calls can already do. -- **Template review expectation**: `~/.trinity/pre-check.py` is executed with full Python interpreter access — it can `import subprocess`, open sockets, read files. This is intentional (operators already trust the template's `CLAUDE.md`, skills, and tool invocations), but `.trinity/pre-check.py` should be reviewed with the same scrutiny as any other executable file the template ships. -- Stdout/message cap at 32 KB UTF-8 — oversized payloads are dropped from the response and the drop is surfaced via a `message_truncated` field + `ERROR` log, not silently swallowed. +- **Template review expectation**: `.trinity/pre-check.py` is executed with full Python interpreter access — it can `import subprocess`, open sockets, read files. This is intentional (operators already trust the template's `CLAUDE.md`, skills, and tool invocations), but `.trinity/pre-check.py` should be reviewed with the same scrutiny as any other executable file the template ships. +- Backend endpoint is gated by the existing `X-Internal-Secret` header (C-003). Only `trinity-scheduler` and other internal services can invoke it. +- Stdout cap at 32 KB on the backend side — oversized output is truncated, still valid as a chat prompt (or truncated to "looks non-empty" which is fine for the fire-with-override path). - Fail-open policy means a malicious/broken pre-check cannot suppress scheduled invocations (worst case: wastes tokens — today's baseline). -- Invariant #5 ("Agent Server Mirrors Backend") preserved: agent exposes an HTTP contract, scheduler proxies to it. -- Invariant #1 ("Three-layer backend: router → service → db") preserved on the scheduler side — CLI wrapper → service → database. ## Testing -**Scheduler-side** (`tests/scheduler_tests/test_pre_check.py`, 12 tests): -- `AgentClient.pre_check` — returns decision on 200, None on 404, None on 5xx, None on unreachable, None on malformed JSON, None on missing `fire` field, normal pass on fire-false, normal pass on fire-true-with-message. -- `SchedulerService._execute_schedule_with_lock` — skip records execution with correct reason, fire-true with message uses override, fail-open routes through backend, fire-true without message uses schedule.message, manual trigger bypasses pre-check. +**Scheduler-side** (`tests/scheduler_tests/test_pre_check.py`, 13 tests): +- `_run_pre_check` translation — `hook_present: False` → None; non-zero exit → None; empty stdout → skip; non-empty stdout → fire with message; 404 / 5xx / connection error / malformed JSON all → None (fail-open). +- `_execute_schedule_with_lock` branch — skip records execution with correct reason; fire-true with message uses override; fail-open routes through backend; fire-true without message uses `schedule.message`; manual trigger bypasses pre-check. -**Agent-server router** (`tests/unit/test_pre_check_router.py`, 15 tests): -- `_normalise_result` — skip/fire/override cases; oversized message drops with `message_truncated` marker; reason clamped to 2000 chars; non-dict return raises `ValueError`; missing `fire` field raises. -- Router HTTP — 404 when `check()` absent, 200 on skip/fire, 500 when `check()` raises, 500 when `check()` returns non-dict, oversized-message fall-back path, async `check()` awaited correctly. +Full scheduler suite: 162/162 passing (was 161 before; +1 for the new `malformed JSON` case). -Full scheduler + unit suite: 176/176 passing. +No test file on the agent-server side — there's no agent-server router anymore. -**Live end-to-end** (verified 2026-04-22 in local Trinity): -- Empty scan on `dolho/pr-reviewer-agent` → `fire:false` → skip row in DB, `$0` cost, zero backend activity. -- Open PR on that repo → next tick `fire:true`, override message with PR list → Claude runs `/review`, posts comment. -- Subsequent tick with existing bot comment → `fire:false` again (stateless dedup via GitHub-hosted comment thread). +**Live end-to-end** (verified 2026-04-22 in local Trinity on the HTTP-endpoint version before pivoting to docker-exec): +- Empty scan → skip row in DB, `$0` cost, zero backend chat activity. +- Open PR → next tick fires with override message, Claude runs `/review`, posts comment. +- Subsequent tick with existing bot comment → `fire:false` again (stateless dedup via GitHub). ## Related Flows - `feature-flows/agent-event-subscriptions.md` — EVT-001, the event-driven analogue (other trigger source, same "non-cron agent invocation" theme). - `feature-flows/scheduler-service.md` — base scheduler behavior this extends. - `docs/planning/PR_REVIEWER_AGENT.md` — the motivating use case that drove this feature. +## Architectural notes +- Preserves **Invariant #11 (Docker as source of truth)**: the pre-check primitive is `docker exec`, matching `git_service.py`'s persistent-state allowlist, `ssh_service.py`, the agent terminal, etc. +- Preserves the **"scheduler → backend → agent"** topology: scheduler never opens a direct HTTP edge to agent-server containers. +- Reuses **`execute_command_in_container`**, an established async helper in `services/docker_service.py`. +- No new schema, no new long-lived process, no new primitive — just a new internal endpoint and a scheduler branch. + ## Migration / Rollout - Zero migration required (no schema change). -- Existing schedules and agent templates behave identically after deploy (endpoint absent → fall back to today's fire semantics). -- Templates opt in by shipping `~/.trinity/pre-check.py`. No Trinity-side flag. +- Existing schedules and agent templates behave identically after deploy (script absent → fall back to today's fire semantics). +- Templates opt in by shipping `.trinity/pre-check.py` (executable, `+x`). No Trinity-side flag. diff --git a/docs/memory/requirements.md b/docs/memory/requirements.md index 24f139d8c..4b040f0bb 100644 --- a/docs/memory/requirements.md +++ b/docs/memory/requirements.md @@ -377,15 +377,17 @@ Trinity is autonomous agent orchestration and infrastructure — sovereign infra - **Status**: ✅ Implemented (2026-04-22) - **Requirement ID**: SCHED-COND-001 - **GitHub Issue**: #454 -- **Description**: Optional agent-owned hook that lets a scheduled cron tick be skipped deterministically — scheduler calls `POST /api/pre-check` on the target agent before firing a chat and records a skipped execution when the agent returns `fire=false`. Eliminates Claude token cost on empty polls for poll-driven agents (PR reviewers, inbox monitors, alert routers, RSS watchers). +- **Description**: Optional template-supplied hook that lets a scheduled cron tick be skipped deterministically — the scheduler calls a new internal backend endpoint which `docker exec`s `~/.trinity/pre-check.py` inside the target agent container; non-empty stdout becomes the chat prompt, empty stdout + exit 0 records a skipped execution. Eliminates Claude token cost on empty polls for poll-driven agents (PR reviewers, inbox monitors, alert routers, RSS watchers). - **Key Features**: - - Contract: agent templates drop a `~/.trinity/pre-check.py` with a top-level `check()` returning `{"fire": bool, "message": str?, "reason": str?}`. Base-image router at `POST /api/pre-check` invokes it. - - Fail-open: endpoint absent (404), timeout, 5xx, malformed response → scheduler fires as usual. A broken pre-check never silently suppresses scheduled work. - - Message override: `fire=true` with a `message` field replaces `schedule.message` for that one invocation — lets the agent inject real work items (e.g. the PR list) into the chat prompt. - - Skip record: `fire=false` writes a row to `schedule_executions` with `status='skipped'`, reason, and zero cost — visible in the Trinity UI alongside successful runs. + - Contract: agent templates drop an executable `~/.trinity/pre-check.py` (shebanged standalone script). Stdout is the chat prompt; empty stdout + exit 0 = skip; non-zero exit = fail-open. + - Backend endpoint: `POST /api/internal/agents/{name}/pre-check` (X-Internal-Secret gated) runs the script via `execute_command_in_container` — the same primitive used by `git_service.py` (persistent-state allowlist, #384 S3), `ssh_service.py`, `agent_service/terminal.py`, `adapters/message_router.py`, `routers/system_agent.py`, `routers/voice.py`. + - Fail-open: script absent, non-zero exit, timeout, backend 5xx / malformed response → scheduler fires as usual. A broken pre-check never silently suppresses scheduled work. + - Message override: non-empty stdout replaces `schedule.message` for that one invocation — lets the agent inject real work items (e.g. the PR list) into the chat prompt. + - Skip record: empty stdout writes a row to `schedule_executions` with `status='skipped'`, reason, and zero cost — visible in the Trinity UI alongside successful runs. - Manual triggers bypass pre-check entirely (explicit operator intent always fires). - Zero DB schema change (reuses existing `ExecutionStatus.SKIPPED` + `create_skipped_execution`). -- **Test plan**: 12 unit tests covering client response shapes (200/404/5xx/timeout/malformed) + scheduler branch behaviors (skip, override, fail-open, manual-bypass). Full 161-test scheduler suite passes. + - **No new HTTP edge**: scheduler calls backend, backend `docker exec`s into agent. Topology stays "scheduler → backend → agent" (Invariant #11). +- **Test plan**: 13 unit tests covering backend-response translation (hook absent / non-zero exit / empty stdout / fire-with-message / 404 / 5xx / connection error / malformed JSON) + scheduler branch behaviors (skip, override, fail-open, manual-bypass). Full 162-test scheduler suite passes. - **Root Cause**: No platform primitive for "deterministic gate before LLM invocation." Previously required per-template daemons backgrounded inside agent containers — invisible to Trinity UI, reimplemented per template, no skip metrics. ### 10.7 Per-Agent Execution Timeout (TIMEOUT-001) diff --git a/src/backend/routers/internal.py b/src/backend/routers/internal.py index 5a2a0f37f..57dcec1d6 100644 --- a/src/backend/routers/internal.py +++ b/src/backend/routers/internal.py @@ -59,6 +59,82 @@ async def internal_health(): return {"status": "ok"} +# --------------------------------------------------------------------------- +# Scheduler pre-check (#454, SCHED-COND-001) +# --------------------------------------------------------------------------- + +# Cap stdout/stderr to keep responses bounded. Stdout becomes a chat prompt; +# 32 KB is plenty even for verbose scan output. +_PRE_CHECK_STDOUT_CAP = 32_000 +_PRE_CHECK_STDERR_CAP = 4_000 +_PRE_CHECK_HOOK_PATH = "/home/developer/.trinity/pre-check.py" +_PRE_CHECK_TIMEOUT_SECONDS = 60 + + +@router.post("/agents/{agent_name}/pre-check") +async def internal_agent_pre_check(agent_name: str): + """Run the agent's optional pre-check hook (SCHED-COND-001 / #454). + + Called by the dedicated scheduler before firing a cron-triggered task. + Executes ``~/.trinity/pre-check.py`` inside the agent container via + ``docker exec`` (the same primitive used by ``services/git_service.py`` + for the persistent-state allowlist, by ``ssh_service`` for key + provisioning, etc.). + + Contract: + - ``hook_present == False`` → scheduler treats as no decision and + fires as usual (backward compat for templates without a hook). + - ``hook_present == True``, ``exit_code != 0`` → fail-open: scheduler + logs the stderr and fires as usual. + - ``hook_present == True``, ``exit_code == 0``, empty stdout → + scheduler records a skipped execution. + - ``hook_present == True``, ``exit_code == 0``, non-empty stdout → + scheduler fires with the stdout used as the chat message + (overrides ``schedule.message``). + + Fail-open is structural: any error here must not silently suppress + scheduled work. Worst case is today's baseline (chat fires, tokens + burn). + """ + from services.docker_service import ( + execute_command_in_container, + get_agent_container, + ) + + container = get_agent_container(agent_name) + if not container: + raise HTTPException(status_code=404, detail="Agent not found") + + container_name = f"agent-{agent_name}" + + # Step 1: fast existence check. If the template ships no hook, return + # immediately so the scheduler can fire as usual without spinning up + # python3 inside the container. + exists = await execute_command_in_container( + container_name=container_name, + command=f"test -f {_PRE_CHECK_HOOK_PATH}", + timeout=5, + ) + if exists.get("exit_code") != 0: + return {"hook_present": False} + + # Step 2: run the hook. Stdout becomes the chat prompt (or empty = skip). + result = await execute_command_in_container( + container_name=container_name, + command=f"python3 {_PRE_CHECK_HOOK_PATH}", + timeout=_PRE_CHECK_TIMEOUT_SECONDS, + ) + output = (result.get("output") or "")[: _PRE_CHECK_STDOUT_CAP + _PRE_CHECK_STDERR_CAP] + return { + "hook_present": True, + "exit_code": int(result.get("exit_code", 1)), + # `output` from container_exec_run is the combined stream — keep both + # fields for forward-compat in case we split them later. + "stdout": output[:_PRE_CHECK_STDOUT_CAP], + "stderr": "", + } + + @router.get("/agents/{agent_name}/sync-health-status") async def internal_agent_sync_health(agent_name: str): """#389: lightweight read used by the dedicated scheduler before dispatching. diff --git a/src/scheduler/agent_client.py b/src/scheduler/agent_client.py index 5ea871b16..61e927a30 100644 --- a/src/scheduler/agent_client.py +++ b/src/scheduler/agent_client.py @@ -152,48 +152,6 @@ async def task( result = response.json() return self._parse_task_response(result) - async def pre_check(self, timeout: float = 60.0) -> Optional[Dict[str, Any]]: - """ - Ask the agent whether a scheduled invocation should actually fire (#454). - - Returns: - None if the endpoint is absent (404) or the call fails for any - reason. The scheduler treats this as "no decision" and falls back - to normal firing (fail-open). - - Otherwise a dict with at least ``fire: bool`` and optionally - ``reason`` (str) and ``message`` (str — chat override). - """ - try: - response = await self._request( - "POST", "/api/pre-check", timeout=timeout, json={} - ) - except AgentClientError as e: - logger.info( - f"[pre-check] agent {self.agent_name} unreachable: {e} — fail-open" - ) - return None - if response.status_code == 404: - return None - if response.status_code >= 400: - logger.warning( - f"[pre-check] agent {self.agent_name} returned {response.status_code} — fail-open" - ) - return None - try: - data = response.json() - except Exception as e: - logger.warning( - f"[pre-check] agent {self.agent_name} invalid JSON ({e}) — fail-open" - ) - return None - if not isinstance(data, dict) or "fire" not in data: - logger.warning( - f"[pre-check] agent {self.agent_name} malformed response — fail-open" - ) - return None - return data - async def health_check(self, timeout: float = 5.0) -> bool: """ Check if agent is healthy and responding. diff --git a/src/scheduler/service.py b/src/scheduler/service.py index 3602665ab..7165996f3 100644 --- a/src/scheduler/service.py +++ b/src/scheduler/service.py @@ -899,23 +899,77 @@ async def _execute_schedule_with_lock(self, schedule_id: str, triggered_by: str }) async def _run_pre_check(self, agent_name: str) -> Optional[dict]: - """Call the agent's ``/api/pre-check`` hook (#454). - - Returns the agent's decision dict, or ``None`` if the agent has no - hook or the call fails. ``None`` signals the caller to fire the - schedule as usual (fail-open). + """Run the agent's optional pre-check hook (#454, SCHED-COND-001). + + Calls the backend's internal endpoint, which executes + ``~/.trinity/pre-check.py`` inside the agent container via + ``docker exec`` (same primitive as the persistent-state allowlist + in ``services/git_service.py``). The scheduler never opens its + own HTTP edge to agents — backend remains the orchestrator. + + Contract translation (backend → caller): + - ``hook_present == False`` → return ``None`` (fire as usual) + - ``exit_code != 0`` → return ``None`` (fail-open + log) + - ``exit_code == 0`` & empty stdout → return ``{"fire": False, "reason": ...}`` + - ``exit_code == 0`` & non-empty stdout → return ``{"fire": True, "message": stdout}`` + + Returning ``None`` means "no decision, fire the schedule as today." + Fail-open is structural — a broken hook or unreachable backend must + never silently suppress scheduled work. """ - from .agent_client import AgentClient + headers = {} + if config.internal_api_secret: + headers["X-Internal-Secret"] = config.internal_api_secret + + try: + async with httpx.AsyncClient() as client: + response = await client.post( + f"{config.backend_url}/api/internal/agents/{agent_name}/pre-check", + headers=headers, + timeout=70.0, # agent-side timeout is 60s, give us headroom + ) + except Exception as e: + logger.warning( + f"[pre-check] backend call for {agent_name} failed ({e}) — fail-open" + ) + return None + + if response.status_code == 404: + logger.warning( + f"[pre-check] backend says agent {agent_name} not found — fail-open" + ) + return None + if response.status_code != 200: + logger.warning( + f"[pre-check] backend returned {response.status_code} for {agent_name} — fail-open" + ) + return None try: - client = AgentClient(agent_name, timeout=60.0) - return await client.pre_check() + data = response.json() except Exception as e: logger.warning( - f"[pre-check] unexpected error for {agent_name}: {e} — fail-open" + f"[pre-check] malformed backend response for {agent_name} ({e}) — fail-open" ) return None + if not data.get("hook_present"): + return None # template has no hook — backward compat + + exit_code = data.get("exit_code", 1) + if exit_code != 0: + stderr = (data.get("stderr") or data.get("stdout") or "")[:500] + logger.warning( + f"[pre-check] hook for {agent_name} exited {exit_code}: " + f"{stderr!r} — fail-open" + ) + return None + + stdout = (data.get("stdout") or "").strip() + if not stdout: + return {"fire": False, "reason": "pre-check returned empty stdout"} + return {"fire": True, "message": stdout} + async def _call_backend_execute_task( self, agent_name: str, diff --git a/tests/scheduler_tests/test_pre_check.py b/tests/scheduler_tests/test_pre_check.py index b6176a122..83e8ab2b6 100644 --- a/tests/scheduler_tests/test_pre_check.py +++ b/tests/scheduler_tests/test_pre_check.py @@ -1,11 +1,14 @@ """ -Tests for the agent-owned pre-check hook (#454). +Tests for the conditional schedule pre-check hook (#454, SCHED-COND-001). Covers: -- AgentClient.pre_check returns dict on 200, None on 404, None on error -- SchedulerService skips execution when pre_check returns fire=False -- SchedulerService uses override message when pre_check returns fire=True with message -- SchedulerService falls through to normal fire when pre_check returns None (fail-open) +- `_run_pre_check` translates backend `docker exec` responses correctly: + hook absent → None, non-zero exit → None, empty stdout → skip, + non-empty stdout → fire with message override. +- `_execute_schedule_with_lock` honours the translated decision: skips + when `fire=False`, fires with override when `fire=True` carries a + message, fail-opens when `_run_pre_check` returns None, bypasses + pre-check entirely for manual triggers. """ # Path setup must happen before scheduler imports @@ -21,85 +24,114 @@ import pytest -from scheduler.agent_client import AgentClient, AgentNotReachableError from scheduler.models import ExecutionStatus # --------------------------------------------------------------------------- -# AgentClient.pre_check unit tests +# _run_pre_check translation layer (backend JSON → scheduler decision) # --------------------------------------------------------------------------- -class TestAgentClientPreCheck: +def _build_svc(db_with_data): + """SchedulerService instance with just the dependencies _run_pre_check needs.""" + from scheduler.service import SchedulerService + + svc = SchedulerService.__new__(SchedulerService) + svc.db = db_with_data + return svc + + +def _mock_httpx_post(response_json=None, status_code=200, raise_exc=None): + """Build the `async with httpx.AsyncClient() as client` patch target.""" + response = MagicMock() + response.status_code = status_code + if response_json is not None: + response.json = MagicMock(return_value=response_json) + client_ctx = AsyncMock() + if raise_exc is not None: + client_ctx.__aenter__.return_value.post = AsyncMock(side_effect=raise_exc) + else: + client_ctx.__aenter__.return_value.post = AsyncMock(return_value=response) + return patch("scheduler.service.httpx.AsyncClient", return_value=client_ctx) + + +class TestRunPreCheckTranslation: @pytest.mark.asyncio - async def test_returns_decision_on_200(self): - client = AgentClient("pr-reviewer") - response = MagicMock() - response.status_code = 200 - response.json.return_value = {"fire": True, "message": "review this"} - with patch.object(client, "_request", AsyncMock(return_value=response)): - result = await client.pre_check() - assert result == {"fire": True, "message": "review this"} + async def test_no_hook_returns_none(self, db_with_data): + svc = _build_svc(db_with_data) + with _mock_httpx_post({"hook_present": False}): + decision = await svc._run_pre_check("test-agent") + assert decision is None @pytest.mark.asyncio - async def test_returns_none_on_404(self): - """Endpoint absent — scheduler should fall back to normal fire.""" - client = AgentClient("no-hook-agent") - response = MagicMock() - response.status_code = 404 - with patch.object(client, "_request", AsyncMock(return_value=response)): - result = await client.pre_check() - assert result is None + async def test_nonzero_exit_returns_none_failopen(self, db_with_data): + svc = _build_svc(db_with_data) + body = { + "hook_present": True, + "exit_code": 2, + "stdout": "", + "stderr": "ModuleNotFoundError: foo", + } + with _mock_httpx_post(body): + decision = await svc._run_pre_check("test-agent") + assert decision is None @pytest.mark.asyncio - async def test_returns_none_on_5xx(self): - client = AgentClient("broken-agent") - response = MagicMock() - response.status_code = 500 - with patch.object(client, "_request", AsyncMock(return_value=response)): - result = await client.pre_check() - assert result is None + async def test_empty_stdout_skips(self, db_with_data): + svc = _build_svc(db_with_data) + body = {"hook_present": True, "exit_code": 0, "stdout": " \n", "stderr": ""} + with _mock_httpx_post(body): + decision = await svc._run_pre_check("test-agent") + assert decision == {"fire": False, "reason": "pre-check returned empty stdout"} @pytest.mark.asyncio - async def test_returns_none_on_unreachable(self): - client = AgentClient("unreachable-agent") - with patch.object( - client, - "_request", - AsyncMock(side_effect=AgentNotReachableError("timeout")), - ): - result = await client.pre_check() - assert result is None + async def test_nonempty_stdout_fires_with_override(self, db_with_data): + svc = _build_svc(db_with_data) + body = { + "hook_present": True, + "exit_code": 0, + "stdout": "Review PR #1\n", + "stderr": "", + } + with _mock_httpx_post(body): + decision = await svc._run_pre_check("test-agent") + assert decision == {"fire": True, "message": "Review PR #1"} @pytest.mark.asyncio - async def test_returns_none_on_malformed_json(self): - client = AgentClient("sketchy-agent") - response = MagicMock() - response.status_code = 200 - response.json.side_effect = ValueError("not json") - with patch.object(client, "_request", AsyncMock(return_value=response)): - result = await client.pre_check() - assert result is None + async def test_backend_404_returns_none(self, db_with_data): + svc = _build_svc(db_with_data) + with _mock_httpx_post(None, status_code=404): + decision = await svc._run_pre_check("missing-agent") + assert decision is None @pytest.mark.asyncio - async def test_returns_none_on_missing_fire_field(self): - client = AgentClient("wrong-shape-agent") - response = MagicMock() - response.status_code = 200 - response.json.return_value = {"message": "hi"} # no "fire" - with patch.object(client, "_request", AsyncMock(return_value=response)): - result = await client.pre_check() - assert result is None + async def test_backend_5xx_returns_none(self, db_with_data): + svc = _build_svc(db_with_data) + with _mock_httpx_post({}, status_code=502): + decision = await svc._run_pre_check("test-agent") + assert decision is None @pytest.mark.asyncio - async def test_returns_skip_decision(self): - client = AgentClient("pr-reviewer") + async def test_connection_error_returns_none(self, db_with_data): + svc = _build_svc(db_with_data) + with _mock_httpx_post(raise_exc=Exception("connection refused")): + decision = await svc._run_pre_check("test-agent") + assert decision is None + + @pytest.mark.asyncio + async def test_malformed_json_returns_none(self, db_with_data): + """Backend returns 200 but body isn't valid JSON → fail-open.""" + svc = _build_svc(db_with_data) response = MagicMock() response.status_code = 200 - response.json.return_value = {"fire": False, "reason": "no new PRs"} - with patch.object(client, "_request", AsyncMock(return_value=response)): - result = await client.pre_check() - assert result == {"fire": False, "reason": "no new PRs"} + response.json = MagicMock(side_effect=ValueError("not json")) + client_ctx = AsyncMock() + client_ctx.__aenter__.return_value.post = AsyncMock(return_value=response) + with patch( + "scheduler.service.httpx.AsyncClient", return_value=client_ctx + ): + decision = await svc._run_pre_check("test-agent") + assert decision is None # --------------------------------------------------------------------------- @@ -109,7 +141,7 @@ async def test_returns_skip_decision(self): @pytest.fixture def mock_scheduler(db_with_data): - """Build a SchedulerService with mocked dependencies so we can drive the + """Build a SchedulerService with mocked I/O deps so we can drive the pre-check branch of `_execute_schedule_with_lock` without real networking.""" from scheduler.service import SchedulerService @@ -163,7 +195,7 @@ async def test_fire_with_override_message(self, mock_scheduler, db_with_data): async def test_fail_open_when_pre_check_returns_none( self, mock_scheduler, db_with_data ): - """pre-check returns None (e.g. 404) → fire as usual with schedule.message.""" + """pre-check returns None (e.g. no hook, exec error) → fire with schedule.message.""" mock_scheduler._run_pre_check = AsyncMock(return_value=None) await mock_scheduler._execute_schedule_with_lock("schedule-1") diff --git a/tests/unit/test_pre_check_router.py b/tests/unit/test_pre_check_router.py deleted file mode 100644 index ff64be073..000000000 --- a/tests/unit/test_pre_check_router.py +++ /dev/null @@ -1,208 +0,0 @@ -""" -Unit tests for the agent-server pre-check router (#454, follow-up to review feedback). - -Covers response normalisation + router behavior without running a real -agent container. Pairs with ``tests/scheduler_tests/test_pre_check.py`` -which covers the scheduler-side integration. -""" -from __future__ import annotations - -import sys -from pathlib import Path -from unittest.mock import patch - -import pytest - -_BASE_IMAGE = ( - Path(__file__).resolve().parent.parent.parent / "docker" / "base-image" -) -_BASE_IMAGE_STR = str(_BASE_IMAGE) -if _BASE_IMAGE_STR not in sys.path: - sys.path.insert(0, _BASE_IMAGE_STR) - -from fastapi import FastAPI # noqa: E402 -from fastapi.testclient import TestClient # noqa: E402 - -# Import the module directly rather than via the ``routers`` package so we -# don't pull in snapshot.py / git.py, which require python-multipart at -# import time. -import importlib.util # noqa: E402 - -_pre_check_path = _BASE_IMAGE / "agent_server" / "routers" / "pre_check.py" -_spec = importlib.util.spec_from_file_location( - "agent_server_pre_check_router", _pre_check_path -) -_pre_check_mod = importlib.util.module_from_spec(_spec) -_spec.loader.exec_module(_pre_check_mod) - -MAX_MESSAGE_BYTES = _pre_check_mod.MAX_MESSAGE_BYTES -_normalise_result = _pre_check_mod._normalise_result -router = _pre_check_mod.router - - -# --------------------------------------------------------------------------- -# Response normalisation -# --------------------------------------------------------------------------- - - -class TestNormaliseResult: - def test_skip_decision_with_reason(self): - out = _normalise_result({"fire": False, "reason": "nothing to do"}) - assert out == {"fire": False, "reason": "nothing to do"} - - def test_fire_decision_with_message(self): - out = _normalise_result({"fire": True, "message": "Run review"}) - assert out == {"fire": True, "message": "Run review"} - - def test_fire_decision_without_message(self): - assert _normalise_result({"fire": True}) == {"fire": True} - - def test_oversized_message_dropped_with_truncation_marker(self): - """Review feedback: silent drop hides template bugs. Expose the drop.""" - oversized = "x" * (MAX_MESSAGE_BYTES + 100) - out = _normalise_result({"fire": True, "message": oversized}) - # Message must not pass through - assert "message" not in out - # Caller gets explicit signal that override was dropped - assert "message_truncated" in out - assert "exceeds" in out["message_truncated"] - assert str(MAX_MESSAGE_BYTES) in out["message_truncated"] - assert out["fire"] is True - - def test_message_exactly_at_cap_is_accepted(self): - msg = "x" * MAX_MESSAGE_BYTES - out = _normalise_result({"fire": True, "message": msg}) - assert out.get("message") == msg - - def test_reason_clamped_to_2000_chars(self): - out = _normalise_result({"fire": False, "reason": "z" * 5000}) - assert len(out["reason"]) == 2000 - - def test_non_dict_return_raises_value_error(self): - """check() returning None/True/etc. must be surfaced as a server - error, not silently normalised away.""" - for bad in (None, True, 42, "hello", [1, 2, 3]): - with pytest.raises(ValueError, match="must return a dict"): - _normalise_result(bad) - - def test_dict_missing_fire_key_raises_value_error(self): - with pytest.raises(ValueError, match="must return a dict"): - _normalise_result({"message": "no fire key"}) - - -# --------------------------------------------------------------------------- -# HTTP router behavior -# --------------------------------------------------------------------------- - - -@pytest.fixture -def app(): - app = FastAPI() - app.include_router(router) - return app - - -class TestRouter: - def test_returns_404_when_check_absent(self, app): - with patch.object( - _pre_check_mod, - "_load_check_callable", - return_value=None, - ): - with TestClient(app) as c: - r = c.post("/api/pre-check") - assert r.status_code == 404 - - def test_returns_200_on_fire_true_with_message(self, app): - def check(): - return {"fire": True, "message": "Review PR #1"} - - with patch.object( - _pre_check_mod, - "_load_check_callable", - return_value=check, - ): - with TestClient(app) as c: - r = c.post("/api/pre-check") - assert r.status_code == 200 - assert r.json() == {"fire": True, "message": "Review PR #1"} - - def test_returns_200_on_fire_false(self, app): - def check(): - return {"fire": False, "reason": "no new PRs"} - - with patch.object( - _pre_check_mod, - "_load_check_callable", - return_value=check, - ): - with TestClient(app) as c: - r = c.post("/api/pre-check") - assert r.status_code == 200 - assert r.json() == {"fire": False, "reason": "no new PRs"} - - def test_returns_500_when_check_raises(self, app): - def check(): - raise RuntimeError("template bug") - - with patch.object( - _pre_check_mod, - "_load_check_callable", - return_value=check, - ): - with TestClient(app) as c: - r = c.post("/api/pre-check") - assert r.status_code == 500 - assert "template bug" in r.json()["detail"] - - def test_returns_500_when_check_returns_non_dict(self, app): - """Review feedback: the ValueError path of _normalise_result was - previously untested end-to-end.""" - - def check(): - return None - - with patch.object( - _pre_check_mod, - "_load_check_callable", - return_value=check, - ): - with TestClient(app) as c: - r = c.post("/api/pre-check") - assert r.status_code == 500 - assert "must return a dict" in r.json()["detail"] - - def test_oversized_message_falls_back_without_override(self, app): - """End-to-end path for the truncation marker — scheduler sees - message_truncated but no `message` key, so falls back to - schedule.message.""" - - def check(): - return {"fire": True, "message": "x" * (MAX_MESSAGE_BYTES + 50)} - - with patch.object( - _pre_check_mod, - "_load_check_callable", - return_value=check, - ): - with TestClient(app) as c: - r = c.post("/api/pre-check") - assert r.status_code == 200 - body = r.json() - assert body["fire"] is True - assert "message" not in body - assert "message_truncated" in body - - def test_async_check_is_awaited(self, app): - async def check(): - return {"fire": True} - - with patch.object( - _pre_check_mod, - "_load_check_callable", - return_value=check, - ): - with TestClient(app) as c: - r = c.post("/api/pre-check") - assert r.status_code == 200 - assert r.json() == {"fire": True} From 6afd0ee0ec46603e97cb2fcf3528567a5cf5b768 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Thu, 23 Apr 2026 15:17:00 +0300 Subject: [PATCH 5/6] refactor(#454): make pre-check hook language-agnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the `python3` invocation from the backend's pre-check exec and rename the convention path from `~/.trinity/pre-check.py` to `~/.trinity/pre-check`. Trinity now execs the file directly — the interpreter is selected by the file's shebang line. Templates can ship Python, bash, node, or a compiled binary; Trinity stops caring about language. `test -f` (not `-x`) is kept for the existence check so a present-but-non-executable file surfaces as an exec failure (exit 126) in the operator log instead of silently falling through to the backward-compat "no hook" path. Docs (architecture / feature-flow / requirements / index) updated to reflect the new contract. Scheduler-side translation tests are unaffected — they assert the JSON contract, not the exec command string. 13/13 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/memory/architecture.md | 4 +-- docs/memory/feature-flows.md | 2 +- .../feature-flows/scheduler-pre-check.md | 20 +++++++++------ docs/memory/requirements.md | 4 +-- src/backend/routers/internal.py | 25 ++++++++++++------- src/scheduler/service.py | 12 +++++---- 6 files changed, 40 insertions(+), 27 deletions(-) diff --git a/docs/memory/architecture.md b/docs/memory/architecture.md index 72ac5f4f5..2c042be49 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -363,7 +363,7 @@ docker exec trinity-vector sh -c "tail -50 /data/logs/agents.json" | jq . - `/api/files` - List workspace files (recursive tree structure) - `/api/files/download` - Download file content (100MB limit) -**Template-supplied pre-check** (optional, SCHED-COND-001): if the template ships `~/.trinity/pre-check.py`, the backend's internal endpoint `POST /api/internal/agents/{name}/pre-check` executes it via `docker exec` before the scheduler fires a cron-triggered chat. The script's stdout becomes the chat message; empty stdout + exit 0 records a skipped execution. No HTTP endpoint is exposed on the agent-server for this — the primitive is the same `execute_command_in_container` already used by `services/git_service.py` (persistent-state allowlist), `ssh_service.py`, and the agent terminal. +**Template-supplied pre-check** (optional, SCHED-COND-001): if the template ships an executable `~/.trinity/pre-check` file, the backend's internal endpoint `POST /api/internal/agents/{name}/pre-check` runs it via `docker exec` before the scheduler fires a cron-triggered chat. The hook is **language-agnostic** — interpreter is selected by the file's shebang line (Python, bash, node, compiled binary, …); Trinity does not invoke `python3` for it. The hook's stdout becomes the chat message; empty stdout + exit 0 records a skipped execution. No HTTP endpoint is exposed on the agent-server for this — the primitive is the same `execute_command_in_container` already used by `services/git_service.py` (persistent-state allowlist), `ssh_service.py`, and the agent terminal. **Persistent Chat:** - All chat messages automatically saved to SQLite (`chat_sessions`, `chat_messages`) @@ -397,7 +397,7 @@ Services that run continuously in the backend process: | **Operator Queue Sync** | `operator_queue_service.py` | Polls running agents every 5s, reads `~/.trinity/operator-queue.json`, syncs to DB, writes responses back. (OPS-001) | | **Sync Health Service** | `sync_health_service.py` | Polls git-enabled agents every 60s, upserts `agent_sync_state`, emits `sync_failing` operator-queue entries when consecutive_failures ≥ 3. (#389 S1) | | **Monitoring Service** | `monitoring_service.py` | Fleet-wide health checks on configurable interval. (MON-001) | -| **Scheduler Service** | `scheduler_service.py` | APScheduler-based cron job execution. Async fire-and-forget with DB polling for status. On each cron-triggered fire, optionally invokes the agent's `~/.trinity/pre-check.py` via the backend's `POST /api/internal/agents/{name}/pre-check` (which `docker exec`s into the agent container). Empty stdout + exit 0 records a skipped execution and does not invoke Claude (SCHED-COND-001, #454). | +| **Scheduler Service** | `scheduler_service.py` | APScheduler-based cron job execution. Async fire-and-forget with DB polling for status. On each cron-triggered fire, optionally invokes the agent's executable `~/.trinity/pre-check` (interpreter chosen by shebang) via the backend's `POST /api/internal/agents/{name}/pre-check` (which `docker exec`s into the agent container). Empty stdout + exit 0 records a skipped execution and does not invoke Claude (SCHED-COND-001, #454). | | **Capacity Maintenance** | `capacity_manager.py` | Calls `CapacityManager.run_maintenance()` every 60s — expires stale queued tasks (>24h) and drains orphans after restart. (BACKLOG-001 / CAPACITY-CONSOLIDATE #428) | The **agent server** also runs a 15-min `auto_sync` heartbeat loop (gated diff --git a/docs/memory/feature-flows.md b/docs/memory/feature-flows.md index 26144d201..ade2815d3 100644 --- a/docs/memory/feature-flows.md +++ b/docs/memory/feature-flows.md @@ -19,7 +19,7 @@ | 2026-04-25 | #487 | Telegram file upload Phase 2 — workspace delivery hardening: NFKC sanitizer with collision dedup, spec injection format `[File uploaded by {uploader}]: {name} ({size}) saved to {path}`, all-writes-failed channel error + abort. Same code path benefits Slack inbound. | [telegram-integration.md](feature-flows/telegram-integration.md), [slack-file-sharing.md](feature-flows/slack-file-sharing.md) | | 2026-04-23 | #476 | SQLite lexicographic cutoff bug fix — new `iso_cutoff(hours)` helper replaces `datetime('now', ...)` in 15 sites across rate-limit / dashboard / schedules; `max_retries` default flipped `1 → 0`; `cleanup_old_rate_limit_events` wired into `CleanupService` (phase 6, hourly) | [subscription-auto-switch.md](feature-flows/subscription-auto-switch.md), [cleanup-service.md](feature-flows/cleanup-service.md), [scheduler-service.md](feature-flows/scheduler-service.md) | | 2026-04-22 | #458 | `.gitignore` init fix — `initialize_git_in_container` now appends missing patterns instead of truncate-and-write; adds `.env`, `.env.*`, `.mcp.json` to the default list and runs for both `/home/developer` and legacy `/home/developer/workspace` (stops credential leak on first GitHub sync) | [github-repo-initialization.md](feature-flows/github-repo-initialization.md) | -| 2026-04-22 | SCHED-COND-001 (#454) | Conditional schedule pre-check — backend `docker exec`s template-supplied `~/.trinity/pre-check.py` before scheduler fires a cron chat; empty stdout + exit 0 records a skipped execution; fail-open; reuses `ExecutionStatus.SKIPPED` (no schema change, no HTTP edge from scheduler to agent) | [scheduler-pre-check.md](feature-flows/scheduler-pre-check.md) | +| 2026-04-22 | SCHED-COND-001 (#454) | Conditional schedule pre-check — backend `docker exec`s the template's executable `~/.trinity/pre-check` (language-agnostic, interpreter from shebang) before scheduler fires a cron chat; empty stdout + exit 0 records a skipped execution; fail-open; reuses `ExecutionStatus.SKIPPED` (no schema change, no HTTP edge from scheduler to agent) | [scheduler-pre-check.md](feature-flows/scheduler-pre-check.md) | | 2026-04-21 | RELIABILITY-003 (#306) | WebSocket event bus on Redis Streams — replaces in-process broadcast with XADD/XREAD, adds reconnect replay via `?last-event-id=`, 3-failure eviction, MAXLEN trim (tunable) | [websocket-event-bus.md](feature-flows/websocket-event-bus.md) | | 2026-04-20 | #420 | Scheduler sync loop fix — `update_schedule_run_times` no longer bumps `updated_at`, stopping the self-triggering re-register of every schedule per tick | [scheduler-service.md](feature-flows/scheduler-service.md) | | 2026-04-20 | #418 | Inter-agent timeout honors per-agent `execution_timeout_seconds` — removed 600s hardcoded defaults in MCP `chat_with_agent`/`fan_out` tools and fan-out service; HTTP client ceiling bumped to platform max (7200s) | [fan-out.md](feature-flows/fan-out.md), [mcp-orchestration.md](feature-flows/mcp-orchestration.md), [parallel-headless-execution.md](feature-flows/parallel-headless-execution.md) | diff --git a/docs/memory/feature-flows/scheduler-pre-check.md b/docs/memory/feature-flows/scheduler-pre-check.md index 9cd1cbb78..f941e20e2 100644 --- a/docs/memory/feature-flows/scheduler-pre-check.md +++ b/docs/memory/feature-flows/scheduler-pre-check.md @@ -1,13 +1,15 @@ # Feature: Conditional Schedule Pre-Check (SCHED-COND-001 / Issue #454) ## Overview -Optional template-supplied hook that lets a scheduled cron tick be skipped deterministically without waking Claude. Before firing a cron-triggered chat, the scheduler calls a **backend** internal endpoint, which `docker exec`s `~/.trinity/pre-check.py` inside the target agent container. Non-empty stdout becomes the chat prompt; empty stdout + exit 0 records a skipped execution. Eliminates per-tick token burn on poll-driven agents (PR reviewers, inbox monitors, alert routers). +Optional template-supplied hook that lets a scheduled cron tick be skipped deterministically without waking Claude. Before firing a cron-triggered chat, the scheduler calls a **backend** internal endpoint, which `docker exec`s the executable `~/.trinity/pre-check` file inside the target agent container. Non-empty stdout becomes the chat prompt; empty stdout + exit 0 records a skipped execution. Eliminates per-tick token burn on poll-driven agents (PR reviewers, inbox monitors, alert routers). + +The hook is **language-agnostic** — Trinity execs the file directly, so the interpreter is chosen by the file's shebang line. Templates ship Python, bash, node, Go binaries — Trinity does not care. ## User Story As the author of a poll-driven agent template, I want a cheap deterministic gate to run before Claude wakes so empty polls (scan→no work) don't burn tokens. As a Trinity operator, I don't want to configure the gate per schedule — the agent template owns it, I just schedule the cadence. ## Entry Points -- **Template contract**: ship `~/.trinity/pre-check.py` as a standalone executable script. Prints chat prompt to stdout when work is found; exits 0 with empty stdout to skip; exits non-zero on error (fail-open). +- **Template contract**: ship `~/.trinity/pre-check` as a `+x` executable file with a shebang (`#!/usr/bin/env python3`, `#!/bin/bash`, etc.). Prints chat prompt to stdout when work is found; exits 0 with empty stdout to skip; exits non-zero on error (fail-open). - **Backend endpoint** (internal, `X-Internal-Secret` auth): `POST /api/internal/agents/{name}/pre-check` — runs the script and returns stdout + exit code. Called only by `trinity-scheduler`. - **No operator-facing API change**: schedule CRUD endpoints and the Schedules UI are unchanged. Operators create normal cron schedules; agents own the gate. @@ -20,8 +22,8 @@ No UI change. Skipped executions appear in the existing schedule executions list Uses `services/docker_service.execute_command_in_container` (the same primitive as `services/git_service.py` persistent-state allowlist, `services/ssh_service.py` key provisioning, `services/agent_service/terminal.py`, `routers/system_agent.py`, `adapters/message_router.py` Slack ingest, etc.). Two exec steps: -1. `test -f /home/developer/.trinity/pre-check.py` (5s timeout). If the file doesn't exist, return `{"hook_present": False}` immediately — scheduler treats as "no decision, fire as usual." -2. `python3 /home/developer/.trinity/pre-check.py` (60s timeout). Returns the output (capped at 32 KB) and exit code. +1. `test -f /home/developer/.trinity/pre-check` (5s timeout). If the file doesn't exist, return `{"hook_present": False}` immediately — scheduler treats as "no decision, fire as usual." Note `-f`, not `-x`: a file present but missing the executable bit is treated as "hook present" so the operator gets a 126 in the logs rather than a silent backward-compat fall-through. +2. `/home/developer/.trinity/pre-check` (60s timeout). Trinity execs the path directly — no `python3` prefix. Interpreter resolution is the file's shebang. Returns the output (capped at 32 KB) and exit code. Returns: ```json @@ -89,8 +91,10 @@ New event type: `schedule_execution_skipped`. | Condition | Scheduler behavior | |---|---| | Agent container doesn't exist | Backend returns 404 → fire as usual (schedule likely stale, let execute-task path handle the 404 surfacing) | -| `~/.trinity/pre-check.py` absent | `hook_present: false` → fire as usual (backward compat) | -| `python3 .../pre-check.py` exits non-zero | Fail-open — log stderr, fire with `schedule.message` | +| `~/.trinity/pre-check` absent | `hook_present: false` → fire as usual (backward compat) | +| File present but not `+x` (exit 126) | Fail-open — log "hook for X exited 126", fire with `schedule.message`. Operator's signal to `chmod +x` the hook. | +| Shebang missing or interpreter not found (exit 127) | Fail-open — log shows "command not found"; operator fixes shebang. | +| Hook exits non-zero for any other reason | Fail-open — log stderr, fire with `schedule.message` | | Exec timeout (>60s) | Backend returns non-zero exit → fail-open | | Backend unreachable (connection error) | Fail-open — fire as usual, log warning | | Backend 5xx / malformed JSON | Fail-open | @@ -100,7 +104,7 @@ New event type: `schedule_execution_skipped`. ## Security - Pre-check runs inside the agent's container as `developer`, same sandbox as chat-mode tool calls. No new privilege over what chat-mode tool calls can already do. -- **Template review expectation**: `.trinity/pre-check.py` is executed with full Python interpreter access — it can `import subprocess`, open sockets, read files. This is intentional (operators already trust the template's `CLAUDE.md`, skills, and tool invocations), but `.trinity/pre-check.py` should be reviewed with the same scrutiny as any other executable file the template ships. +- **Template review expectation**: `.trinity/pre-check` is exec'd directly via the kernel — full process privileges of `developer`, in whatever language the shebang names. It can spawn subprocesses, open sockets, read files. This is intentional (operators already trust the template's `CLAUDE.md`, skills, and tool invocations), but `.trinity/pre-check` should be reviewed with the same scrutiny as any other executable the template ships. - Backend endpoint is gated by the existing `X-Internal-Secret` header (C-003). Only `trinity-scheduler` and other internal services can invoke it. - Stdout cap at 32 KB on the backend side — oversized output is truncated, still valid as a chat prompt (or truncated to "looks non-empty" which is fine for the fire-with-override path). - Fail-open policy means a malicious/broken pre-check cannot suppress scheduled invocations (worst case: wastes tokens — today's baseline). @@ -133,4 +137,4 @@ No test file on the agent-server side — there's no agent-server router anymore ## Migration / Rollout - Zero migration required (no schema change). - Existing schedules and agent templates behave identically after deploy (script absent → fall back to today's fire semantics). -- Templates opt in by shipping `.trinity/pre-check.py` (executable, `+x`). No Trinity-side flag. +- Templates opt in by shipping `.trinity/pre-check` (executable, `+x`, with a shebang). No Trinity-side flag. File extension is intentionally absent — the file is whatever the shebang says it is. diff --git a/docs/memory/requirements.md b/docs/memory/requirements.md index 4b040f0bb..8050b1a79 100644 --- a/docs/memory/requirements.md +++ b/docs/memory/requirements.md @@ -377,9 +377,9 @@ Trinity is autonomous agent orchestration and infrastructure — sovereign infra - **Status**: ✅ Implemented (2026-04-22) - **Requirement ID**: SCHED-COND-001 - **GitHub Issue**: #454 -- **Description**: Optional template-supplied hook that lets a scheduled cron tick be skipped deterministically — the scheduler calls a new internal backend endpoint which `docker exec`s `~/.trinity/pre-check.py` inside the target agent container; non-empty stdout becomes the chat prompt, empty stdout + exit 0 records a skipped execution. Eliminates Claude token cost on empty polls for poll-driven agents (PR reviewers, inbox monitors, alert routers, RSS watchers). +- **Description**: Optional template-supplied hook that lets a scheduled cron tick be skipped deterministically — the scheduler calls a new internal backend endpoint which `docker exec`s the executable `~/.trinity/pre-check` file inside the target agent container; non-empty stdout becomes the chat prompt, empty stdout + exit 0 records a skipped execution. The hook is language-agnostic (interpreter chosen by shebang). Eliminates Claude token cost on empty polls for poll-driven agents (PR reviewers, inbox monitors, alert routers, RSS watchers). - **Key Features**: - - Contract: agent templates drop an executable `~/.trinity/pre-check.py` (shebanged standalone script). Stdout is the chat prompt; empty stdout + exit 0 = skip; non-zero exit = fail-open. + - Contract: agent templates drop an executable `~/.trinity/pre-check` file with a shebang (`#!/usr/bin/env python3`, `#!/bin/bash`, …). Trinity execs it directly — no `python3` prefix, no language assumption. Stdout is the chat prompt; empty stdout + exit 0 = skip; non-zero exit = fail-open. - Backend endpoint: `POST /api/internal/agents/{name}/pre-check` (X-Internal-Secret gated) runs the script via `execute_command_in_container` — the same primitive used by `git_service.py` (persistent-state allowlist, #384 S3), `ssh_service.py`, `agent_service/terminal.py`, `adapters/message_router.py`, `routers/system_agent.py`, `routers/voice.py`. - Fail-open: script absent, non-zero exit, timeout, backend 5xx / malformed response → scheduler fires as usual. A broken pre-check never silently suppresses scheduled work. - Message override: non-empty stdout replaces `schedule.message` for that one invocation — lets the agent inject real work items (e.g. the PR list) into the chat prompt. diff --git a/src/backend/routers/internal.py b/src/backend/routers/internal.py index 57dcec1d6..f85babc55 100644 --- a/src/backend/routers/internal.py +++ b/src/backend/routers/internal.py @@ -67,7 +67,10 @@ async def internal_health(): # 32 KB is plenty even for verbose scan output. _PRE_CHECK_STDOUT_CAP = 32_000 _PRE_CHECK_STDERR_CAP = 4_000 -_PRE_CHECK_HOOK_PATH = "/home/developer/.trinity/pre-check.py" +# Convention: language-agnostic executable. Template ships any executable +# (Python, bash, node, compiled binary) — interpreter is determined by the +# shebang line, not by Trinity. File must be marked +x. +_PRE_CHECK_HOOK_PATH = "/home/developer/.trinity/pre-check" _PRE_CHECK_TIMEOUT_SECONDS = 60 @@ -76,16 +79,19 @@ async def internal_agent_pre_check(agent_name: str): """Run the agent's optional pre-check hook (SCHED-COND-001 / #454). Called by the dedicated scheduler before firing a cron-triggered task. - Executes ``~/.trinity/pre-check.py`` inside the agent container via + Executes ``~/.trinity/pre-check`` inside the agent container via ``docker exec`` (the same primitive used by ``services/git_service.py`` for the persistent-state allowlist, by ``ssh_service`` for key - provisioning, etc.). + provisioning, etc.). The hook is a language-agnostic executable; the + interpreter is selected by the file's shebang line. Contract: - ``hook_present == False`` → scheduler treats as no decision and fires as usual (backward compat for templates without a hook). - ``hook_present == True``, ``exit_code != 0`` → fail-open: scheduler - logs the stderr and fires as usual. + logs the stderr and fires as usual. Exit 126 (file not executable) + and 127 (no shebang / interpreter missing) surface here too — the + log is the operator's signal to fix the template. - ``hook_present == True``, ``exit_code == 0``, empty stdout → scheduler records a skipped execution. - ``hook_present == True``, ``exit_code == 0``, non-empty stdout → @@ -107,9 +113,9 @@ async def internal_agent_pre_check(agent_name: str): container_name = f"agent-{agent_name}" - # Step 1: fast existence check. If the template ships no hook, return - # immediately so the scheduler can fire as usual without spinning up - # python3 inside the container. + # Step 1: fast existence check (`-f`, not `-x`). A present-but-non-executable + # file is treated as "hook present" so the operator gets a 126 in the logs + # rather than a silent backward-compat fall-through. exists = await execute_command_in_container( container_name=container_name, command=f"test -f {_PRE_CHECK_HOOK_PATH}", @@ -118,10 +124,11 @@ async def internal_agent_pre_check(agent_name: str): if exists.get("exit_code") != 0: return {"hook_present": False} - # Step 2: run the hook. Stdout becomes the chat prompt (or empty = skip). + # Step 2: run the hook directly — shebang picks interpreter. Stdout + # becomes the chat prompt (or empty = skip). result = await execute_command_in_container( container_name=container_name, - command=f"python3 {_PRE_CHECK_HOOK_PATH}", + command=_PRE_CHECK_HOOK_PATH, timeout=_PRE_CHECK_TIMEOUT_SECONDS, ) output = (result.get("output") or "")[: _PRE_CHECK_STDOUT_CAP + _PRE_CHECK_STDERR_CAP] diff --git a/src/scheduler/service.py b/src/scheduler/service.py index 7165996f3..585e6bb1f 100644 --- a/src/scheduler/service.py +++ b/src/scheduler/service.py @@ -901,11 +901,13 @@ async def _execute_schedule_with_lock(self, schedule_id: str, triggered_by: str async def _run_pre_check(self, agent_name: str) -> Optional[dict]: """Run the agent's optional pre-check hook (#454, SCHED-COND-001). - Calls the backend's internal endpoint, which executes - ``~/.trinity/pre-check.py`` inside the agent container via - ``docker exec`` (same primitive as the persistent-state allowlist - in ``services/git_service.py``). The scheduler never opens its - own HTTP edge to agents — backend remains the orchestrator. + Calls the backend's internal endpoint, which `docker exec`s the + executable ``~/.trinity/pre-check`` file inside the agent + container (same primitive as the persistent-state allowlist in + ``services/git_service.py``). The hook is language-agnostic — + Trinity execs the path directly, so the interpreter is chosen + by the file's shebang. The scheduler never opens its own HTTP + edge to agents — backend remains the orchestrator. Contract translation (backend → caller): - ``hook_present == False`` → return ``None`` (fire as usual) From 5df651ee6198b176a3bf67f12a393bab858acbd7 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Thu, 23 Apr 2026 17:02:09 +0300 Subject: [PATCH 6/6] refactor(#454): extract pre_check_service from internal router MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the pre-check exec logic out of routers/internal.py into a dedicated services/pre_check_service.py. Aligns with Invariant #1 (Router → Service → DB): the router becomes a 5-line passthrough, all business logic (path constant, two exec calls, stdout capping, contract dict) lives in the service. Same shape as services/slot_service.py, services/task_execution_service.py, services/monitoring_service.py — testable in isolation, discoverable via grep, won't trip /validate-architecture later. Behavior unchanged. 13/13 scheduler unit tests still pass; live endpoint hit returns same contract dict. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/backend/routers/internal.py | 76 ++---------------- src/backend/services/pre_check_service.py | 94 +++++++++++++++++++++++ 2 files changed, 101 insertions(+), 69 deletions(-) create mode 100644 src/backend/services/pre_check_service.py diff --git a/src/backend/routers/internal.py b/src/backend/routers/internal.py index f85babc55..076b81ce4 100644 --- a/src/backend/routers/internal.py +++ b/src/backend/routers/internal.py @@ -63,84 +63,22 @@ async def internal_health(): # Scheduler pre-check (#454, SCHED-COND-001) # --------------------------------------------------------------------------- -# Cap stdout/stderr to keep responses bounded. Stdout becomes a chat prompt; -# 32 KB is plenty even for verbose scan output. -_PRE_CHECK_STDOUT_CAP = 32_000 -_PRE_CHECK_STDERR_CAP = 4_000 -# Convention: language-agnostic executable. Template ships any executable -# (Python, bash, node, compiled binary) — interpreter is determined by the -# shebang line, not by Trinity. File must be marked +x. -_PRE_CHECK_HOOK_PATH = "/home/developer/.trinity/pre-check" -_PRE_CHECK_TIMEOUT_SECONDS = 60 - @router.post("/agents/{agent_name}/pre-check") async def internal_agent_pre_check(agent_name: str): """Run the agent's optional pre-check hook (SCHED-COND-001 / #454). - Called by the dedicated scheduler before firing a cron-triggered task. - Executes ``~/.trinity/pre-check`` inside the agent container via - ``docker exec`` (the same primitive used by ``services/git_service.py`` - for the persistent-state allowlist, by ``ssh_service`` for key - provisioning, etc.). The hook is a language-agnostic executable; the - interpreter is selected by the file's shebang line. - - Contract: - - ``hook_present == False`` → scheduler treats as no decision and - fires as usual (backward compat for templates without a hook). - - ``hook_present == True``, ``exit_code != 0`` → fail-open: scheduler - logs the stderr and fires as usual. Exit 126 (file not executable) - and 127 (no shebang / interpreter missing) surface here too — the - log is the operator's signal to fix the template. - - ``hook_present == True``, ``exit_code == 0``, empty stdout → - scheduler records a skipped execution. - - ``hook_present == True``, ``exit_code == 0``, non-empty stdout → - scheduler fires with the stdout used as the chat message - (overrides ``schedule.message``). - - Fail-open is structural: any error here must not silently suppress - scheduled work. Worst case is today's baseline (chat fires, tokens - burn). + Thin passthrough — all logic lives in + ``services/pre_check_service.py`` (Invariant #1: Router → Service + → DB). See that module for the full contract. """ - from services.docker_service import ( - execute_command_in_container, - get_agent_container, - ) + from services.pre_check_service import run_pre_check, AgentNotFound - container = get_agent_container(agent_name) - if not container: + try: + return await run_pre_check(agent_name) + except AgentNotFound: raise HTTPException(status_code=404, detail="Agent not found") - container_name = f"agent-{agent_name}" - - # Step 1: fast existence check (`-f`, not `-x`). A present-but-non-executable - # file is treated as "hook present" so the operator gets a 126 in the logs - # rather than a silent backward-compat fall-through. - exists = await execute_command_in_container( - container_name=container_name, - command=f"test -f {_PRE_CHECK_HOOK_PATH}", - timeout=5, - ) - if exists.get("exit_code") != 0: - return {"hook_present": False} - - # Step 2: run the hook directly — shebang picks interpreter. Stdout - # becomes the chat prompt (or empty = skip). - result = await execute_command_in_container( - container_name=container_name, - command=_PRE_CHECK_HOOK_PATH, - timeout=_PRE_CHECK_TIMEOUT_SECONDS, - ) - output = (result.get("output") or "")[: _PRE_CHECK_STDOUT_CAP + _PRE_CHECK_STDERR_CAP] - return { - "hook_present": True, - "exit_code": int(result.get("exit_code", 1)), - # `output` from container_exec_run is the combined stream — keep both - # fields for forward-compat in case we split them later. - "stdout": output[:_PRE_CHECK_STDOUT_CAP], - "stderr": "", - } - @router.get("/agents/{agent_name}/sync-health-status") async def internal_agent_sync_health(agent_name: str): diff --git a/src/backend/services/pre_check_service.py b/src/backend/services/pre_check_service.py new file mode 100644 index 000000000..d18738894 --- /dev/null +++ b/src/backend/services/pre_check_service.py @@ -0,0 +1,94 @@ +""" +Pre-Check Service for Trinity platform (SCHED-COND-001 / #454). + +Runs the agent's optional template-supplied pre-check hook +``~/.trinity/pre-check`` via ``docker exec`` and returns a normalized +contract dict. The hook is language-agnostic — interpreter is selected +by the file's shebang, not by Trinity. + +Called by ``routers/internal.py`` on behalf of the dedicated scheduler +before each cron-triggered fire. Reuses ``execute_command_in_container`` +(the same primitive as ``git_service``, ``ssh_service``, +``agent_service/terminal``, Slack ingest, etc.) — no new HTTP edge from +backend to agent-server, no new long-lived process. +""" +from __future__ import annotations + +import logging +from typing import Dict + +from services.docker_service import ( + execute_command_in_container, + get_agent_container, +) + +logger = logging.getLogger(__name__) + + +# Convention: language-agnostic executable shipped by the template. +# Interpreter is selected by the shebang line; the file must be marked +x. +HOOK_PATH = "/home/developer/.trinity/pre-check" + +# Stdout becomes the chat prompt — 32 KB is plenty even for verbose scan output. +STDOUT_CAP = 32_000 +STDERR_CAP = 4_000 + +EXISTENCE_TIMEOUT_S = 5 +EXEC_TIMEOUT_S = 60 + + +class AgentNotFound(Exception): + """Raised when the target agent has no running container.""" + + +async def run_pre_check(agent_name: str) -> Dict: + """Run the agent's optional pre-check hook. + + Two-step exec: + 1. ``test -f`` (5s) — file presence check. Note ``-f``, not ``-x``: + a present-but-non-executable file surfaces as exec failure + (exit 126) so the operator gets a signal, instead of silently + falling through to the backward-compat "no hook" path. + 2. Run the hook directly (60s). Trinity does not prefix ``python3`` + — the shebang determines interpreter. + + Returns one of: + ``{"hook_present": False}`` + Template ships no hook. Caller should fire as usual. + + ``{"hook_present": True, "exit_code": int, "stdout": str, "stderr": str}`` + Hook ran. Caller translates per the SCHED-COND-001 contract: + - exit != 0 → fail-open + log (broken hook must not suppress work) + - exit 0, empty stdout → record skip + - exit 0, non-empty stdout → fire with stdout as override message + + Raises: + AgentNotFound: if no running container for ``agent_name``. + """ + if not get_agent_container(agent_name): + raise AgentNotFound(agent_name) + + container_name = f"agent-{agent_name}" + + exists = await execute_command_in_container( + container_name=container_name, + command=f"test -f {HOOK_PATH}", + timeout=EXISTENCE_TIMEOUT_S, + ) + if exists.get("exit_code") != 0: + return {"hook_present": False} + + result = await execute_command_in_container( + container_name=container_name, + command=HOOK_PATH, + timeout=EXEC_TIMEOUT_S, + ) + # `output` from container_exec_run is the combined stream — keep + # `stdout`/`stderr` as separate fields for forward-compat. + output = (result.get("output") or "")[: STDOUT_CAP + STDERR_CAP] + return { + "hook_present": True, + "exit_code": int(result.get("exit_code", 1)), + "stdout": output[:STDOUT_CAP], + "stderr": "", + }