diff --git a/docs/memory/architecture.md b/docs/memory/architecture.md index 3d8c46847..2c042be49 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -363,6 +363,8 @@ 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 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`) - Sessions survive container restarts/deletions @@ -395,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. | +| **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 8332e90bd..ade2815d3 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 — 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 new file mode 100644 index 000000000..f941e20e2 --- /dev/null +++ b/docs/memory/feature-flows/scheduler-pre-check.md @@ -0,0 +1,140 @@ +# 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 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` 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. + +## Frontend Layer +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. + +## Backend Layer +**Router**: `src/backend/routers/internal.py` — `POST /api/internal/agents/{name}/pre-check` + +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` (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 +{"hook_present": true, "exit_code": 0, "stdout": "Review PR #1\n", "stderr": ""} +``` + +## Scheduler Layer +**Service**: `src/scheduler/service.py` — `_run_pre_check(agent_name)` + +Calls the backend's internal endpoint (not the agent directly — topology stays "scheduler → backend → agent"). Translates the backend response into a scheduler decision: + +| 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 in `_execute_schedule_with_lock`: called 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): + self.db.create_skipped_execution(...) + self._publish_event({"type": "schedule_execution_skipped", ...}) + return + override = decision.get("message") + if override: + effective_message = override +# ... fires with effective_message +``` + +## Data Layer +**Zero schema change.** Reuses existing: +- `ExecutionStatus.SKIPPED` (already defined for Issue #46 — APScheduler max-instances drops). +- `SchedulerDatabase.create_skipped_execution(...)`. + +Skip rows carry `status='skipped'`, `error="pre-check: "`, `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": "pre-check: pre-check returned empty stdout" +} +``` + +## Side Effects +- 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 +- No `/api/internal/execute-task` call, no backend task creation, no Claude invocation, no backlog slot acquisition + +## Error Handling +| 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` 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 | +| 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` 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). + +## Testing +**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. + +Full scheduler suite: 162/162 passing (was 161 before; +1 for the new `malformed JSON` case). + +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 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 (script absent → fall back to today's fire semantics). +- 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 a86ee380d..8050b1a79 100644 --- a/docs/memory/requirements.md +++ b/docs/memory/requirements.md @@ -373,6 +373,23 @@ 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 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` 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. + - 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`). + - **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) - **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 diff --git a/src/backend/routers/internal.py b/src/backend/routers/internal.py index 5a2a0f37f..076b81ce4 100644 --- a/src/backend/routers/internal.py +++ b/src/backend/routers/internal.py @@ -59,6 +59,27 @@ async def internal_health(): return {"status": "ok"} +# --------------------------------------------------------------------------- +# Scheduler pre-check (#454, SCHED-COND-001) +# --------------------------------------------------------------------------- + + +@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). + + 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.pre_check_service import run_pre_check, AgentNotFound + + try: + return await run_pre_check(agent_name) + except AgentNotFound: + raise HTTPException(status_code=404, detail="Agent not found") + + @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/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": "", + } diff --git a/src/scheduler/service.py b/src/scheduler/service.py index 546d3d0aa..585e6bb1f 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,80 @@ 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]: + """Run the agent's optional pre-check hook (#454, SCHED-COND-001). + + 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) + - ``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. + """ + 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: + data = response.json() + except Exception as e: + logger.warning( + 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 new file mode 100644 index 000000000..83e8ab2b6 --- /dev/null +++ b/tests/scheduler_tests/test_pre_check.py @@ -0,0 +1,231 @@ +""" +Tests for the conditional schedule pre-check hook (#454, SCHED-COND-001). + +Covers: +- `_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 +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.models import ExecutionStatus + + +# --------------------------------------------------------------------------- +# _run_pre_check translation layer (backend JSON → scheduler decision) +# --------------------------------------------------------------------------- + + +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_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_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_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_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_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_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_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 = 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 + + +# --------------------------------------------------------------------------- +# SchedulerService._execute_schedule_with_lock pre-check branch +# --------------------------------------------------------------------------- + + +@pytest.fixture +def mock_scheduler(db_with_data): + """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 + + 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. 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") + + 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()