feat(scheduler): agent-owned pre-check hook (#454)

[dolho]

Closes #454.

Summary

Adds an optional agent-owned hook (POST /api/pre-check on the agent container) that lets a scheduled cron tick be deterministically skipped without waking Claude. Eliminates per-tick token cost on poll-driven agents (PR reviewers, inbox monitors, alert routers, RSS watchers).

Fail-open by design: endpoint absent, timeout, 5xx, or malformed response all fall through to today's fire-as-usual semantics. Backward compatible — no migration, no schema change.

Why

Trinity's scheduler always fires a chat on every cron tick (scheduler_service._execute_schedule_with_lock → /api/internal/execute-task → Claude). For poll-driven agents this burns tokens on every empty poll; at 15-min cadence that's ~96 fires/day × ~$0.02–0.05 = ~$2–5/day per agent doing nothing.

Current workaround required each template to background a long-running daemon inside its container — invisible to Trinity UI, reimplemented per template, no skip metrics. Discovered while building dolho/pr-reviewer-agent; see docs/planning/PR_REVIEWER_AGENT.md for the full motivation.

Contract

Agent templates drop ~/.trinity/pre-check.py with a top-level check() function:

def check() -> dict:
    # Deterministic logic. No LLM calls.
    if not work_to_do():
        return {"fire": False, "reason": "no new items"}
    return {"fire": True, "message": "Handle these items: …"}

Base-image router at POST /api/pre-check loads and invokes it. Scheduler calls the endpoint before firing cron-triggered chats (manual triggers bypass — explicit operator intent always fires).

Response	Scheduler action
200 {fire: true, …}	Fire chat (use message override if provided)
200 {fire: false, reason}	Record skipped execution, no chat
404 Not Found	Fire as today (template has no hook — backward compat)
Connection error / timeout / 5xx / malformed	Fire as today (fail-open)

Skip rows go into schedule_executions with status='skipped' — visible in the Trinity UI alongside success/failure rows, zero cost, reason preserved.

Changes

Base image (docker/base-image/agent_server/):

• routers/pre_check.py — new router: dynamic-load ~/.trinity/pre-check.py, normalise response, 32 KB message cap, sync+async check() support, 500 on exceptions
• routers/__init__.py + main.py — mount the new router

Scheduler (src/scheduler/):

• agent_client.py — AgentClient.pre_check() method with fail-open semantics (404/5xx/timeout/malformed all return None)
• service.py — _run_pre_check() helper + intercept branch in _execute_schedule_with_lock before create_execution. Threads effective_message override through to _call_backend_execute_task. Publishes schedule_execution_skipped WebSocket event on skip.

Data: zero schema change. Reuses existing ExecutionStatus.SKIPPED and SchedulerDatabase.create_skipped_execution (both defined for Issue #46).

Docs:

• docs/memory/feature-flows/scheduler-pre-check.md — new feature flow
• docs/memory/architecture.md §Agent Containers + §Background Services
• docs/memory/requirements.md §10.6.1 SCHED-COND-001
• docs/memory/feature-flows.md index row
• docs/planning/PR_REVIEWER_AGENT.md — design doc (the motivating use case)

Test plan

• 12 new unit tests in tests/scheduler_tests/test_pre_check.py — client response shapes (200 / 404 / 5xx / unreachable / malformed JSON / missing fire field / skip / override); scheduler behaviour (skip records execution, override passes through, fail-open, manual-trigger bypass)
• Full scheduler suite: 161/161 passing
• Live end-to-end in local Trinity with dolho/pr-reviewer-agent:
  • Empty scan → fire:false → skip row in DB, $0, zero backend activity (confirmed via Trinity Executions tab + scheduler logs)
  • Open PR on tracked repo → next tick fire:true, override message with PR list (146 chars) → Claude runs /review, posts real comment to GitHub (~$0.12)
  • Subsequent tick with existing bot comment → fire:false again (stateless dedup verified)
  • Manual trigger bypass verified via unit test
• Base image + scheduler container rebuilt and deployed locally

Notes

• No frontend change required — skipped executions render using the existing status badge.
• Fail-open is deliberate: a broken pre-check must never silently suppress real work. Worst case is today's baseline (tokens burned on empty polls).
• Architectural invariants preserved: Fix: Add missing Docker labels to system agent container #1 (router → service → db), Setup improvements #5 (Agent Server Mirrors Backend — agent exposes HTTP, scheduler proxies).

🤖 Generated with Claude Code

[dolho]

🤖 PR Reviewer (Trinity)

PR Review — feat(scheduler): agent-owned pre-check hook (#454)

Overall: Clean, well-scoped feature. The fail-open semantics are correct and the test coverage is solid. A few items worth flagging.

Potential bugs

Module re-loading on every request (pre_check.py:89-103): _load_check_callable() calls importlib.util.spec_from_file_location + exec_module on every POST /api/pre-check invocation with no caching. If the scheduler polls at 15-min intervals this is negligible, but if polling tightens or multiple schedules share the agent, repeated module exec could produce subtle side effects (module-level globals reset each call). Consider caching the loaded function until the file's mtime changes, or at least documenting the "no cache" intent.

asyncio.get_event_loop() deprecation (pre_check.py:141): asyncio.get_event_loop().run_in_executor(None, fn) is deprecated in Python 3.10+; use asyncio.get_running_loop().run_in_executor(None, fn) to avoid the DeprecationWarning and future breakage.

Race in _run_pre_check / create_skipped_execution (service.py): The skip branch updates schedule.last_run_at / next_run_at and publishes the WebSocket event but does not release any lock before returning. This is fine if _execute_schedule_with_lock holds the lock for the full duration — just worth confirming the lock scope covers the skip path (it appears to, but the diff doesn't show the lock acquisition).

Security

check() has unrestricted execution scope: The pre-check file is loaded from ~/.trinity/pre-check.py and executed with full Python interpreter access — it can import subprocess, open network sockets, etc. This is intentional per the design (same sandbox as chat-mode tool calls), but the doc comment at pre_check.py:71 states "No new privilege" without acknowledging that a malicious/compromised template could exfiltrate data via the pre-check endpoint. Worth a brief note for operators reviewing templates.

message override length check drops silently (pre_check.py:122-127): When message exceeds 32 KB the code logs a warning and falls through to schedule.message — the caller never knows the override was dropped. A template author relying on the override could be silently suppressed. Consider including the drop reason in the skip/fire response or documenting this truncation behavior clearly in the contract.

Missing test coverage

• No test for fire=True with a message that exceeds MAX_MESSAGE_BYTES — the drop-and-fall-back path in _normalise_result is exercised only by inspection.
• No test for check() returning a non-dict (e.g. None, True) — _normalise_result raises ValueError in that case, which becomes a 500; the unit test suite only covers the missing fire field case.

Scope check

Diff is tightly scoped to the pre-check feature. The large planning doc (docs/planning/PR_REVIEWER_AGENT.md) is additive documentation, not functional code — fine to include. No unrelated changes spotted.

[dolho]

Addressed in 03be56c. Point-by-point:

Bugs fixed

• asyncio.get_event_loop() → asyncio.get_running_loop() on pre_check.py:97.
• Oversized message override no longer drops silently. _normalise_result now sets a sibling message_truncated: "override dropped: N bytes exceeds 32000 cap" field when the 32 KB cap trips, and the log is escalated to ERROR with size + limit. The scheduler still falls back to schedule.message (fail-safe behavior), but the drop is now visible in both the response body and agent-server logs.

Security doc

• Module-level docstring in pre_check.py now states explicitly that check() runs with full Python interpreter access (subprocess, sockets, filesystem) and calls out template-review expectations. The feature-flow doc's §Security section mirrors this.

Test coverage added

• tests/unit/test_pre_check_router.py — 15 new tests covering:
  • _normalise_result oversized-message path → asserts message_truncated set and message key absent
  • _normalise_result non-dict return (None, True, 42, "str", list) → raises ValueError
  • _normalise_result dict missing fire key → raises ValueError
  • Router HTTP round-trip for the above (500 surfaces correctly)
  • Plus fire/skip/404/async-check happy paths for completeness

Scheduler + unit combined: 176/176 passing.

Not changed (with rationale)

• Module reload caching: kept intentional. We hit exactly this gotcha during development — a cached import of pr_reviewer.* served stale code after a template update and produced a misleading 500. The docstring now documents the trade-off. mtime-based caching is an easy add if poll cadence ever drops below seconds, but at minutes-to-hours scale the exec cost is noise.
• Lock scope in skip path: confirmed fine. _execute_schedule acquires the lock in a try/finally (see service.py:660-676 unchanged in this PR), and the skip branch inside _execute_schedule_with_lock just returns — the outer finally releases. No leak.

🤖 self-review reply

[dolho]

Pivoted the design per the architectural concern raised in chat. Pushed in c9381a7. Net diff vs. previous version: +313 / −523 (smaller, simpler).

What changed

The agent-server HTTP endpoint and its importlib loader are gone. Replaced with a backend internal endpoint that uses services/docker_service.execute_command_in_container — the same primitive already used by services/git_service.py (persistent-state allowlist, #384 S3), services/ssh_service.py, services/agent_service/terminal.py, routers/system_agent.py, adapters/message_router.py, etc.

Why

The HTTP-endpoint version had two architectural smells:

1. New system edge: scheduler → agent-server direct call. Trinity's convention is scheduler → backend → agent, with backend as the only orchestrator that talks to agent containers.
2. Novel pattern: dynamic importlib loading of template-supplied Python in a router was unprecedented in Trinity's agent-server. Reviewers (legitimately) had to reason about caching, module re-execution, and contract validation.

Fixed by routing through the backend via docker exec, which is the dominant pattern. Topology now matches every other agent-side execution flow.

New topology

trinity-scheduler
   ↓ POST /api/internal/agents/{name}/pre-check (X-Internal-Secret)
trinity-backend
   ↓ docker exec agent-{name} test -f /home/developer/.trinity/pre-check.py
   ↓ docker exec agent-{name} python3 /home/developer/.trinity/pre-check.py
agent container (.trinity/pre-check.py — standalone executable script)
   → stdout = chat prompt    (scheduler fires with override)
   → empty + exit 0          (scheduler records skipped)
   → non-zero exit           (scheduler fail-opens, fires with schedule.message)

No HTTP endpoint on agent-server. No new long-lived process. Contract is just exit code + stdout.

Files

• Deleted: docker/base-image/agent_server/routers/pre_check.py, tests/unit/test_pre_check_router.py (whole router gone, no tests needed for it)
• Added: POST /api/internal/agents/{name}/pre-check in routers/internal.py (~50 lines, uses existing execute_command_in_container)
• Rewritten: _run_pre_check in src/scheduler/service.py — now calls the backend endpoint and translates {hook_present, exit_code, stdout} into the existing {fire, message, reason} shape so _execute_schedule_with_lock is unchanged
• Removed: AgentClient.pre_check from src/scheduler/agent_client.py
• Rewritten: tests/scheduler_tests/test_pre_check.py — 13 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)
• Updated: architecture.md, requirements.md, feature-flows/scheduler-pre-check.md, feature-flows.md index — all now describe the docker-exec topology

Template side (dolho/pr-reviewer-agent 4330d2a): .trinity/pre-check.py rewritten as a standalone shebanged script. Prints chat prompt to stdout if work, exits 0 empty otherwise. No check() wrapper, no importlib dance — just a script.

Tests

• 162/162 scheduler suite passing (was 161; +1 for the new malformed-JSON case)
• Tests previously living in tests/unit/test_pre_check_router.py are gone with the router

Architectural invariants preserved

• fix: Add process documentation volume mounts for production #11 (Docker as source of truth): docker exec matches the established Trinity pattern for agent-container command execution
• "Backend is the single orchestrator": scheduler doesn't open a direct edge to agent-server containers
• Fix: Add missing Docker labels to system agent container #1 (router → service → db): backend endpoint follows the standard layering

Live test status

The HTTP-endpoint version was end-to-end verified yesterday (skip + fire + dedup paths). The docker-exec version has unit-test coverage but hasn't been live-deployed yet — happy to spin up locally and re-verify before merge if desired, or merge first since the contract surface is smaller and the failure modes are the same.

🤖 self-review-driven refactor

[dolho]

@vybe REady for review

[vybe]

⚠️ Wrong base branch — this PR targets main directly but all feature work must land in dev first per our branching model (feature/* → dev → main).

Please either:

• Rebase this branch onto dev and update the base branch, or
• Close this PR and open a new one targeting dev

See docs/DEVELOPMENT_WORKFLOW.md for the full SDLC.

[dolho]

@vybe I've updated the branch, ready for review

[vybe]

LGTM. Clean implementation, full docs, 162/162 tests passing. Fail-open design is correct. Architecture invariants preserved.