diff --git a/changelog.d/2650.fixed.md b/changelog.d/2650.fixed.md new file mode 100644 index 00000000..f6f5472a --- /dev/null +++ b/changelog.d/2650.fixed.md @@ -0,0 +1 @@ +The Project board reads every page of its GitHub pull-request and issue lists instead of the first hundred rows, so its seven-day merge and close medians and its day counts now cover the whole window rather than roughly the newest three days ([#2650](https://github.com/ThinkFlowLab/FlyBot/issues/2650)). diff --git a/core/src/host/project-metrics.js b/core/src/host/project-metrics.js index 744532fc..5926af4f 100644 --- a/core/src/host/project-metrics.js +++ b/core/src/host/project-metrics.js @@ -12,6 +12,25 @@ export const METRIC_KEYS = [ ] const DAY_MS = 24 * 60 * 60 * 1000 const RECENT_MS = 7 * DAY_MS +// #2650: the lists are followed page by page, and a `next` that never ends must not hang the board. +// Reaching the cap keeps the rows read so far and answers `truncated`, never a window claimed whole. +const MAX_PAGES = 30 +const API_ORIGIN = 'https://api.github.com' +const apiUrl = path => (path.startsWith('/') ? `${API_ORIGIN}${path}` : path) + +// The next page lives in the `Link` header, not in the body, so the reader has to see the response +// and not only `response.json()`. A foreign host is never followed with the token. +const nextPage = response => { + const link = typeof response.headers?.get === 'function' ? text(response.headers.get('link')) : '' + for (const part of link.split(',')) { + const match = part.match(/<([^>]+)>\s*;\s*rel="?next"?/i) + if (!match) continue + const href = match[1].trim() + if (href.startsWith(`${API_ORIGIN}/`)) return href + if (href.startsWith('/')) return `${API_ORIGIN}${href}` + } + return null +} const text = value => (typeof value === 'string' ? value.trim() : '') const timeOf = value => { const ms = Date.parse(text(value)) @@ -179,17 +198,38 @@ export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', no const since = new Date(now - RECENT_MS).toISOString() const headers = { Authorization: `Bearer ${bearer}`, Accept: 'application/vnd.github+json', 'User-Agent': 'flybot-project-metrics' } const get = async (path) => { - const response = await fetchImpl(`https://api.github.com${path}`, { headers }) + const response = await fetchImpl(apiUrl(path), { headers }) if (!response.ok) throw new Error(`${path} answered ${response.status}`) - return response.json() + return { rows: await response.json(), next: nextPage(response) } + } + // #2650: both lists are longer than one page in a seven-day window, and `/pulls` does not accept + // `since` at all, so a one-page read was the newest 100 rows rather than the window. Follow + // `Link: rel="next"` until it ends; MAX_PAGES bounds a repository that keeps advertising one. + const readAll = async (path) => { + const rows = [] + let next = path + let pages = 0 + while (next !== null && pages < MAX_PAGES) { + const page = await get(next) + if (!Array.isArray(page.rows)) throw new Error(`${next} answered a non-list`) + rows.push(...page.rows) + next = page.next + pages += 1 + } + return { rows, truncated: next !== null } } let source try { const [pullRequests, issues] = await Promise.all([ - get(`/repos/${repo}/pulls?state=all&sort=updated&direction=desc&per_page=100&since=${since}`), - get(`/repos/${repo}/issues?state=all&sort=updated&direction=desc&per_page=100&since=${since}`), + readAll(`/repos/${repo}/pulls?state=all&sort=updated&direction=desc&per_page=100`), + readAll(`/repos/${repo}/issues?state=all&sort=updated&direction=desc&per_page=100&since=${since}`), ]) - source = { ok: true, pullRequests, issues: issues.filter(item => item.pull_request === undefined) } + source = { + ok: true, + pullRequests: pullRequests.rows, + issues: issues.rows.filter(item => item.pull_request === undefined), + truncated: pullRequests.truncated || issues.truncated, + } } catch (error) { return { ...summarizeMetrics({ now, timeZone: zone, tasks, source: { ok: false, reason: `read-failed: ${error.message}` } }), cache: 'miss' } } @@ -204,5 +244,5 @@ export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', no metrics: { prsOpenedToday, prsMergedToday, issuesOpenedToday, issuesClosedToday, prMergeMedianMs, issueCloseMedianMs }, } cacheStore.set(key, { expiresAt: now + ttlMs, value: repositoryValue }) - return { ...withLive(value), cache: 'miss' } + return { ...withLive(value), cache: 'miss', ...(source.truncated ? { truncated: true } : {}) } } diff --git a/core/test/project-metrics.test.js b/core/test/project-metrics.test.js index 6c79c42b..d7f0fde2 100644 --- a/core/test/project-metrics.test.js +++ b/core/test/project-metrics.test.js @@ -222,3 +222,65 @@ test('only cancelled and failed cards is null, never zero', () => { const out = summarizeMetrics({ now: NOW, timeZone: 'UTC', source: { ok: true, pullRequests: [], issues: [] }, tasks }) assert.equal(out.metrics.taskDoneMedianMs, null) }) + +// #2650: seven days is longer than one page. The first page is full and carries the `Link` header; +// the rows that decide the counts and the medians sit on page two, where a one-page read never looks. +const pageRows = (count, make) => Array.from({ length: count }, (_, index) => make(index)) +const pageLink = (pathname, page) => new Map([['link', `; rel="next"`]]) + +test('#2650: the metric lists are read page by page, so a deciding row on page two counts', async () => { + const pageOnePulls = pageRows(100, index => ({ number: index + 1, created_at: '2026-10-29T11:00:00Z', updated_at: '2026-10-30T11:59:00Z', merged_at: null })) + const pageTwoPulls = pageRows(5, index => ({ number: 101 + index, created_at: '2026-10-28T06:00:00Z', updated_at: '2026-10-30T06:00:00Z', merged_at: '2026-10-30T06:00:00Z' })) + const pageOneIssues = pageRows(100, index => ({ number: index + 1, created_at: '2026-10-29T11:00:00Z', updated_at: '2026-10-30T11:59:00Z', closed_at: null })) + const pageTwoIssues = [{ number: 101, created_at: '2026-10-29T06:00:00Z', updated_at: '2026-10-30T10:00:00Z', closed_at: '2026-10-30T10:00:00Z' }] + const seen = [] + const fetchImpl = async (url) => { + const at = new URL(url) + seen.push(at) + const page = Number(at.searchParams.get('page') ?? '1') + const pulls = at.pathname.endsWith('/pulls') + const rows = pulls + ? (page === 1 ? pageOnePulls : page === 2 ? pageTwoPulls : []) + : (page === 1 ? pageOneIssues : page === 2 ? pageTwoIssues : []) + return { ok: true, headers: page === 1 ? pageLink(at.pathname, 2) : new Map(), json: async () => rows } + } + const out = await readMetrics({ repo: 'o/r', token: 'tok', now: NOW, timeZone: 'UTC', fetchImpl, cacheStore: new Map() }) + assert.equal(out.ok, true) + assert.equal(seen.length, 4, 'both lists were followed to their second page') + assert.equal(seen.filter(at => at.pathname.endsWith('/pulls')).every(at => at.searchParams.has('since') === false), true, + '`/pulls` does not accept `since`, so the request no longer pretends to filter by it') + assert.equal(out.metrics.prsMergedToday, 5) + assert.equal(out.metrics.prMergeMedianMs, 48 * HOUR) + assert.equal(out.metrics.issuesClosedToday, 1) + assert.equal(out.metrics.issueCloseMedianMs, 28 * HOUR) + assert.equal(out.metrics.prsOpenedToday, 0) + assert.equal(out.truncated, undefined) +}) + +test('#2650: a `next` that never ends stops at the page cap and answers truncated', async () => { + let calls = 0 + const fetchImpl = async () => { + calls += 1 + return { ok: true, headers: pageLink('/next', calls + 1), json: async () => [pr('2026-10-30T06:00:00Z', null)] } + } + const out = await readMetrics({ repo: 'o/r', token: 'tok', now: NOW, timeZone: 'UTC', fetchImpl, cacheStore: new Map() }) + assert.equal(out.ok, true) + assert.equal(out.truncated, true, 'the answer does not claim the whole window was read') + assert.equal(calls, 60, 'thirty pages for each of the two lists') + assert.equal(out.metrics.prsOpenedToday, 30, 'the rows read before the cap are kept') +}) + +test('#2650: a failed page is a failed read, never a silent partial list', async () => { + const store = new Map() + let calls = 0 + const fetchImpl = async () => { + calls += 1 + if (calls <= 2) return { ok: true, headers: pageLink('/next', 2), json: async () => [] } + return { ok: false, status: 502, json: async () => ({}) } + } + const out = await readMetrics({ repo: 'o/r', token: 'tok', now: NOW, timeZone: 'UTC', fetchImpl, cacheStore: store }) + assert.equal(out.ok, false) + assert.match(out.reason, /^read-failed: .*answered 502$/) + assert.equal(out.metrics.prsOpenedToday, null) + assert.equal(store.size, 0, 'a failed page is not cached as a complete window') +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index ad165476..e3b18f02 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -840,7 +840,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` 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). +`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). Since #2650 both GitHub lists are read page by page through the response's `Link: rel="next"` header instead of the first 100 rows (and `/pulls` no longer sends a `since` the endpoint ignores), with a 30-page cap that keeps the rows read and answers `truncated: true` rather than claiming a complete window. ## 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 744532fc..5926af4f 100644 --- a/patches/ds-bot/files/src/host/project-metrics.js +++ b/patches/ds-bot/files/src/host/project-metrics.js @@ -12,6 +12,25 @@ export const METRIC_KEYS = [ ] const DAY_MS = 24 * 60 * 60 * 1000 const RECENT_MS = 7 * DAY_MS +// #2650: the lists are followed page by page, and a `next` that never ends must not hang the board. +// Reaching the cap keeps the rows read so far and answers `truncated`, never a window claimed whole. +const MAX_PAGES = 30 +const API_ORIGIN = 'https://api.github.com' +const apiUrl = path => (path.startsWith('/') ? `${API_ORIGIN}${path}` : path) + +// The next page lives in the `Link` header, not in the body, so the reader has to see the response +// and not only `response.json()`. A foreign host is never followed with the token. +const nextPage = response => { + const link = typeof response.headers?.get === 'function' ? text(response.headers.get('link')) : '' + for (const part of link.split(',')) { + const match = part.match(/<([^>]+)>\s*;\s*rel="?next"?/i) + if (!match) continue + const href = match[1].trim() + if (href.startsWith(`${API_ORIGIN}/`)) return href + if (href.startsWith('/')) return `${API_ORIGIN}${href}` + } + return null +} const text = value => (typeof value === 'string' ? value.trim() : '') const timeOf = value => { const ms = Date.parse(text(value)) @@ -179,17 +198,38 @@ export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', no const since = new Date(now - RECENT_MS).toISOString() const headers = { Authorization: `Bearer ${bearer}`, Accept: 'application/vnd.github+json', 'User-Agent': 'flybot-project-metrics' } const get = async (path) => { - const response = await fetchImpl(`https://api.github.com${path}`, { headers }) + const response = await fetchImpl(apiUrl(path), { headers }) if (!response.ok) throw new Error(`${path} answered ${response.status}`) - return response.json() + return { rows: await response.json(), next: nextPage(response) } + } + // #2650: both lists are longer than one page in a seven-day window, and `/pulls` does not accept + // `since` at all, so a one-page read was the newest 100 rows rather than the window. Follow + // `Link: rel="next"` until it ends; MAX_PAGES bounds a repository that keeps advertising one. + const readAll = async (path) => { + const rows = [] + let next = path + let pages = 0 + while (next !== null && pages < MAX_PAGES) { + const page = await get(next) + if (!Array.isArray(page.rows)) throw new Error(`${next} answered a non-list`) + rows.push(...page.rows) + next = page.next + pages += 1 + } + return { rows, truncated: next !== null } } let source try { const [pullRequests, issues] = await Promise.all([ - get(`/repos/${repo}/pulls?state=all&sort=updated&direction=desc&per_page=100&since=${since}`), - get(`/repos/${repo}/issues?state=all&sort=updated&direction=desc&per_page=100&since=${since}`), + readAll(`/repos/${repo}/pulls?state=all&sort=updated&direction=desc&per_page=100`), + readAll(`/repos/${repo}/issues?state=all&sort=updated&direction=desc&per_page=100&since=${since}`), ]) - source = { ok: true, pullRequests, issues: issues.filter(item => item.pull_request === undefined) } + source = { + ok: true, + pullRequests: pullRequests.rows, + issues: issues.rows.filter(item => item.pull_request === undefined), + truncated: pullRequests.truncated || issues.truncated, + } } catch (error) { return { ...summarizeMetrics({ now, timeZone: zone, tasks, source: { ok: false, reason: `read-failed: ${error.message}` } }), cache: 'miss' } } @@ -204,5 +244,5 @@ export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', no metrics: { prsOpenedToday, prsMergedToday, issuesOpenedToday, issuesClosedToday, prMergeMedianMs, issueCloseMedianMs }, } cacheStore.set(key, { expiresAt: now + ttlMs, value: repositoryValue }) - return { ...withLive(value), cache: 'miss' } + return { ...withLive(value), cache: 'miss', ...(source.truncated ? { truncated: true } : {}) } } diff --git a/patches/ds-bot/files/test/project-metrics.test.js b/patches/ds-bot/files/test/project-metrics.test.js index 6c79c42b..d7f0fde2 100644 --- a/patches/ds-bot/files/test/project-metrics.test.js +++ b/patches/ds-bot/files/test/project-metrics.test.js @@ -222,3 +222,65 @@ test('only cancelled and failed cards is null, never zero', () => { const out = summarizeMetrics({ now: NOW, timeZone: 'UTC', source: { ok: true, pullRequests: [], issues: [] }, tasks }) assert.equal(out.metrics.taskDoneMedianMs, null) }) + +// #2650: seven days is longer than one page. The first page is full and carries the `Link` header; +// the rows that decide the counts and the medians sit on page two, where a one-page read never looks. +const pageRows = (count, make) => Array.from({ length: count }, (_, index) => make(index)) +const pageLink = (pathname, page) => new Map([['link', `; rel="next"`]]) + +test('#2650: the metric lists are read page by page, so a deciding row on page two counts', async () => { + const pageOnePulls = pageRows(100, index => ({ number: index + 1, created_at: '2026-10-29T11:00:00Z', updated_at: '2026-10-30T11:59:00Z', merged_at: null })) + const pageTwoPulls = pageRows(5, index => ({ number: 101 + index, created_at: '2026-10-28T06:00:00Z', updated_at: '2026-10-30T06:00:00Z', merged_at: '2026-10-30T06:00:00Z' })) + const pageOneIssues = pageRows(100, index => ({ number: index + 1, created_at: '2026-10-29T11:00:00Z', updated_at: '2026-10-30T11:59:00Z', closed_at: null })) + const pageTwoIssues = [{ number: 101, created_at: '2026-10-29T06:00:00Z', updated_at: '2026-10-30T10:00:00Z', closed_at: '2026-10-30T10:00:00Z' }] + const seen = [] + const fetchImpl = async (url) => { + const at = new URL(url) + seen.push(at) + const page = Number(at.searchParams.get('page') ?? '1') + const pulls = at.pathname.endsWith('/pulls') + const rows = pulls + ? (page === 1 ? pageOnePulls : page === 2 ? pageTwoPulls : []) + : (page === 1 ? pageOneIssues : page === 2 ? pageTwoIssues : []) + return { ok: true, headers: page === 1 ? pageLink(at.pathname, 2) : new Map(), json: async () => rows } + } + const out = await readMetrics({ repo: 'o/r', token: 'tok', now: NOW, timeZone: 'UTC', fetchImpl, cacheStore: new Map() }) + assert.equal(out.ok, true) + assert.equal(seen.length, 4, 'both lists were followed to their second page') + assert.equal(seen.filter(at => at.pathname.endsWith('/pulls')).every(at => at.searchParams.has('since') === false), true, + '`/pulls` does not accept `since`, so the request no longer pretends to filter by it') + assert.equal(out.metrics.prsMergedToday, 5) + assert.equal(out.metrics.prMergeMedianMs, 48 * HOUR) + assert.equal(out.metrics.issuesClosedToday, 1) + assert.equal(out.metrics.issueCloseMedianMs, 28 * HOUR) + assert.equal(out.metrics.prsOpenedToday, 0) + assert.equal(out.truncated, undefined) +}) + +test('#2650: a `next` that never ends stops at the page cap and answers truncated', async () => { + let calls = 0 + const fetchImpl = async () => { + calls += 1 + return { ok: true, headers: pageLink('/next', calls + 1), json: async () => [pr('2026-10-30T06:00:00Z', null)] } + } + const out = await readMetrics({ repo: 'o/r', token: 'tok', now: NOW, timeZone: 'UTC', fetchImpl, cacheStore: new Map() }) + assert.equal(out.ok, true) + assert.equal(out.truncated, true, 'the answer does not claim the whole window was read') + assert.equal(calls, 60, 'thirty pages for each of the two lists') + assert.equal(out.metrics.prsOpenedToday, 30, 'the rows read before the cap are kept') +}) + +test('#2650: a failed page is a failed read, never a silent partial list', async () => { + const store = new Map() + let calls = 0 + const fetchImpl = async () => { + calls += 1 + if (calls <= 2) return { ok: true, headers: pageLink('/next', 2), json: async () => [] } + return { ok: false, status: 502, json: async () => ({}) } + } + const out = await readMetrics({ repo: 'o/r', token: 'tok', now: NOW, timeZone: 'UTC', fetchImpl, cacheStore: store }) + assert.equal(out.ok, false) + assert.match(out.reason, /^read-failed: .*answered 502$/) + assert.equal(out.metrics.prsOpenedToday, null) + assert.equal(store.size, 0, 'a failed page is not cached as a complete window') +})