From 1a622105a72b0e43f82e90fe332b574b2dadf0b5 Mon Sep 17 00:00:00 2001 From: Dev-4 Date: Sat, 31 Oct 2026 02:20:44 +0000 Subject: [PATCH] Count a finished Project card from the shape the store writes A card finishes with transition(task, 'accepted'), which sets status and pushes a terminal actor entry; completedAt is never written, so the task-done median was always null on a real board. Count a card when the board's own columnOf puts it in the done column and its status is accepted, and take the completion time from the last terminal actor's at, falling back to completedAt then updatedAt. A cancelled or failed card sits in that column but never completed, so it stays out of the median, and an empty board stays null rather than zero. Written-by: Dev-4 --- .../1884-p255-task-done-median.fixed.md | 1 + core/src/host/project-metrics.js | 26 +++++++++-- core/test/project-metrics-scope.test.js | 4 +- core/test/project-metrics.test.js | 46 +++++++++++++++++-- docs/subsystems/core-host.md | 2 +- .../ds-bot/files/src/host/project-metrics.js | 26 +++++++++-- .../files/test/project-metrics-scope.test.js | 4 +- .../ds-bot/files/test/project-metrics.test.js | 46 +++++++++++++++++-- 8 files changed, 132 insertions(+), 23 deletions(-) create mode 100644 changelog.d/1884-p255-task-done-median.fixed.md 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) +})