diff --git a/changelog.d/1884-p255-task-done-median.fixed.md b/changelog.d/1884-p255-task-done-median.fixed.md new file mode 100644 index 000000000..9c3e6e528 --- /dev/null +++ b/changelog.d/1884-p255-task-done-median.fixed.md @@ -0,0 +1 @@ +The Project board's task-completion median now counts a card the way the task store finishes it — `status: 'accepted'`, timed from the last terminal `actors[]` entry — instead of the `completedAt` field the store never writes, so the number stops reading `—` on a real deployment ([#1884](https://github.com/ThinkFlowLab/FlyBot/issues/1884)). diff --git a/core/src/host/project-metrics.js b/core/src/host/project-metrics.js index c7c2f4954..744532fca 100644 --- a/core/src/host/project-metrics.js +++ b/core/src/host/project-metrics.js @@ -3,6 +3,8 @@ // `unavailable` and every value reads `null`, never `0`: a zero claims "none today" and the board // must not turn "cannot read" into a number. The reads are injected, so a test counts calls without // a network, and the day boundary is the reader's own time zone, not the host's UTC clock. +import { columnOf } from '../shared/task-board.js' + export const METRICS_TTL_MS = 5 * 60 * 1000 export const METRIC_KEYS = [ 'prsOpenedToday', 'prsMergedToday', 'issuesOpenedToday', 'issuesClosedToday', @@ -67,13 +69,27 @@ const durationInWindow = (items, from, to, now) => items.reduce((values, item) = // as a map, so a caller handing the whole store keeps its old answer. const taskList = tasks => (Array.isArray(tasks) ? tasks : Object.values(tasks ?? {})) -// The task half: a card that reached `done` inside the window. `completedAt` is the store's own -// stamp; a record written before the stamp reads `updatedAt` and is not counted as a duration the -// store never recorded. +// The task half: a card that really completed. The board's own reading decides that column +// (`columnOf`, so no second status table exists here), and `accepted` is the one column value that +// means "finished": a cancelled or failed card is in the done column but has no completion moment, +// so timing it from its cancellation would only stretch the median. A card finishes with +// `transition(task, 'accepted')`, which writes `status` and pushes a terminal entry onto `actors`; +// `completedAt` is not a field that path ever writes, so it is only a fallback for older records, +// after the last terminal actor's `at`. +const finished = task => columnOf(task) === 'done' && task.status === 'accepted' +const finishedAt = task => { + const actors = Array.isArray(task.actors) ? task.actors : [] + for (let index = actors.length - 1; index >= 0; index -= 1) { + const entry = actors[index] + if (entry !== null && typeof entry === 'object' && finished(entry) && Number.isFinite(entry.at)) return entry.at + } + if (Number.isFinite(task.completedAt)) return task.completedAt + return Number.isFinite(task.updatedAt) ? task.updatedAt : null +} const doneTaskDurations = (tasks, now) => taskList(tasks).reduce((values, task) => { - if (task === null || typeof task !== 'object' || task.status !== 'done') return values + if (task === null || typeof task !== 'object' || !finished(task)) return values const start = Number.isFinite(task.createdAt) ? task.createdAt : null - const end = Number.isFinite(task.completedAt) ? task.completedAt : null + const end = finishedAt(task) if (start === null || end === null || end < start) return values if (now - end > RECENT_MS) return values values.push(end - start) diff --git a/core/test/project-metrics-scope.test.js b/core/test/project-metrics-scope.test.js index 45599d69f..ec229aabf 100644 --- a/core/test/project-metrics-scope.test.js +++ b/core/test/project-metrics-scope.test.js @@ -40,10 +40,10 @@ const seed = () => ({ tasks: { // This board: one card in flight, one finished an hour after it started. 'task-group': { id: 'task-group', kind: 'group', status: 'claimed', claimedBy: 'Writer', claimedAt: NOW - 2 * HOUR, origin: { roomId: 'r1' }, ownerBotId: 'a', assignee: 'Writer', title: 'Ship it', createdAt: 5, updatedAt: NOW - HOUR }, - 'task-done-a': { id: 'task-done-a', kind: 'group', status: 'done', origin: { roomId: 'r1' }, ownerBotId: 'a', title: 'Closed on p1', createdAt: NOW - 2 * HOUR, completedAt: NOW - HOUR, updatedAt: NOW - HOUR }, + 'task-done-a': { id: 'task-done-a', kind: 'group', status: 'accepted', origin: { roomId: 'r1' }, ownerBotId: 'a', title: 'Closed on p1', createdAt: NOW - 2 * HOUR, completedAt: NOW - HOUR, updatedAt: NOW - HOUR }, // The other Project: same shapes, and nine hours from start to done. 'task-foreign': { id: 'task-foreign', kind: 'group', status: 'claimed', claimedBy: 'Foreigner', claimedAt: NOW - 2 * HOUR, origin: { roomId: 'r2' }, ownerBotId: 'foreign', title: 'Secret salary review', createdAt: 5, updatedAt: NOW - HOUR }, - 'task-foreign-done': { id: 'task-foreign-done', kind: 'group', status: 'done', origin: { roomId: 'r2' }, ownerBotId: 'foreign', title: 'Foreign closed', createdAt: NOW - 10 * HOUR, completedAt: NOW - HOUR, updatedAt: NOW - HOUR }, + 'task-foreign-done': { id: 'task-foreign-done', kind: 'group', status: 'accepted', origin: { roomId: 'r2' }, ownerBotId: 'foreign', title: 'Foreign closed', createdAt: NOW - 10 * HOUR, completedAt: NOW - HOUR, updatedAt: NOW - HOUR }, }, }) diff --git a/core/test/project-metrics.test.js b/core/test/project-metrics.test.js index eed002619..6c79c42bf 100644 --- a/core/test/project-metrics.test.js +++ b/core/test/project-metrics.test.js @@ -3,7 +3,7 @@ // a failed read answers `null` (painted `—`), never `0`. import test from 'node:test' import assert from 'node:assert/strict' -import { METRICS_TTL_MS, dayKey, median, readMetrics, runningOf, summarizeMetrics } from '../src/host/project-metrics.js' +import { METRIC_KEYS, METRICS_TTL_MS, dayKey, median, readMetrics, runningOf, summarizeMetrics } from '../src/host/project-metrics.js' const HOUR = 60 * 60 * 1000 const NOW = Date.parse('2026-10-30T12:00:00Z') @@ -116,9 +116,9 @@ test('a running row carries who, how long and where from', () => { test('the task median reads the store stamps only', () => { const tasks = { - a: { status: 'done', createdAt: NOW - 10 * HOUR, completedAt: NOW - 8 * HOUR }, - b: { status: 'done', createdAt: NOW - 6 * HOUR, completedAt: NOW - 4 * HOUR }, - c: { status: 'done', createdAt: NOW - 6 * HOUR }, + a: { status: 'accepted', createdAt: NOW - 10 * HOUR, completedAt: NOW - 8 * HOUR }, + b: { status: 'accepted', createdAt: NOW - 6 * HOUR, completedAt: NOW - 4 * HOUR }, + c: { status: 'accepted', createdAt: NOW - 6 * HOUR }, d: { status: 'open', createdAt: NOW - 6 * HOUR }, } const out = summarizeMetrics({ now: NOW, timeZone: 'UTC', source: { ok: true, pullRequests: [], issues: [] }, tasks }) @@ -184,3 +184,41 @@ test('#1849: a valid zone keeps the day boundary and the answer it had', () => { // A zone `Intl` accepts but the host would never guess stays as sent, and UTC still reads one day. assert.equal(summarizeMetrics({ now: NOW, timeZone: 'UTC', source }).metrics.prsOpenedToday, 1) }) + +// #1884: a card finishes by `transition(task, 'accepted')` (core/src/host/tasks.js:269-279), which +// writes `status`, `updatedAt` and one `actors[]` row — never `completedAt`. So the duration is +// `createdAt` → the last terminal `actors[].at` (9h here), not a field the store never writes. +const finished = (id, createdAt, acceptedAt) => ({ + id, + title: id, + status: 'accepted', + createdAt, + updatedAt: acceptedAt, + actors: [ + { status: 'open', at: createdAt }, + { status: 'claimed', at: createdAt + HOUR }, + { status: 'accepted', at: acceptedAt }, + ], +}) + +test('a card that finished the way the host finishes cards is a done duration', () => { + const tasks = [ + finished('t-9h', NOW - 10 * HOUR, NOW - 1 * HOUR), + { id: 't-cancelled', status: 'cancelled', createdAt: NOW - 10 * HOUR, updatedAt: NOW - 2 * HOUR, actors: [{ status: 'cancelled', at: NOW - 2 * HOUR }] }, + ] + const out = summarizeMetrics({ now: NOW, timeZone: 'UTC', source: { ok: true, pullRequests: [], issues: [] }, tasks }) + assert.equal(out.metrics.taskDoneMedianMs, 9 * HOUR) + assert.deepEqual(Object.keys(out.metrics).sort(), [...METRIC_KEYS].sort()) +}) + +// A board whose only finished cards were abandoned was never completed on: `—`, never `0`. This one +// passes on main today only because no card is counted at all, so it is the non-regression half of +// the pair — it starts guarding the `accepted`-only rule the moment the first test goes green. +test('only cancelled and failed cards is null, never zero', () => { + const tasks = [ + { id: 't-cancelled', status: 'cancelled', createdAt: NOW - 10 * HOUR, updatedAt: NOW - 2 * HOUR, actors: [{ status: 'cancelled', at: NOW - 2 * HOUR }] }, + { id: 't-failed', status: 'failed', createdAt: NOW - 8 * HOUR, updatedAt: NOW - 3 * HOUR, actors: [{ status: 'failed', at: NOW - 3 * HOUR }] }, + ] + const out = summarizeMetrics({ now: NOW, timeZone: 'UTC', source: { ok: true, pullRequests: [], issues: [] }, tasks }) + assert.equal(out.metrics.taskDoneMedianMs, null) +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index b9fb31e65..d2a7e2684 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -790,7 +790,7 @@ the host side only, so the tier stays T2 and `core/src/shared/` is untouched. ## Project metrics (#1829) -`core/src/host/project-metrics.js` answers the Project board's seven numbers: PRs opened and merged today, issues opened and closed today, and the seven-day medians of PR merge, issue close and task completion durations. The four day counts compare `created_at` / `merged_at` / `closed_at` against the reader's own time zone (`Intl.DateTimeFormat`), not the host clock — the zone arrives in the request body, so `resolveTimeZone` validates it once and an unknown name answers the named `UTC` instead of throwing out of the whole board read, rows, queues and decisions with it (#1849) — and the local task half reads `createdAt` / `completedAt` from `state.tasks`. The running table and the task-done median read one board's own cards, not the deployment's task store: `project-board.js` hands `readMetrics` the same `mine` list its rows were built from (#1848), so a card of another Project, or one this reader is refused, appears in neither, and every branch (no credential, no fetch, a failed read, a cache hit, a success) reproduces both from that same list. The five-minute cache is shared by every Project on the repository, so an entry holds only the repository-level numbers (`ok`, the time zone, the stamp and the six GitHub counts and medians); `running` and `taskDoneMedianMs` are re-projected from the caller's cards on a hit exactly as on a miss (#1880). The numbers ride on the existing `project-board` endpoint (`store.js` carries the `metrics` section beside the rows), which passes the host secret store's `GITHUB_TOKEN` and the reader's time zone from the request, requires the Project, and answers `ok: false` with `null` values when the credential is missing or a read fails — the client paints `—`, never `0`. One five-minute in-process cache keys on repository, time zone and window, so a hit spends no source call; a failed read is not cached. `runningOf` projects a claimed card into the running table's row (who, claim age, source, last activity). +`core/src/host/project-metrics.js` answers the Project board's seven numbers: PRs opened and merged today, issues opened and closed today, and the seven-day medians of PR merge, issue close and task completion durations. The four day counts compare `created_at` / `merged_at` / `closed_at` against the reader's own time zone (`Intl.DateTimeFormat`), not the host clock — the zone arrives in the request body, so `resolveTimeZone` validates it once and an unknown name answers the named `UTC` instead of throwing out of the whole board read, rows, queues and decisions with it (#1849) — and the local task half reads `createdAt` and the last terminal `actors[].at` (falling back to `completedAt`, then `updatedAt`) and counts only `accepted` cards. The running table and the task-done median read one board's own cards, not the deployment's task store: `project-board.js` hands `readMetrics` the same `mine` list its rows were built from (#1848), so a card of another Project, or one this reader is refused, appears in neither, and every branch (no credential, no fetch, a failed read, a cache hit, a success) reproduces both from that same list. The five-minute cache is shared by every Project on the repository, so an entry holds only the repository-level numbers (`ok`, the time zone, the stamp and the six GitHub counts and medians); `running` and `taskDoneMedianMs` are re-projected from the caller's cards on a hit exactly as on a miss (#1880). The numbers ride on the existing `project-board` endpoint (`store.js` carries the `metrics` section beside the rows), which passes the host secret store's `GITHUB_TOKEN` and the reader's time zone from the request, requires the Project, and answers `ok: false` with `null` values when the credential is missing or a read fails — the client paints `—`, never `0`. One five-minute in-process cache keys on repository, time zone and window, so a hit spends no source call; a failed read is not cached. `runningOf` projects a claimed card into the running table's row (who, claim age, source, last activity). ## The Project run seam (#1856) diff --git a/patches/ds-bot/files/src/host/project-metrics.js b/patches/ds-bot/files/src/host/project-metrics.js index c7c2f4954..744532fca 100644 --- a/patches/ds-bot/files/src/host/project-metrics.js +++ b/patches/ds-bot/files/src/host/project-metrics.js @@ -3,6 +3,8 @@ // `unavailable` and every value reads `null`, never `0`: a zero claims "none today" and the board // must not turn "cannot read" into a number. The reads are injected, so a test counts calls without // a network, and the day boundary is the reader's own time zone, not the host's UTC clock. +import { columnOf } from '../shared/task-board.js' + export const METRICS_TTL_MS = 5 * 60 * 1000 export const METRIC_KEYS = [ 'prsOpenedToday', 'prsMergedToday', 'issuesOpenedToday', 'issuesClosedToday', @@ -67,13 +69,27 @@ const durationInWindow = (items, from, to, now) => items.reduce((values, item) = // as a map, so a caller handing the whole store keeps its old answer. const taskList = tasks => (Array.isArray(tasks) ? tasks : Object.values(tasks ?? {})) -// The task half: a card that reached `done` inside the window. `completedAt` is the store's own -// stamp; a record written before the stamp reads `updatedAt` and is not counted as a duration the -// store never recorded. +// The task half: a card that really completed. The board's own reading decides that column +// (`columnOf`, so no second status table exists here), and `accepted` is the one column value that +// means "finished": a cancelled or failed card is in the done column but has no completion moment, +// so timing it from its cancellation would only stretch the median. A card finishes with +// `transition(task, 'accepted')`, which writes `status` and pushes a terminal entry onto `actors`; +// `completedAt` is not a field that path ever writes, so it is only a fallback for older records, +// after the last terminal actor's `at`. +const finished = task => columnOf(task) === 'done' && task.status === 'accepted' +const finishedAt = task => { + const actors = Array.isArray(task.actors) ? task.actors : [] + for (let index = actors.length - 1; index >= 0; index -= 1) { + const entry = actors[index] + if (entry !== null && typeof entry === 'object' && finished(entry) && Number.isFinite(entry.at)) return entry.at + } + if (Number.isFinite(task.completedAt)) return task.completedAt + return Number.isFinite(task.updatedAt) ? task.updatedAt : null +} const doneTaskDurations = (tasks, now) => taskList(tasks).reduce((values, task) => { - if (task === null || typeof task !== 'object' || task.status !== 'done') return values + if (task === null || typeof task !== 'object' || !finished(task)) return values const start = Number.isFinite(task.createdAt) ? task.createdAt : null - const end = Number.isFinite(task.completedAt) ? task.completedAt : null + const end = finishedAt(task) if (start === null || end === null || end < start) return values if (now - end > RECENT_MS) return values values.push(end - start) diff --git a/patches/ds-bot/files/test/project-metrics-scope.test.js b/patches/ds-bot/files/test/project-metrics-scope.test.js index 45599d69f..ec229aabf 100644 --- a/patches/ds-bot/files/test/project-metrics-scope.test.js +++ b/patches/ds-bot/files/test/project-metrics-scope.test.js @@ -40,10 +40,10 @@ const seed = () => ({ tasks: { // This board: one card in flight, one finished an hour after it started. 'task-group': { id: 'task-group', kind: 'group', status: 'claimed', claimedBy: 'Writer', claimedAt: NOW - 2 * HOUR, origin: { roomId: 'r1' }, ownerBotId: 'a', assignee: 'Writer', title: 'Ship it', createdAt: 5, updatedAt: NOW - HOUR }, - 'task-done-a': { id: 'task-done-a', kind: 'group', status: 'done', origin: { roomId: 'r1' }, ownerBotId: 'a', title: 'Closed on p1', createdAt: NOW - 2 * HOUR, completedAt: NOW - HOUR, updatedAt: NOW - HOUR }, + 'task-done-a': { id: 'task-done-a', kind: 'group', status: 'accepted', origin: { roomId: 'r1' }, ownerBotId: 'a', title: 'Closed on p1', createdAt: NOW - 2 * HOUR, completedAt: NOW - HOUR, updatedAt: NOW - HOUR }, // The other Project: same shapes, and nine hours from start to done. 'task-foreign': { id: 'task-foreign', kind: 'group', status: 'claimed', claimedBy: 'Foreigner', claimedAt: NOW - 2 * HOUR, origin: { roomId: 'r2' }, ownerBotId: 'foreign', title: 'Secret salary review', createdAt: 5, updatedAt: NOW - HOUR }, - 'task-foreign-done': { id: 'task-foreign-done', kind: 'group', status: 'done', origin: { roomId: 'r2' }, ownerBotId: 'foreign', title: 'Foreign closed', createdAt: NOW - 10 * HOUR, completedAt: NOW - HOUR, updatedAt: NOW - HOUR }, + 'task-foreign-done': { id: 'task-foreign-done', kind: 'group', status: 'accepted', origin: { roomId: 'r2' }, ownerBotId: 'foreign', title: 'Foreign closed', createdAt: NOW - 10 * HOUR, completedAt: NOW - HOUR, updatedAt: NOW - HOUR }, }, }) diff --git a/patches/ds-bot/files/test/project-metrics.test.js b/patches/ds-bot/files/test/project-metrics.test.js index eed002619..6c79c42bf 100644 --- a/patches/ds-bot/files/test/project-metrics.test.js +++ b/patches/ds-bot/files/test/project-metrics.test.js @@ -3,7 +3,7 @@ // a failed read answers `null` (painted `—`), never `0`. import test from 'node:test' import assert from 'node:assert/strict' -import { METRICS_TTL_MS, dayKey, median, readMetrics, runningOf, summarizeMetrics } from '../src/host/project-metrics.js' +import { METRIC_KEYS, METRICS_TTL_MS, dayKey, median, readMetrics, runningOf, summarizeMetrics } from '../src/host/project-metrics.js' const HOUR = 60 * 60 * 1000 const NOW = Date.parse('2026-10-30T12:00:00Z') @@ -116,9 +116,9 @@ test('a running row carries who, how long and where from', () => { test('the task median reads the store stamps only', () => { const tasks = { - a: { status: 'done', createdAt: NOW - 10 * HOUR, completedAt: NOW - 8 * HOUR }, - b: { status: 'done', createdAt: NOW - 6 * HOUR, completedAt: NOW - 4 * HOUR }, - c: { status: 'done', createdAt: NOW - 6 * HOUR }, + a: { status: 'accepted', createdAt: NOW - 10 * HOUR, completedAt: NOW - 8 * HOUR }, + b: { status: 'accepted', createdAt: NOW - 6 * HOUR, completedAt: NOW - 4 * HOUR }, + c: { status: 'accepted', createdAt: NOW - 6 * HOUR }, d: { status: 'open', createdAt: NOW - 6 * HOUR }, } const out = summarizeMetrics({ now: NOW, timeZone: 'UTC', source: { ok: true, pullRequests: [], issues: [] }, tasks }) @@ -184,3 +184,41 @@ test('#1849: a valid zone keeps the day boundary and the answer it had', () => { // A zone `Intl` accepts but the host would never guess stays as sent, and UTC still reads one day. assert.equal(summarizeMetrics({ now: NOW, timeZone: 'UTC', source }).metrics.prsOpenedToday, 1) }) + +// #1884: a card finishes by `transition(task, 'accepted')` (core/src/host/tasks.js:269-279), which +// writes `status`, `updatedAt` and one `actors[]` row — never `completedAt`. So the duration is +// `createdAt` → the last terminal `actors[].at` (9h here), not a field the store never writes. +const finished = (id, createdAt, acceptedAt) => ({ + id, + title: id, + status: 'accepted', + createdAt, + updatedAt: acceptedAt, + actors: [ + { status: 'open', at: createdAt }, + { status: 'claimed', at: createdAt + HOUR }, + { status: 'accepted', at: acceptedAt }, + ], +}) + +test('a card that finished the way the host finishes cards is a done duration', () => { + const tasks = [ + finished('t-9h', NOW - 10 * HOUR, NOW - 1 * HOUR), + { id: 't-cancelled', status: 'cancelled', createdAt: NOW - 10 * HOUR, updatedAt: NOW - 2 * HOUR, actors: [{ status: 'cancelled', at: NOW - 2 * HOUR }] }, + ] + const out = summarizeMetrics({ now: NOW, timeZone: 'UTC', source: { ok: true, pullRequests: [], issues: [] }, tasks }) + assert.equal(out.metrics.taskDoneMedianMs, 9 * HOUR) + assert.deepEqual(Object.keys(out.metrics).sort(), [...METRIC_KEYS].sort()) +}) + +// A board whose only finished cards were abandoned was never completed on: `—`, never `0`. This one +// passes on main today only because no card is counted at all, so it is the non-regression half of +// the pair — it starts guarding the `accepted`-only rule the moment the first test goes green. +test('only cancelled and failed cards is null, never zero', () => { + const tasks = [ + { id: 't-cancelled', status: 'cancelled', createdAt: NOW - 10 * HOUR, updatedAt: NOW - 2 * HOUR, actors: [{ status: 'cancelled', at: NOW - 2 * HOUR }] }, + { id: 't-failed', status: 'failed', createdAt: NOW - 8 * HOUR, updatedAt: NOW - 3 * HOUR, actors: [{ status: 'failed', at: NOW - 3 * HOUR }] }, + ] + const out = summarizeMetrics({ now: NOW, timeZone: 'UTC', source: { ok: true, pullRequests: [], issues: [] }, tasks }) + assert.equal(out.metrics.taskDoneMedianMs, null) +})