From 6b56468421ae09dea69e6c77de07c12ce7bd1569 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Sun, 15 Nov 2026 11:43:54 +0800 Subject: [PATCH] fix: print the scheduled next run in the daily digest (#713) --- changelog.d/713.fixed.md | 1 + docs/architecture.md | 6 +- ...17-one-next-run-decision-three-surfaces.md | 66 ++++++++++++++ src/jobs/service.ts | 34 ++++++- src/lib/nextRunFor.ts | 60 +++++++++++++ test/jobs.test.ts | 88 +++++++++++++++++++ test/nextScheduledRun.test.ts | 67 ++++++++++++++ 7 files changed, 318 insertions(+), 4 deletions(-) create mode 100644 changelog.d/713.fixed.md create mode 100644 docs/decisions/0017-one-next-run-decision-three-surfaces.md create mode 100644 src/lib/nextRunFor.ts diff --git a/changelog.d/713.fixed.md b/changelog.d/713.fixed.md new file mode 100644 index 00000000..d56a91be --- /dev/null +++ b/changelog.d/713.fixed.md @@ -0,0 +1 @@ +The daily digest's next-run line now uses the same rule as the repository page and dashboard. A repository with a saved schedule row is answered by the planner, and a disabled, not-ready, idle, active-job or never-run repository prints the page's sentence instead of an interval time; a disabled repository with a saved row keeps the planner's instant. The email no longer promises a run that the page says will not start, and a cron repository no longer gets a different next-run time in the digest. diff --git a/docs/architecture.md b/docs/architecture.md index bc689515..148f764b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -144,7 +144,11 @@ defect. because that row owns its cadence; the dashboard route passes the instant with the repository's facts and both surfaces render it through `plannedNextLine`, so the card cannot fall back to the interval rule for a scheduled repository and - disagree with the page below it. The dashboard route passes open work, the + disagree with the page below it. The daily digest is the third surface: `buildDigest` + calls `nextRunFor` (`src/lib/nextRunFor.ts`), which makes the same row-vs-interval + choice and delegates the instant to `nextTime`/`dueAfterMs`, so the email prints + the same state and time as the page. The digest owns only its Chinese wording; it + does not import `src/views/**`. The dashboard route passes open work, the active job and instance readiness per repository because those live outside `RepoRow`; the two surfaces therefore print one string, and the wording says "next run", never an estimate. The newest job is read on its own, not diff --git a/docs/decisions/0017-one-next-run-decision-three-surfaces.md b/docs/decisions/0017-one-next-run-decision-three-surfaces.md new file mode 100644 index 00000000..2ba94e91 --- /dev/null +++ b/docs/decisions/0017-one-next-run-decision-three-surfaces.md @@ -0,0 +1,66 @@ +# 0017 — one next-run decision, three surfaces + +Status: **accepted** (2026-11-15) +Date: 2026-11-15 +Supersedes: nothing; it applies the one-next-run rule to the daily digest + +## Context + +The repository page and the dashboard already print one next-run time per +repository. The rule that picks it is decided by the repository, not by the +surface: a repository with a saved schedule row is answered by the planner +(`src/schedules/next.ts`, `nextTime`), and a repository without one is answered +by the legacy interval rule (`src/lib/nextRun.ts`, `nextRun`, through +`dueAfterMs`). + +The daily digest was the third surface and the only one outside that rule. +`buildDigest` (`src/jobs/service.ts`) computed `last_run_at + min(interval, +workCooldownMinutes)` for every `auto_iterate = 1` repository. It never read +`store.scheduleForRepo()`, `repos.enabled` or the interval floor, so a cron +repository got a different time in the email, and a disabled repository was +promised a run the page said would never start. The digest cannot import the +view helpers that make this choice in `src/views/pages.ts`. + +## Decision + +Add `src/lib/nextRunFor.ts` and use it from `buildDigest`: + +- `nextRunFor` takes the `RepoRow`, the optional `ScheduleRow`, open-work, + active-job and instance-ready facts, plus the work cooldown. +- A saved row is answered by `scheduleFromRow` + `nextTime`; a row the planner + cannot read answers `unknown-unreadable`, and a closed window answers + `unknown-closed`. The state for a live instant is `due` when it is at or + before `now`, otherwise `scheduled`, exactly as the page renders it. +- A repository without a row delegates to `nextRun`, so the interval instant, + the disabled/not-ready/auto-off/active-job/idle/waiting-first-run states and + the 15-minute floor stay in one place. +- The digest maps those states to its own Chinese sentences. It does not import + `src/views/**`; only the wording is duplicated, because the email is a + Chinese-only surface today. + +The instance-ready fallback stays in the caller: `config.flybot.instanceImage +=== '' ? true : instance?.status === 'ready'`, the same guard the page and +dashboard use, so a deployment with no FlyBot image never prints "not ready". + +## Rejected + +- Keeping the formula in `buildDigest`: it is a second implementation of the + rule, and the page already proves the failure mode. +- Widening `NextRunState` with the two planner answers: `nextRunLine` switches on + that union exhaustively, so a new member would drag a view change into this + slice. +- Moving `nextRunLine`/`plannedNextLine` out of `src/views/pages.ts`: that + re-points every view reference and the route's `ScheduledNext` contract for a + one-line email contradiction. + +## Consequences + +- The choice between a saved row and the interval rule now lives in the route + (for the two existing surfaces) and in `nextRunFor` (for the digest). The + instant itself remains single-sourced in `nextTime`/`dueAfterMs`; a later card + can collapse the route onto the helper if the view-layer refactor is worth it. +- The digest now prints the page's state sentences for disabled, + not-ready, auto-off, active-job, idle and never-run repositories, and the + planner instant for a scheduled repository — including a disabled repository + that has a saved row, where the row decides the cadence on both surfaces. +- `src/jobs/` remains a caller of the rule, never its owner. diff --git a/src/jobs/service.ts b/src/jobs/service.ts index 9a97d4bc..cd289b19 100644 --- a/src/jobs/service.ts +++ b/src/jobs/service.ts @@ -17,6 +17,7 @@ import { classifyFailure, providerRefusalCode, type EnqueueRefusalCode, type Fai import { parseModelRun } from '../lib/modelRun.js' import { retryDecision, reviewRetryDecision } from '../lib/retry.js' import { jobEmailDecision } from '../lib/emailPolicy.js' +import { nextRunFor, type NextRunFor } from '../lib/nextRunFor.js' import { duplicateOutcome, issueNumberFromText } from '../lib/duplicates.js' import { incCounter, observeHistogram, setGauge } from '../lib/metrics.js' import { checkStage, parseStageMode } from '../lib/stages.js' @@ -2611,6 +2612,26 @@ export class Jobs { }).format(value) } + // The digest's next-run line is the same decision the page and dashboard make, + // but the email owns its own Chinese wording because `src/jobs` must not import + // `src/views/**` (#713). The instant itself comes from `nextRunFor`, which + // delegates to `nextTime`/`dueAfterMs`, so the three surfaces cannot disagree + // about the time. + private digestNextRunLine(next: NextRunFor): string { + switch (next.state) { + case 'disabled': return '仓库已停用，自动运行不会开始' + case 'not-ready': return 'FlyBot 实例未就绪，自动运行不会开始' + case 'auto-off': return '自动工作已关闭' + case 'active-job': return '当前任务结束后再排' + case 'idle': return '空闲：没有开放工作项；会按发现间隔检查' + case 'waiting-first-run': return '等待首次运行' + case 'due': return '等待调度' + case 'scheduled': return this.digestTime(new Date(next.at ?? Date.now())) + case 'unknown-unreadable': return 'unknown（排期无法解析）' + case 'unknown-closed': return 'unknown（排期窗口已结束）' + } + } + private async buildDigest(user: UserRow, repos: RepoRow[], date: string): Promise { const since = new Date(Date.now() - 24 * 60 * 60 * 1_000).toISOString() const jobs = this.store.listJobsSince(user.id, since) @@ -2622,9 +2643,16 @@ export class Jobs { const label = `${repo.upstream_owner || repo.owner}/${repo.upstream_name || repo.name}` teamLinks.push({ repo: label, url: `${this.config.baseUrl}/repos/${repo.id}#inbox` }) if (repo.auto_iterate === 1) { - const last = repo.last_run_at === null ? Number.NaN : Date.parse(repo.last_run_at) - const next = Number.isFinite(last) ? new Date(last + Math.min(repo.interval_minutes, this.config.runner.workCooldownMinutes) * 60_000) : new Date() - nextRuns.push({ repo: label, when: next.getTime() <= Date.now() ? '等待调度' : this.digestTime(next) }) + const instance = this.store.getRepoInstance(repo.id) + const next = nextRunFor({ + repo, + row: this.store.scheduleForRepo(repo.id), + openWork: this.store.listOpenWorkItems(repo.id).length > 0, + activeJob: this.store.activeJobForRepo(repo.id) !== undefined, + instanceReady: this.config.flybot.instanceImage === '' ? true : instance?.status === 'ready', + workCooldownMinutes: this.config.runner.workCooldownMinutes, + }) + nextRuns.push({ repo: label, when: this.digestNextRunLine(next) }) } if (this.instances === undefined || this.config.flybot.instanceImage === '') continue try { diff --git a/src/lib/nextRunFor.ts b/src/lib/nextRunFor.ts new file mode 100644 index 00000000..6d15f3f0 --- /dev/null +++ b/src/lib/nextRunFor.ts @@ -0,0 +1,60 @@ +// #713: the third surface of the one next-run rule. +// +// `src/lib/nextRun.ts` owns the legacy interval answer and +// `src/schedules/next.ts` owns the planner answer. The repository page and the +// dashboard already pick between them in the view layer, which a job module +// cannot call. This function is that choice, moved where the digest can reach it +// without importing `src/views/**`. +// +// The instant stays single-sourced: the interval branch delegates to `nextRun` +// (and through it `dueAfterMs`), and the schedule branch delegates to +// `scheduleFromRow` + `nextTime`. Only the state names are this module's own. +import type { RepoRow, ScheduleRow } from '../db.js' +import { nextRun, type NextRunState } from './nextRun.js' +import { nextTime } from '../schedules/next.js' +import { scheduleFromRow } from '../schedules/tick.js' + +export type NextRunForState = NextRunState | 'unknown-unreadable' | 'unknown-closed' + +export interface NextRunForInput { + repo: RepoRow + /** Present only when the repository has saved a schedule row. */ + row: ScheduleRow | undefined + openWork: boolean + activeJob: boolean + /** + * The caller applies the same fallback the page and dashboard use + * (`config.flybot.instanceImage === '' ? true : instance?.status === 'ready'`), + * so a deployment with no FlyBot image never reports `not-ready`. + */ + instanceReady: boolean + workCooldownMinutes: number + now?: number +} + +export interface NextRunFor { + at: number | null + state: NextRunForState +} + +export function nextRunFor(input: NextRunForInput): NextRunFor { + const now = input.now ?? Date.now() + if (input.row !== undefined) { + const schedule = scheduleFromRow(input.row) + if (schedule === null) return { at: null, state: 'unknown-unreadable' } + const at = nextTime(schedule, now) + if (at === null) return { at: null, state: 'unknown-closed' } + return { at, state: at <= now ? 'due' : 'scheduled' } + } + return nextRun({ + lastRunAt: input.repo.last_run_at, + intervalMinutes: input.repo.interval_minutes, + workCooldownMinutes: input.workCooldownMinutes, + autoIterate: input.repo.auto_iterate === 1, + hasOpenWork: input.openWork, + hasActiveJob: input.activeJob, + repoEnabled: input.repo.enabled === 1, + instanceReady: input.instanceReady, + now, + }) +} diff --git a/test/jobs.test.ts b/test/jobs.test.ts index f8700d64..3a721729 100644 --- a/test/jobs.test.ts +++ b/test/jobs.test.ts @@ -9,6 +9,8 @@ import { buildCarryOver, buildRunDigest, classifyOutput, decideNextJob, failureS import type { Runner, RunnerContext } from '../src/jobs/runner.js' import { encryptSecret, sha256Hex } from '../src/lib/secrets.js' import { REVIEW_INSTRUCTION_MAX, REVIEW_INSTRUCTION_LABEL } from '../src/lib/followUpInstruction.js' +import { nextTime } from '../src/schedules/next.js' +import { scheduleFromRow } from '../src/schedules/tick.js' import { json, makeUser, MemoryMailer, readBody, startFakeServer, testConfig } from './helpers.js' test('iterate dispatches to the Chief and review to the Reviewer', async () => { @@ -395,6 +397,92 @@ test('daily digest sends once and includes the day summary', async () => { } }) +function digestTime(at: number): string { + return new Intl.DateTimeFormat('zh-CN', { + timeZone: 'Asia/Shanghai', + month: '2-digit', + day: '2-digit', + hour: '2-digit', + minute: '2-digit', + hourCycle: 'h23', + }).format(new Date(at)) +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') +} + +test('the digest prints the planner next run for a scheduled repository', async () => { + const config = testConfig({ digest: { hour: 0, timeZone: 'Asia/Shanghai' } }) + const store = new Store(':memory:') + const mailer = new MemoryMailer() + const jobs = new Jobs(store, config, { name: 'noop', async run() { return { status: 'running' } } }, mailer) + try { + const user = makeUser(store, { login: 'digest-schedule', status: 'approved' }) + const now = Date.now() + const repo = store.createRepo({ id: 'repo_digest_schedule', userId: user.id, owner: 'o', name: 'schedule', defaultBranch: 'main', tokenEnc: 'x', permission: 'admin', repoRole: 'maintainer', goal: '', autoIterate: true, intervalMinutes: 1440 }) + store.updateRepo(repo.id, { last_run_at: new Date(now - 10 * 60_000).toISOString() }) + const row = store.saveSchedule({ id: 'sch_digest', repoId: repo.id, kind: 'cron', expression: '0 9 * * 1-5', timezone: 'Asia/Shanghai' }) + const schedule = scheduleFromRow(row) + assert.ok(schedule !== null) + const expected = digestTime(nextTime(schedule, Date.now())!) + const intervalAnswer = digestTime(now - 10 * 60_000 + 15 * 60_000) + assert.notEqual(expected, intervalAnswer, 'the fixture separates the planner answer from the interval answer') + + await jobs.tick() + const digest = mailer.messages.find((message) => /今日摘要/.test(message.subject)) + assert.ok(digest, 'the digest was sent') + assert.match(digest.text, new RegExp(`下次自动迭代：o/schedule · ${escapeRegExp(expected)}`)) + assert.doesNotMatch(digest.text, new RegExp(escapeRegExp(intervalAnswer))) + } finally { + store.close() + } +}) + +test('the digest says a disabled repository will not run when it has no schedule row', async () => { + const config = testConfig({ digest: { hour: 0, timeZone: 'Asia/Shanghai' } }) + const store = new Store(':memory:') + const mailer = new MemoryMailer() + const jobs = new Jobs(store, config, { name: 'noop', async run() { return { status: 'running' } } }, mailer) + try { + const user = makeUser(store, { login: 'digest-disabled', status: 'approved' }) + const repo = store.createRepo({ id: 'repo_digest_disabled', userId: user.id, owner: 'o', name: 'disabled', defaultBranch: 'main', tokenEnc: 'x', permission: 'admin', repoRole: 'maintainer', goal: '', autoIterate: true, intervalMinutes: 1440 }) + store.updateRepo(repo.id, { enabled: 0, last_run_at: new Date(Date.now() - 10 * 60_000).toISOString() }) + + await jobs.tick() + const digest = mailer.messages.find((message) => /今日摘要/.test(message.subject)) + assert.ok(digest, 'the digest was sent') + assert.match(digest.text, /下次自动迭代：o\/disabled · 仓库已停用，自动运行不会开始/) + assert.doesNotMatch(digest.text, /下次自动迭代：o\/disabled · \d{2}\/\d{2}/, 'no instant is promised for a disabled repository') + } finally { + store.close() + } +}) + +test('the digest keeps the planner instant for a disabled repository with a schedule row', async () => { + const config = testConfig({ digest: { hour: 0, timeZone: 'Asia/Shanghai' } }) + const store = new Store(':memory:') + const mailer = new MemoryMailer() + const jobs = new Jobs(store, config, { name: 'noop', async run() { return { status: 'running' } } }, mailer) + try { + const user = makeUser(store, { login: 'digest-disabled-scheduled', status: 'approved' }) + const repo = store.createRepo({ id: 'repo_digest_disabled_scheduled', userId: user.id, owner: 'o', name: 'disabled-scheduled', defaultBranch: 'main', tokenEnc: 'x', permission: 'admin', repoRole: 'maintainer', goal: '', autoIterate: true, intervalMinutes: 1440 }) + store.updateRepo(repo.id, { enabled: 0, last_run_at: new Date(Date.now() - 10 * 60_000).toISOString() }) + const row = store.saveSchedule({ id: 'sch_digest_disabled', repoId: repo.id, kind: 'cron', expression: '0 9 * * 1-5', timezone: 'Asia/Shanghai' }) + const schedule = scheduleFromRow(row) + assert.ok(schedule !== null) + const expected = digestTime(nextTime(schedule, Date.now())!) + + await jobs.tick() + const digest = mailer.messages.find((message) => /今日摘要/.test(message.subject)) + assert.ok(digest, 'the digest was sent') + assert.match(digest.text, new RegExp(`下次自动迭代：o/disabled-scheduled · ${escapeRegExp(expected)}`)) + assert.doesNotMatch(digest.text, /仓库已停用，自动运行不会开始/, 'a saved row answers before enabled, exactly as on the page') + } finally { + store.close() + } +}) + test('plan jobs dispatch as plan and keep a checkpoint', async () => { const config = testConfig() const store = new Store(':memory:') diff --git a/test/nextScheduledRun.test.ts b/test/nextScheduledRun.test.ts index 2ccc301d..52b1de99 100644 --- a/test/nextScheduledRun.test.ts +++ b/test/nextScheduledRun.test.ts @@ -4,6 +4,7 @@ import { buildApp } from '../src/app.js' import { Store, type WorkItemRow } from '../src/db.js' import { MockRunner } from '../src/jobs/mock.js' import { nextRun } from '../src/lib/nextRun.js' +import { nextRunFor } from '../src/lib/nextRunFor.js' import { humanTimeShort } from '../src/lib/time.js' import { nextTime } from '../src/schedules/next.js' import { scheduleFromRow } from '../src/schedules/tick.js' @@ -112,6 +113,72 @@ function renderBoth(input: { return { dashboard, page } } +test('the shared next-run helper chooses the planner for a saved row', () => { + const { store, repo } = fixture() + try { + const row = store.saveSchedule({ id: 'sch_for', repoId: repo.id, kind: 'cron', expression: '0 9 * * 1-5', timezone: 'Asia/Shanghai' }) + const schedule = scheduleFromRow(row) + assert.ok(schedule !== null) + const expected = nextTime(schedule, NOW) + assert.ok(expected !== null) + const next = nextRunFor({ repo, row, openWork: true, activeJob: false, instanceReady: true, workCooldownMinutes: COOLDOWN_MINUTES, now: NOW }) + assert.equal(next.state, 'scheduled') + assert.equal(next.at, expected) + store.updateRepo(repo.id, { enabled: 0 }) + const disabled = nextRunFor({ repo: store.getRepo(repo.id)!, row, openWork: true, activeJob: false, instanceReady: true, workCooldownMinutes: COOLDOWN_MINUTES, now: NOW }) + assert.equal(disabled.state, 'scheduled', 'a saved row answers before enabled, exactly as on the page') + assert.equal(disabled.at, expected) + + const unreadable = store.saveSchedule({ id: 'sch_for_bad', repoId: repo.id, kind: 'cron', expression: null, timezone: 'Asia/Shanghai' }) + assert.deepEqual( + nextRunFor({ repo: store.getRepo(repo.id)!, row: unreadable, openWork: true, activeJob: false, instanceReady: true, workCooldownMinutes: COOLDOWN_MINUTES, now: NOW }), + { at: null, state: 'unknown-unreadable' }, + ) + const closed = store.saveSchedule({ id: 'sch_for_closed', repoId: repo.id, kind: 'interval', everyMs: 60_000, endAt: NOW - 1 }) + assert.deepEqual( + nextRunFor({ repo: store.getRepo(repo.id)!, row: closed, openWork: true, activeJob: false, instanceReady: true, workCooldownMinutes: COOLDOWN_MINUTES, now: NOW }), + { at: null, state: 'unknown-closed' }, + ) + } finally { + store.close() + } +}) + +test('the shared next-run helper keeps every interval state the page prints', () => { + const cases: Array<{ label: string; patch: { enabled?: number; autoIterate?: boolean; lastRunAt?: string | null; openWork?: boolean; activeJob?: boolean; instanceReady?: boolean }; state: string }> = [ + { label: 'disabled', patch: { enabled: 0 }, state: 'disabled' }, + { label: 'not-ready', patch: { instanceReady: false }, state: 'not-ready' }, + { label: 'auto-off', patch: { autoIterate: false }, state: 'auto-off' }, + { label: 'active-job', patch: { activeJob: true }, state: 'active-job' }, + { label: 'idle', patch: { openWork: false }, state: 'idle' }, + { label: 'waiting-first-run', patch: { lastRunAt: null }, state: 'waiting-first-run' }, + { label: 'due', patch: { lastRunAt: new Date(NOW - 60 * 60_000).toISOString() }, state: 'due' }, + { label: 'scheduled', patch: { lastRunAt: LAST_RUN_RECENT }, state: 'scheduled' }, + ] + for (const testCase of cases) { + const { store, repo } = fixture() + try { + store.updateRepo(repo.id, { + enabled: testCase.patch.enabled ?? 1, + auto_iterate: testCase.patch.autoIterate === false ? 0 : 1, + ...(testCase.patch.lastRunAt === undefined ? {} : { last_run_at: testCase.patch.lastRunAt }), + }) + const next = nextRunFor({ + repo: store.getRepo(repo.id)!, + row: undefined, + openWork: testCase.patch.openWork ?? true, + activeJob: testCase.patch.activeJob ?? false, + instanceReady: testCase.patch.instanceReady ?? true, + workCooldownMinutes: COOLDOWN_MINUTES, + now: NOW, + }) + assert.equal(next.state, testCase.state, `${testCase.label} keeps the page's state`) + } finally { + store.close() + } + } +}) + test('the cooldown the worker uses decides the next run, not the discovery interval', () => { const next = nextRun({ lastRunAt: LAST_RUN_AT,