feat(scheduler): internalize groomer, pr-followup, prune-closed#520
feat(scheduler): internalize groomer, pr-followup, prune-closed#520joryirving wants to merge 1 commit into
Conversation
Extends the in-app scheduler (#502) with the remaining periodic jobs so all external heartbeat cronjobs can be retired: - groomer -> POST /api/groomer/run (default 10m; now safe — #503 added a run lock) - pr-followup -> POST /api/pr-followup/sync (default 15m) - prune-closed -> POST /api/issues/prune-closed (default 24h) All authed with DISPATCH_AGENT_TOKEN (the groomer endpoint accepts it via authorizeGroomerRequest's fallback). Each job's interval env can be set to "0" to disable it individually. Closes #504
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
Recommendation: Request Changes
This PR adds the groomer, pr-followup, and prune-closed jobs to the in-app scheduler. The implementation logic appears sound, but it cannot be approved due to a CI failure on the Docker Build step. The PR body claims verification passed locally, but CI has not confirmed these results for this specific commit.
Change-by-Change Findings
1. AGENTS.md — Environment Variable Documentation
Verdict: Correct
The four new environment variables are correctly documented with:
DISPATCH_GROOMER_INTERVAL_MS(default 10m)DISPATCH_PR_FOLLOWUP_INTERVAL_MS(default 15m)DISPATCH_PRUNE_CLOSED_INTERVAL_MS(default 24h)- Updated
DISPATCH_SCHEDULER_ENABLEDdescription to list all jobs
This matches the schema from src/lib/scheduler.ts and aligns with the environment variable convention established for DISPATCH_SYNC_INTERVAL_MS.
2. src/lib/scheduler.test.ts — Test Coverage
Verdict: Correct
Two new test cases added:
configures sync + groomer + pr-followup + prune-closed with defaults: Verifies all four jobs are configured with correct paths and intervalsdisables an individual job when its interval env is 0: Verifies that setting an interval to "0" filters out that job from the jobs array
Existing test honors DISPATCH_SYNC_INTERVAL_MS and falls back on garbage is refactored to use a helper function for job lookup, maintaining backward compatibility.
3. src/lib/scheduler.ts — Scheduler Implementation
Verdict: Correct
Key implementation details verified:
| Job | Path | Default Interval |
|---|---|---|
| sync | /api/sync/scheduled |
15m |
| groomer | /api/groomer/run |
10m |
| pr-followup | /api/pr-followup/sync |
15m |
| prune-closed | /api/issues/prune-closed |
24h |
- New
jobIntervalFromEnv()function correctly handles: unset → fallback, "0" → disabled (returns 0), invalid → fallback - Jobs filtered with
.filter((job) => job.intervalMs > 0)ensures disabled jobs don't appear in the jobs array - Authorization note in docstring confirms
DISPATCH_AGENT_TOKENis used (matching existingtokenfield in config)
Linked Issue Fit
Issue PR 504 requirements verified:
| Requirement | Status | Evidence |
|---|---|---|
Groomer job (POST /api/groomer/run) |
✅ | Path and interval correctly configured |
PR-followup job (POST /api/pr-followup/sync) |
✅ | Path and interval correctly configured |
Prune-closed job (POST /api/issues/prune-closed) |
✅ | Path and interval correctly configured |
Per-job env vars (DISPATCH_GROOMER_INTERVAL_MS, etc.) |
✅ | Documented in AGENTS.md and implemented |
| Jobs can be disabled via "0" | ✅ | jobIntervalFromEnv handles this |
| Uses existing endpoints (loopback POSTs) | ✅ | All paths match existing route files |
Auth with DISPATCH_AGENT_TOKEN |
✅ | Documented; scheduler config already uses this token |
The PR references PR 503 for the groomer DB lock safety. The repository history confirms commit 4d6529a added "serialize runs behind a DB lock" — this prerequisite is satisfied.
Standards Compliance
| Convention | Status | Notes |
|---|---|---|
| No agent-specific names in generic docs | ✅ | Uses generic agent/* in documentation |
| Prisma schema unchanged | ✅ | No schema modifications |
| API routes return appropriate status codes | ✅ | No API changes; uses existing endpoints |
Error handling via error instanceof Error |
✅ | No new error paths introduced |
| No secrets committed | ✅ | No secrets in diff |
| Environment variable naming convention | ✅ | DISPATCH_*_INTERVAL_MS pattern matches existing |
Evidence Provider Findings
No evidence providers were configured for this PR.
Tool Harness Findings
All three targeted endpoint paths were verified to exist in the repository:
| Path | Found In |
|---|---|
/api/groomer/run |
Multiple route files and docs |
/api/pr-followup/sync |
Route file and scheduler |
/api/issues/prune-closed |
Route file and scheduler |
The endpoints exist and the paths in the scheduler configuration are correct.
CI Check Results
CRITICAL BLOCKER: The CI status shows Docker Build: failure for this commit. While Tests, Build, Lint, and Typecheck are still in_progress, the Docker Build failure indicates an issue that must be resolved.
The PR description states:
vitest run src/lib/scheduler.test.ts→ 12 passing (2 new)
tsc→ 0;eslint→ clean
However, these results are from the author's local environment, not from CI verification for commit 4792cd50e0d1729992aa45eb1b7a87301fceffe9. CI must pass before this PR can be merged.
Unknowns / Needs Verification
-
Docker Build failure cause: The reason for the Docker Build failure is not visible in the provided corpus. This could be:
- A transient infrastructure issue
- An actual build problem with this commit
- A pre-existing issue unrelated to these changes
-
Test results from CI: The PR body claims test verification passed, but CI's
Testscheck is stillin_progress. The actual test results for this commit are not yet available in the corpus.
Summary
The implementation is logically correct and satisfies all requirements from issue PR 504:
- All three new jobs are correctly configured with proper paths and intervals
- Environment variable documentation is accurate
- New tests cover the key behaviors (defaults, disabling via "0")
- The groomer DB lock prerequisite (PR 503) is confirmed present in repository history
However, this PR must wait for CI to pass. The Docker Build failure is a blocker. Once CI confirms all checks pass (Tests, Build, Lint, Typecheck, Docker Build), this PR can be approved.
Summary
/api/groomer/run, 10m), pr-followup (/api/pr-followup/sync, 15m), prune-closed (/api/issues/prune-closed, 24h). With these, the external heartbeat cronjobs can be retired (home-ops change follows).DISPATCH_AGENT_TOKEN(the groomer endpoint accepts it viaauthorizeGroomerRequest's fallback). Each job's*_INTERVAL_MSenv can be0to disable it individually.Safety
Verification
vitest run src/lib/scheduler.test.ts→ 12 passing (2 new: all four jobs configured with defaults;0disables a job).tsc→ 0;eslint→ clean.Closes #504 · part of #498