From 091fc8c5fadf7b2bec60886d6344acd0a2734cf6 Mon Sep 17 00:00:00 2001 From: Dev-4 Date: Sat, 31 Oct 2026 01:14:13 +0000 Subject: [PATCH 1/2] Scope the Project board's running table to the board itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The board handed readMetrics the deployment's whole task store, so the running table named another Project's claimed cards — title and holder — and emptied when the GitHub source was unavailable or failed. Pass the already filtered `mine` list instead, and carry that one list through every branch of the metrics read; the done-task median reads the same list. The four GitHub numbers stay repository-wide. --- changelog.d/1848-running-table-scope.fixed.md | 1 + core/src/host/project-board.js | 4 +- core/src/host/project-metrics.js | 24 ++- core/test/project-metrics-scope.test.js | 172 ++++++++++++++++++ docs/subsystems/core-host.md | 2 +- .../ds-bot/files/src/host/project-board.js | 4 +- .../ds-bot/files/src/host/project-metrics.js | 24 ++- .../files/test/project-metrics-scope.test.js | 172 ++++++++++++++++++ 8 files changed, 384 insertions(+), 19 deletions(-) create mode 100644 changelog.d/1848-running-table-scope.fixed.md create mode 100644 core/test/project-metrics-scope.test.js create mode 100644 patches/ds-bot/files/test/project-metrics-scope.test.js diff --git a/changelog.d/1848-running-table-scope.fixed.md b/changelog.d/1848-running-table-scope.fixed.md new file mode 100644 index 000000000..36cb7da7b --- /dev/null +++ b/changelog.d/1848-running-table-scope.fixed.md @@ -0,0 +1 @@ +The Project board's running table now reads the cards of the board it belongs to instead of the whole deployment's task store, so another Project's claimed card no longer hands out its title and holder; the same list is carried by every branch of the metrics read, so the table stays populated when the GitHub source is unavailable or fails, and the done-task median is this board's cards too ([#1848](https://github.com/ThinkFlowLab/FlyBot/issues/1848)). diff --git a/core/src/host/project-board.js b/core/src/host/project-board.js index 4074f5c9f..53251fd4a 100644 --- a/core/src/host/project-board.js +++ b/core/src/host/project-board.js @@ -461,13 +461,15 @@ export function install(rt) { const decisions = await decisionsOf(project) // #1829: the seven numbers ride on the same read as the rows, so the board and its header // cannot disagree, and a credential-less host answers `ok: false` (the client paints a dash). + // #1848: the running table and the done-task median read `mine` — exactly the cards this + // reader was allowed above — never the deployment's task table. const metrics = await readMetrics({ repo: text(rt.githubRepo) || 'ThinkFlowLab/FlyBot', token: text(rt.secretValue?.('GITHUB_TOKEN')), fetchImpl: globalThis.fetch, timeZone: text(options.timeZone) || 'UTC', now: typeof rt.now === 'function' ? rt.now() : Date.now(), - tasks, + tasks: mine, }) return { rows: open, done, ...queues, decisions, meetings: meetingsOf(project, decisions), metrics } } diff --git a/core/src/host/project-metrics.js b/core/src/host/project-metrics.js index c2c200f14..ef60b22c6 100644 --- a/core/src/host/project-metrics.js +++ b/core/src/host/project-metrics.js @@ -47,10 +47,15 @@ const durationInWindow = (items, from, to, now) => items.reduce((values, item) = return values }, []) +// The task rows this read is about (#1848): one board's cards that its reader may see, not the +// deployment's. The caller passes the same list its rows were built from; an object is still read +// 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. -const doneTaskDurations = (tasks, now) => Object.values(tasks ?? {}).reduce((values, task) => { +const doneTaskDurations = (tasks, now) => taskList(tasks).reduce((values, task) => { if (task === null || typeof task !== 'object' || task.status !== 'done') return values const start = Number.isFinite(task.createdAt) ? task.createdAt : null const end = Number.isFinite(task.completedAt) ? task.completedAt : null @@ -91,7 +96,10 @@ const emptyMetrics = () => Object.fromEntries(METRIC_KEYS.map(key => [key, null] // `source` is `{ ok, pullRequests, issues, reason }`; a failed half is answered as unavailable for // the metrics it feeds and never as zero. export function summarizeMetrics({ now = Date.now(), timeZone = 'UTC', source, tasks } = {}) { - const unavailable = (reason) => ({ ok: false, reason, timeZone, generatedAt: now, metrics: emptyMetrics(), running: [] }) + // #1848: the running rows are the board's own visible cards in every branch, so a source that is + // missing or failed no longer empties a table that needs no token to read. + const running = () => taskList(tasks).map(task => runningOf(task, now)).filter(row => row !== null) + const unavailable = (reason) => ({ ok: false, reason, timeZone, generatedAt: now, metrics: emptyMetrics(), running: running() }) if (source === undefined || source === null) return unavailable('no-source') if (source.ok !== true) return unavailable(text(source.reason) || 'source-unavailable') const pullRequests = Array.isArray(source.pullRequests) ? source.pullRequests : [] @@ -111,21 +119,21 @@ export function summarizeMetrics({ now = Date.now(), timeZone = 'UTC', source, t issueCloseMedianMs: median(durationInWindow(issues, item => item.created_at, item => item.closed_at, now)), taskDoneMedianMs: median(doneTaskDurations(tasks, now)), }, - running: Object.values(tasks ?? {}).map(task => runningOf(task, now)).filter(row => row !== null), + running: running(), } } // The read the endpoint calls. `fetchImpl` and `token` are seams: no token answers unavailable // without a request, and one failed request answers unavailable for this tick rather than a cache -// entry that would keep the failure for five minutes. +// entry that would keep the failure for five minutes. `tasks` is the board's visible rows (#1848). export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', now = Date.now(), tasks, ttlMs = METRICS_TTL_MS, cacheStore = cache } = {}) { // The cache carries the seven numbers only: a card claimed a moment ago must appear in the running // table on the next read, so those rows are projected from the live tasks every time (#1829). - const liveRunning = () => Object.values(tasks ?? {}).map(task => runningOf(task, now)).filter(row => row !== null) + const liveRunning = () => taskList(tasks).map(task => runningOf(task, now)).filter(row => row !== null) const withRunning = value => ({ ...value, running: liveRunning() }) const bearer = text(token) - if (bearer === '') return { ...summarizeMetrics({ now, timeZone, source: { ok: false, reason: 'no-credentials' } }), cache: 'miss' } - if (typeof fetchImpl !== 'function') return { ...summarizeMetrics({ now, timeZone, source: { ok: false, reason: 'no-fetch' } }), cache: 'miss' } + if (bearer === '') return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: 'no-credentials' } }), cache: 'miss' } + if (typeof fetchImpl !== 'function') return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: 'no-fetch' } }), cache: 'miss' } const key = `${repo}|${timeZone}|${Math.floor(now / ttlMs)}` const hit = cacheStore.get(key) if (hit !== undefined && hit.expiresAt > now) return { ...withRunning(hit.value), cache: 'hit' } @@ -144,7 +152,7 @@ export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', no ]) source = { ok: true, pullRequests, issues: issues.filter(item => item.pull_request === undefined) } } catch (error) { - return { ...summarizeMetrics({ now, timeZone, source: { ok: false, reason: `read-failed: ${error.message}` } }), cache: 'miss' } + return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: `read-failed: ${error.message}` } }), cache: 'miss' } } const value = summarizeMetrics({ now, timeZone, source, tasks }) const { running: _live, ...cached } = value diff --git a/core/test/project-metrics-scope.test.js b/core/test/project-metrics-scope.test.js new file mode 100644 index 000000000..a225e7acc --- /dev/null +++ b/core/test/project-metrics-scope.test.js @@ -0,0 +1,172 @@ +// #1848: the board's running table is the board's own cards, not the deployment's. Every branch of +// the metrics read carries the same list the rows were built from, and a card the same actor is +// refused on a row never appears in `running` — the host must not send the titles at all. +import assert from 'node:assert/strict' +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { createRuntime, provideFlyBot } from '../index.js' +import { fakeContext } from './fake-ctx.js' + +const HOUR = 60 * 60 * 1000 +const NOW = Date.parse('2026-10-30T12:00:00Z') + +// Two Projects with two owners, and a card in flight on each one. The second Project's card is the +// leak: it is `claimed`, carries a title and a holder, and the first owner is refused on it. +const seed = () => ({ + version: 2, + revision: 1, + mainBotIds: ['a'], + members: { + 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'a' }, + 'owner-2': { id: 'owner-2', createdAt: 2, designatedChiefId: 'foreign' }, + }, + bots: { + a: { id: 'a', name: 'Chief', role: 'Boss', sessionId: 'a', createdAt: 1, ownerId: 'owner-1' }, + foreign: { id: 'foreign', name: 'Foreigner Bot', role: 'Bot', sessionId: 'foreign', createdAt: 2, ownerId: 'owner-2' }, + }, + rooms: { + r1: { id: 'r1', name: 'Ops', members: ['a'], mode: 'admin', admin: 'a', named: true, sessions: {}, log: [], seq: 0, seen: {}, projectId: 'project-p1' }, + r2: { id: 'r2', name: 'Garden', members: ['foreign'], mode: 'admin', admin: 'foreign', named: true, sessions: {}, log: [], seq: 0, seen: {}, projectId: 'project-p2' }, + }, + agents: {}, exchanges: [], exchangeTurns: {}, questions: {}, prefs: {}, grants: {}, + projects: { + 'project-p1': { id: 'project-p1', name: 'P1', repository: 'ThinkFlowLab/FlyBot', defaultBranch: 'main', ownerUserId: 'owner-1', createdAt: 1, updatedAt: 1, status: 'active' }, + 'project-p2': { id: 'project-p2', name: 'P2', repository: 'ThinkFlowLab/FlyBot', defaultBranch: 'main', ownerUserId: 'owner-2', createdAt: 1, updatedAt: 1, status: 'active' }, + }, + projectRuns: {}, + 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 }, + // 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 }, + }, +}) + +async function start(home = mkdtempSync(join(tmpdir(), 'ds-bot-scope-')), team = seed()) { + if (!existsSync(join(home, 'state.json'))) writeFileSync(join(home, 'state.json'), JSON.stringify(team)) + const ctx = fakeContext() + const agents = new Map() + const agentOf = (sessionId) => { + if (!agents.has(sessionId)) agents.set(sessionId, { session: { id: sessionId }, followup() {}, steer() {}, inject() {}, cancel() {}, whenIdle() {}, inbox: { nextStep: [], nextTurn: [], remove() {} } }) + return agents.get(sessionId) + } + Object.assign(ctx, { + llm: { listProviders: () => [], listModels: async () => [], resolveModelInfo: async () => ({}) }, + workspaceRegistry: { get: () => undefined, resolveByPath: async () => undefined, create: async path => ({ id: 'w1', path }), archiveSession: async () => {} }, + sessionController: { create: async () => ({ sessionId: 's1' }), resolveAgent: async id => ({ agent: agentOf(id) }), selectModel: async () => {} }, + sessionTitle: { rename: async () => {} }, + sessions: { flush: async () => {} }, + }) + const rt = createRuntime(ctx, { home }) + provideFlyBot(ctx, rt.flybot) + await rt.load() + // Somebody has signed in: the row guard of #1723 is on, and the running table must obey it too. + rt.state.authSessions = { 's-1': { memberId: 'owner-1', tokenHash: 'h1' }, 's-2': { memberId: 'owner-2', tokenHash: 'h2' } } + rt.now = () => NOW + const stop = async ({ keepHome = false } = {}) => { + await ctx.dispose() + if (!keepHome) rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + } + return { rt, stop } +} + +test('#1848: another Project\'s claimed card appears in neither the rows nor the running table', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-1' + // A source that answers, so both boards get to their success branch. + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + // Reading the other Project's board as a member who cannot see its cards. + const foreign = await rt.projectBoard('project-p2', { memberId: 'owner-1' }) + assert.deepEqual(foreign.rows, [], 'a card of the other Project is not a row here') + assert.deepEqual(foreign.metrics.running, [], 'and it is not a running row here either') + // This Project's own board: the one card in flight is one row, and the other Project's card is + // not beside it. + const board = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + const rows = board.rows.map(row => row.id) + assert.equal(rows.includes('task-group'), true) + assert.equal(rows.some(id => id.startsWith('task-foreign')), false, 'no card of the other Project is a row here') + assert.deepEqual(board.metrics.running.map(row => row.id), ['task-group']) + assert.equal(board.metrics.running[0].title, 'Ship it') + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +test('#1848: the board\'s own card stays in the running table when the GitHub source is unavailable', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-2' + try { + // No credential: the four numbers answer `ok: false`, but the running table is local data and + // must keep the card it can see. + rt.secretValue = () => undefined + const noToken = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(noToken.metrics.ok, false) + assert.equal(noToken.metrics.reason, 'no-credentials') + assert.deepEqual(noToken.metrics.running.map(row => row.id), ['task-group']) + // A source that fails answers the same way for the same reason. + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => { throw new Error('socket hang up') } + const failed = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(failed.metrics.ok, false) + assert.match(failed.metrics.reason, /^read-failed:/) + assert.deepEqual(failed.metrics.running.map(row => row.id), ['task-group']) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +test('#1848: a cache hit and a cache miss carry the same running rows, byte for byte', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-3' + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + const miss = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(miss.metrics.cache, 'miss') + // Same instant, so the second read is the cache hit and its numbers come from the cache. + const hit = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(hit.metrics.cache, 'hit') + assert.deepEqual(hit.metrics.running, miss.metrics.running) + assert.deepEqual(miss.metrics.running.map(row => row.id), ['task-group']) + assert.equal(JSON.stringify(hit.metrics.running), JSON.stringify(miss.metrics.running)) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +test('#1848: the done-task median is this board\'s cards, not the deployment\'s', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-4' + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + const board = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + // One card finished an hour after it started here; the other Project's nine-hour card is not + // this board's work. + assert.equal(board.metrics.metrics.taskDoneMedianMs, HOUR) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index adf49abc9..8dda7fcd8 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -786,7 +786,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, and the local task half reads `createdAt` / `completedAt` from `state.tasks`. 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, 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) carries that same list. 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-board.js b/patches/ds-bot/files/src/host/project-board.js index 4074f5c9f..53251fd4a 100644 --- a/patches/ds-bot/files/src/host/project-board.js +++ b/patches/ds-bot/files/src/host/project-board.js @@ -461,13 +461,15 @@ export function install(rt) { const decisions = await decisionsOf(project) // #1829: the seven numbers ride on the same read as the rows, so the board and its header // cannot disagree, and a credential-less host answers `ok: false` (the client paints a dash). + // #1848: the running table and the done-task median read `mine` — exactly the cards this + // reader was allowed above — never the deployment's task table. const metrics = await readMetrics({ repo: text(rt.githubRepo) || 'ThinkFlowLab/FlyBot', token: text(rt.secretValue?.('GITHUB_TOKEN')), fetchImpl: globalThis.fetch, timeZone: text(options.timeZone) || 'UTC', now: typeof rt.now === 'function' ? rt.now() : Date.now(), - tasks, + tasks: mine, }) return { rows: open, done, ...queues, decisions, meetings: meetingsOf(project, decisions), metrics } } diff --git a/patches/ds-bot/files/src/host/project-metrics.js b/patches/ds-bot/files/src/host/project-metrics.js index c2c200f14..ef60b22c6 100644 --- a/patches/ds-bot/files/src/host/project-metrics.js +++ b/patches/ds-bot/files/src/host/project-metrics.js @@ -47,10 +47,15 @@ const durationInWindow = (items, from, to, now) => items.reduce((values, item) = return values }, []) +// The task rows this read is about (#1848): one board's cards that its reader may see, not the +// deployment's. The caller passes the same list its rows were built from; an object is still read +// 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. -const doneTaskDurations = (tasks, now) => Object.values(tasks ?? {}).reduce((values, task) => { +const doneTaskDurations = (tasks, now) => taskList(tasks).reduce((values, task) => { if (task === null || typeof task !== 'object' || task.status !== 'done') return values const start = Number.isFinite(task.createdAt) ? task.createdAt : null const end = Number.isFinite(task.completedAt) ? task.completedAt : null @@ -91,7 +96,10 @@ const emptyMetrics = () => Object.fromEntries(METRIC_KEYS.map(key => [key, null] // `source` is `{ ok, pullRequests, issues, reason }`; a failed half is answered as unavailable for // the metrics it feeds and never as zero. export function summarizeMetrics({ now = Date.now(), timeZone = 'UTC', source, tasks } = {}) { - const unavailable = (reason) => ({ ok: false, reason, timeZone, generatedAt: now, metrics: emptyMetrics(), running: [] }) + // #1848: the running rows are the board's own visible cards in every branch, so a source that is + // missing or failed no longer empties a table that needs no token to read. + const running = () => taskList(tasks).map(task => runningOf(task, now)).filter(row => row !== null) + const unavailable = (reason) => ({ ok: false, reason, timeZone, generatedAt: now, metrics: emptyMetrics(), running: running() }) if (source === undefined || source === null) return unavailable('no-source') if (source.ok !== true) return unavailable(text(source.reason) || 'source-unavailable') const pullRequests = Array.isArray(source.pullRequests) ? source.pullRequests : [] @@ -111,21 +119,21 @@ export function summarizeMetrics({ now = Date.now(), timeZone = 'UTC', source, t issueCloseMedianMs: median(durationInWindow(issues, item => item.created_at, item => item.closed_at, now)), taskDoneMedianMs: median(doneTaskDurations(tasks, now)), }, - running: Object.values(tasks ?? {}).map(task => runningOf(task, now)).filter(row => row !== null), + running: running(), } } // The read the endpoint calls. `fetchImpl` and `token` are seams: no token answers unavailable // without a request, and one failed request answers unavailable for this tick rather than a cache -// entry that would keep the failure for five minutes. +// entry that would keep the failure for five minutes. `tasks` is the board's visible rows (#1848). export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', now = Date.now(), tasks, ttlMs = METRICS_TTL_MS, cacheStore = cache } = {}) { // The cache carries the seven numbers only: a card claimed a moment ago must appear in the running // table on the next read, so those rows are projected from the live tasks every time (#1829). - const liveRunning = () => Object.values(tasks ?? {}).map(task => runningOf(task, now)).filter(row => row !== null) + const liveRunning = () => taskList(tasks).map(task => runningOf(task, now)).filter(row => row !== null) const withRunning = value => ({ ...value, running: liveRunning() }) const bearer = text(token) - if (bearer === '') return { ...summarizeMetrics({ now, timeZone, source: { ok: false, reason: 'no-credentials' } }), cache: 'miss' } - if (typeof fetchImpl !== 'function') return { ...summarizeMetrics({ now, timeZone, source: { ok: false, reason: 'no-fetch' } }), cache: 'miss' } + if (bearer === '') return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: 'no-credentials' } }), cache: 'miss' } + if (typeof fetchImpl !== 'function') return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: 'no-fetch' } }), cache: 'miss' } const key = `${repo}|${timeZone}|${Math.floor(now / ttlMs)}` const hit = cacheStore.get(key) if (hit !== undefined && hit.expiresAt > now) return { ...withRunning(hit.value), cache: 'hit' } @@ -144,7 +152,7 @@ export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', no ]) source = { ok: true, pullRequests, issues: issues.filter(item => item.pull_request === undefined) } } catch (error) { - return { ...summarizeMetrics({ now, timeZone, source: { ok: false, reason: `read-failed: ${error.message}` } }), cache: 'miss' } + return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: `read-failed: ${error.message}` } }), cache: 'miss' } } const value = summarizeMetrics({ now, timeZone, source, tasks }) const { running: _live, ...cached } = value diff --git a/patches/ds-bot/files/test/project-metrics-scope.test.js b/patches/ds-bot/files/test/project-metrics-scope.test.js new file mode 100644 index 000000000..a225e7acc --- /dev/null +++ b/patches/ds-bot/files/test/project-metrics-scope.test.js @@ -0,0 +1,172 @@ +// #1848: the board's running table is the board's own cards, not the deployment's. Every branch of +// the metrics read carries the same list the rows were built from, and a card the same actor is +// refused on a row never appears in `running` — the host must not send the titles at all. +import assert from 'node:assert/strict' +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { createRuntime, provideFlyBot } from '../index.js' +import { fakeContext } from './fake-ctx.js' + +const HOUR = 60 * 60 * 1000 +const NOW = Date.parse('2026-10-30T12:00:00Z') + +// Two Projects with two owners, and a card in flight on each one. The second Project's card is the +// leak: it is `claimed`, carries a title and a holder, and the first owner is refused on it. +const seed = () => ({ + version: 2, + revision: 1, + mainBotIds: ['a'], + members: { + 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'a' }, + 'owner-2': { id: 'owner-2', createdAt: 2, designatedChiefId: 'foreign' }, + }, + bots: { + a: { id: 'a', name: 'Chief', role: 'Boss', sessionId: 'a', createdAt: 1, ownerId: 'owner-1' }, + foreign: { id: 'foreign', name: 'Foreigner Bot', role: 'Bot', sessionId: 'foreign', createdAt: 2, ownerId: 'owner-2' }, + }, + rooms: { + r1: { id: 'r1', name: 'Ops', members: ['a'], mode: 'admin', admin: 'a', named: true, sessions: {}, log: [], seq: 0, seen: {}, projectId: 'project-p1' }, + r2: { id: 'r2', name: 'Garden', members: ['foreign'], mode: 'admin', admin: 'foreign', named: true, sessions: {}, log: [], seq: 0, seen: {}, projectId: 'project-p2' }, + }, + agents: {}, exchanges: [], exchangeTurns: {}, questions: {}, prefs: {}, grants: {}, + projects: { + 'project-p1': { id: 'project-p1', name: 'P1', repository: 'ThinkFlowLab/FlyBot', defaultBranch: 'main', ownerUserId: 'owner-1', createdAt: 1, updatedAt: 1, status: 'active' }, + 'project-p2': { id: 'project-p2', name: 'P2', repository: 'ThinkFlowLab/FlyBot', defaultBranch: 'main', ownerUserId: 'owner-2', createdAt: 1, updatedAt: 1, status: 'active' }, + }, + projectRuns: {}, + 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 }, + // 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 }, + }, +}) + +async function start(home = mkdtempSync(join(tmpdir(), 'ds-bot-scope-')), team = seed()) { + if (!existsSync(join(home, 'state.json'))) writeFileSync(join(home, 'state.json'), JSON.stringify(team)) + const ctx = fakeContext() + const agents = new Map() + const agentOf = (sessionId) => { + if (!agents.has(sessionId)) agents.set(sessionId, { session: { id: sessionId }, followup() {}, steer() {}, inject() {}, cancel() {}, whenIdle() {}, inbox: { nextStep: [], nextTurn: [], remove() {} } }) + return agents.get(sessionId) + } + Object.assign(ctx, { + llm: { listProviders: () => [], listModels: async () => [], resolveModelInfo: async () => ({}) }, + workspaceRegistry: { get: () => undefined, resolveByPath: async () => undefined, create: async path => ({ id: 'w1', path }), archiveSession: async () => {} }, + sessionController: { create: async () => ({ sessionId: 's1' }), resolveAgent: async id => ({ agent: agentOf(id) }), selectModel: async () => {} }, + sessionTitle: { rename: async () => {} }, + sessions: { flush: async () => {} }, + }) + const rt = createRuntime(ctx, { home }) + provideFlyBot(ctx, rt.flybot) + await rt.load() + // Somebody has signed in: the row guard of #1723 is on, and the running table must obey it too. + rt.state.authSessions = { 's-1': { memberId: 'owner-1', tokenHash: 'h1' }, 's-2': { memberId: 'owner-2', tokenHash: 'h2' } } + rt.now = () => NOW + const stop = async ({ keepHome = false } = {}) => { + await ctx.dispose() + if (!keepHome) rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + } + return { rt, stop } +} + +test('#1848: another Project\'s claimed card appears in neither the rows nor the running table', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-1' + // A source that answers, so both boards get to their success branch. + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + // Reading the other Project's board as a member who cannot see its cards. + const foreign = await rt.projectBoard('project-p2', { memberId: 'owner-1' }) + assert.deepEqual(foreign.rows, [], 'a card of the other Project is not a row here') + assert.deepEqual(foreign.metrics.running, [], 'and it is not a running row here either') + // This Project's own board: the one card in flight is one row, and the other Project's card is + // not beside it. + const board = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + const rows = board.rows.map(row => row.id) + assert.equal(rows.includes('task-group'), true) + assert.equal(rows.some(id => id.startsWith('task-foreign')), false, 'no card of the other Project is a row here') + assert.deepEqual(board.metrics.running.map(row => row.id), ['task-group']) + assert.equal(board.metrics.running[0].title, 'Ship it') + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +test('#1848: the board\'s own card stays in the running table when the GitHub source is unavailable', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-2' + try { + // No credential: the four numbers answer `ok: false`, but the running table is local data and + // must keep the card it can see. + rt.secretValue = () => undefined + const noToken = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(noToken.metrics.ok, false) + assert.equal(noToken.metrics.reason, 'no-credentials') + assert.deepEqual(noToken.metrics.running.map(row => row.id), ['task-group']) + // A source that fails answers the same way for the same reason. + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => { throw new Error('socket hang up') } + const failed = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(failed.metrics.ok, false) + assert.match(failed.metrics.reason, /^read-failed:/) + assert.deepEqual(failed.metrics.running.map(row => row.id), ['task-group']) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +test('#1848: a cache hit and a cache miss carry the same running rows, byte for byte', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-3' + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + const miss = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(miss.metrics.cache, 'miss') + // Same instant, so the second read is the cache hit and its numbers come from the cache. + const hit = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(hit.metrics.cache, 'hit') + assert.deepEqual(hit.metrics.running, miss.metrics.running) + assert.deepEqual(miss.metrics.running.map(row => row.id), ['task-group']) + assert.equal(JSON.stringify(hit.metrics.running), JSON.stringify(miss.metrics.running)) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +test('#1848: the done-task median is this board\'s cards, not the deployment\'s', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-4' + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + const board = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + // One card finished an hour after it started here; the other Project's nine-hour card is not + // this board's work. + assert.equal(board.metrics.metrics.taskDoneMedianMs, HOUR) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) From 6b28d06447ba1b7910142cc842c0d6630b385da0 Mon Sep 17 00:00:00 2001 From: Dev-4 Date: Sat, 31 Oct 2026 01:29:35 +0000 Subject: [PATCH 2/2] Re-project the done-task median on every metrics read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cache is keyed by repository, time zone and five-minute bucket, so a hit is shared by every Project on that repo — and it carried the median computed from the first reader's cards. Store only the repository-level numbers, and project `running` and `taskDoneMedianMs` from this caller's tasks on both the hit and the miss; the entry is written field by field so a caller-scoped number cannot ride in by a later spread. The unavailable branches keep their nulls: a source that could not be read is not a zero median. --- changelog.d/1848-running-table-scope.fixed.md | 2 +- core/src/host/project-metrics.js | 24 ++-- core/test/project-metrics-scope.test.js | 106 ++++++++++++++++++ docs/subsystems/core-host.md | 2 +- .../ds-bot/files/src/host/project-metrics.js | 24 ++-- .../files/test/project-metrics-scope.test.js | 106 ++++++++++++++++++ 6 files changed, 248 insertions(+), 16 deletions(-) diff --git a/changelog.d/1848-running-table-scope.fixed.md b/changelog.d/1848-running-table-scope.fixed.md index 36cb7da7b..30a81c199 100644 --- a/changelog.d/1848-running-table-scope.fixed.md +++ b/changelog.d/1848-running-table-scope.fixed.md @@ -1 +1 @@ -The Project board's running table now reads the cards of the board it belongs to instead of the whole deployment's task store, so another Project's claimed card no longer hands out its title and holder; the same list is carried by every branch of the metrics read, so the table stays populated when the GitHub source is unavailable or fails, and the done-task median is this board's cards too ([#1848](https://github.com/ThinkFlowLab/FlyBot/issues/1848)). +The Project board's running table now reads the cards of the board it belongs to instead of the whole deployment's task store, so another Project's claimed card no longer hands out its title and holder; the same list is carried by every branch of the metrics read, so the table stays populated when the GitHub source is unavailable or fails, and the done-task median is this board's cards too ([#1848](https://github.com/ThinkFlowLab/FlyBot/issues/1848)); a five-minute cache entry shared by two Projects on the same repository no longer hands one Project's done-task median to the other ([#1880](https://github.com/ThinkFlowLab/FlyBot/pull/1880)). diff --git a/core/src/host/project-metrics.js b/core/src/host/project-metrics.js index ef60b22c6..03d95342f 100644 --- a/core/src/host/project-metrics.js +++ b/core/src/host/project-metrics.js @@ -127,16 +127,18 @@ export function summarizeMetrics({ now = Date.now(), timeZone = 'UTC', source, t // without a request, and one failed request answers unavailable for this tick rather than a cache // entry that would keep the failure for five minutes. `tasks` is the board's visible rows (#1848). export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', now = Date.now(), tasks, ttlMs = METRICS_TTL_MS, cacheStore = cache } = {}) { - // The cache carries the seven numbers only: a card claimed a moment ago must appear in the running - // table on the next read, so those rows are projected from the live tasks every time (#1829). + // The cache key is the repository and the five-minute bucket, not the reader, so it carries the + // repository-level numbers only. Everything computed from `tasks` — the running rows and the done + // median — is projected from this caller's list on every read, hit or miss (#1829, #1880): a + // shared entry must not answer one board with the numbers of the board that filled it. const liveRunning = () => taskList(tasks).map(task => runningOf(task, now)).filter(row => row !== null) - const withRunning = value => ({ ...value, running: liveRunning() }) + const withLive = value => ({ ...value, metrics: { ...value.metrics, taskDoneMedianMs: median(doneTaskDurations(tasks, now)) }, running: liveRunning() }) const bearer = text(token) if (bearer === '') return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: 'no-credentials' } }), cache: 'miss' } if (typeof fetchImpl !== 'function') return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: 'no-fetch' } }), cache: 'miss' } const key = `${repo}|${timeZone}|${Math.floor(now / ttlMs)}` const hit = cacheStore.get(key) - if (hit !== undefined && hit.expiresAt > now) return { ...withRunning(hit.value), cache: 'hit' } + if (hit !== undefined && hit.expiresAt > now) return { ...withLive(hit.value), cache: 'hit' } 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) => { @@ -155,7 +157,15 @@ export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', no return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: `read-failed: ${error.message}` } }), cache: 'miss' } } const value = summarizeMetrics({ now, timeZone, source, tasks }) - const { running: _live, ...cached } = value - cacheStore.set(key, { expiresAt: now + ttlMs, value: cached }) - return { ...withRunning(value), cache: 'miss' } + // Written field by field rather than by spreading, so a caller-scoped number added later cannot + // ride into a shared entry by accident: this list is the whole shape the cache may hold. + const { prsOpenedToday, prsMergedToday, issuesOpenedToday, issuesClosedToday, prMergeMedianMs, issueCloseMedianMs } = value.metrics + const repositoryValue = { + ok: value.ok, + timeZone: value.timeZone, + generatedAt: value.generatedAt, + metrics: { prsOpenedToday, prsMergedToday, issuesOpenedToday, issuesClosedToday, prMergeMedianMs, issueCloseMedianMs }, + } + cacheStore.set(key, { expiresAt: now + ttlMs, value: repositoryValue }) + return { ...withLive(value), cache: 'miss' } } diff --git a/core/test/project-metrics-scope.test.js b/core/test/project-metrics-scope.test.js index a225e7acc..45599d69f 100644 --- a/core/test/project-metrics-scope.test.js +++ b/core/test/project-metrics-scope.test.js @@ -7,6 +7,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' import { createRuntime, provideFlyBot } from '../index.js' +import { readMetrics } from '../src/host/project-metrics.js' import { fakeContext } from './fake-ctx.js' const HOUR = 60 * 60 * 1000 @@ -170,3 +171,108 @@ test('#1848: the done-task median is this board\'s cards, not the deployment\'s' await stop() } }) + +// #1880: the metrics cache is keyed by repository, time zone and five-minute bucket only, so a hit +// is shared by every reader of that repo. The done-task median is computed from the reader's own +// cards, like the running table, so a hit must re-project it and never answer with the cards of +// whoever filled the entry first. +test('#1880: a cache hit projects the done-task median from the reader\'s own cards', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-5' + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + // The other board fills this bucket: one card, nine hours from start to done. + const other = await rt.projectBoard('project-p2', { memberId: 'owner-2' }) + assert.equal(other.metrics.cache, 'miss') + assert.equal(other.metrics.metrics.taskDoneMedianMs, 9 * HOUR) + // Same repo, same bucket, a different board: the read is a hit, and the median is this board's + // hour — not the nine hours the first reader's cards produced. + const mine = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(mine.metrics.cache, 'hit') + assert.equal(mine.metrics.metrics.taskDoneMedianMs, HOUR) + // A hit belongs to its reader, not to whoever filled the entry: back on the other board the + // nine hours return. + const back = await rt.projectBoard('project-p2', { memberId: 'owner-2' }) + assert.equal(back.metrics.cache, 'hit') + assert.equal(back.metrics.metrics.taskDoneMedianMs, 9 * HOUR) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +test('#1880: a cache hit\'s running rows and done median come from the same board', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-6' + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + const miss = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(miss.metrics.cache, 'miss') + const hit = await rt.projectBoard('project-p2', { memberId: 'owner-2' }) + assert.equal(hit.metrics.cache, 'hit') + // One projection, one source: the rows are this board's card and the median beside it is this + // board's nine hours, never a row from one reader with a number from the other. + assert.deepEqual(hit.metrics.running.map(row => row.id), ['task-foreign']) + assert.equal(hit.metrics.metrics.taskDoneMedianMs, 9 * HOUR) + assert.equal(miss.metrics.metrics.taskDoneMedianMs, HOUR) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +// The shape, not only this afternoon's behaviour: the entry is shared by every Project on the repo, +// so a caller-scoped field in it is the same leak waiting for the next writer. The whitelist is the +// repository-level numbers and the three stamps beside them, and nothing computed from `tasks`. +test('#1880: the metrics cache holds repository-level numbers only', async () => { + const entries = new Map() + const cacheStore = { get: key => entries.get(key), set: (key, value) => entries.set(key, value) } + const tasks = Object.values(seed().tasks) + await readMetrics({ + repo: 'test/scope-7', + token: 'tok', + fetchImpl: async () => ({ ok: true, json: async () => [] }), + timeZone: 'UTC', + now: NOW, + tasks, + cacheStore, + }) + const [[key, entry]] = [...entries] + assert.equal(key, `test/scope-7|UTC|${Math.floor(NOW / (5 * 60 * 1000))}`, 'the key stays repo, time zone and bucket') + assert.deepEqual(Object.keys(entry.value).sort(), ['generatedAt', 'metrics', 'ok', 'timeZone']) + assert.deepEqual(Object.keys(entry.value.metrics).sort(), [ + 'issueCloseMedianMs', 'issuesClosedToday', 'issuesOpenedToday', + 'prMergeMedianMs', 'prsMergedToday', 'prsOpenedToday', + ]) + assert.equal(JSON.stringify(entry.value).includes('running'), false, 'the caller\'s rows are never cached') + assert.equal(JSON.stringify(entry.value).includes('taskDoneMedianMs'), false, 'nor the median computed from them') +}) + +// #1880: the projection that re-reads the caller's numbers on every read must not reach the +// unavailable branches. "Read it, could not" is answered with nulls, never with a zero that reads +// as "no card ever finished here" — the one thing #1848 and #1849 both keep. +test('#1880: an unavailable source keeps the done median null, never zero', async () => { + const tasks = Object.values(seed().tasks).filter(task => task.origin.roomId === 'r1') + const cacheStore = { get: () => undefined, set: () => {} } + const base = { repo: 'test/scope-8', timeZone: 'UTC', now: NOW, tasks, cacheStore } + const cases = [ + ['no-credentials', { ...base, token: '' }], + ['no-fetch', { ...base, token: 'tok' }], + ['read-failed', { ...base, token: 'tok', fetchImpl: async () => { throw new Error('socket hang up') } }], + ] + for (const [label, args] of cases) { + const read = await readMetrics(args) + assert.equal(read.ok, false, `${label} answers not ok`) + assert.match(read.reason, label === 'read-failed' ? /^read-failed: / : new RegExp(`^${label}$`)) + assert.equal(read.metrics.taskDoneMedianMs, null, `${label}: an unreadable source is not a zero median`) + assert.deepEqual(read.running.map(row => row.id), ['task-group'], `${label}: the local rows are still the board's`) + } +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 8dda7fcd8..50005a868 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -786,7 +786,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, 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) carries that same list. 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, 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). ## 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 ef60b22c6..03d95342f 100644 --- a/patches/ds-bot/files/src/host/project-metrics.js +++ b/patches/ds-bot/files/src/host/project-metrics.js @@ -127,16 +127,18 @@ export function summarizeMetrics({ now = Date.now(), timeZone = 'UTC', source, t // without a request, and one failed request answers unavailable for this tick rather than a cache // entry that would keep the failure for five minutes. `tasks` is the board's visible rows (#1848). export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', now = Date.now(), tasks, ttlMs = METRICS_TTL_MS, cacheStore = cache } = {}) { - // The cache carries the seven numbers only: a card claimed a moment ago must appear in the running - // table on the next read, so those rows are projected from the live tasks every time (#1829). + // The cache key is the repository and the five-minute bucket, not the reader, so it carries the + // repository-level numbers only. Everything computed from `tasks` — the running rows and the done + // median — is projected from this caller's list on every read, hit or miss (#1829, #1880): a + // shared entry must not answer one board with the numbers of the board that filled it. const liveRunning = () => taskList(tasks).map(task => runningOf(task, now)).filter(row => row !== null) - const withRunning = value => ({ ...value, running: liveRunning() }) + const withLive = value => ({ ...value, metrics: { ...value.metrics, taskDoneMedianMs: median(doneTaskDurations(tasks, now)) }, running: liveRunning() }) const bearer = text(token) if (bearer === '') return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: 'no-credentials' } }), cache: 'miss' } if (typeof fetchImpl !== 'function') return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: 'no-fetch' } }), cache: 'miss' } const key = `${repo}|${timeZone}|${Math.floor(now / ttlMs)}` const hit = cacheStore.get(key) - if (hit !== undefined && hit.expiresAt > now) return { ...withRunning(hit.value), cache: 'hit' } + if (hit !== undefined && hit.expiresAt > now) return { ...withLive(hit.value), cache: 'hit' } 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) => { @@ -155,7 +157,15 @@ export async function readMetrics({ repo, token, fetchImpl, timeZone = 'UTC', no return { ...summarizeMetrics({ now, timeZone, tasks, source: { ok: false, reason: `read-failed: ${error.message}` } }), cache: 'miss' } } const value = summarizeMetrics({ now, timeZone, source, tasks }) - const { running: _live, ...cached } = value - cacheStore.set(key, { expiresAt: now + ttlMs, value: cached }) - return { ...withRunning(value), cache: 'miss' } + // Written field by field rather than by spreading, so a caller-scoped number added later cannot + // ride into a shared entry by accident: this list is the whole shape the cache may hold. + const { prsOpenedToday, prsMergedToday, issuesOpenedToday, issuesClosedToday, prMergeMedianMs, issueCloseMedianMs } = value.metrics + const repositoryValue = { + ok: value.ok, + timeZone: value.timeZone, + generatedAt: value.generatedAt, + metrics: { prsOpenedToday, prsMergedToday, issuesOpenedToday, issuesClosedToday, prMergeMedianMs, issueCloseMedianMs }, + } + cacheStore.set(key, { expiresAt: now + ttlMs, value: repositoryValue }) + return { ...withLive(value), cache: 'miss' } } 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 a225e7acc..45599d69f 100644 --- a/patches/ds-bot/files/test/project-metrics-scope.test.js +++ b/patches/ds-bot/files/test/project-metrics-scope.test.js @@ -7,6 +7,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' import { createRuntime, provideFlyBot } from '../index.js' +import { readMetrics } from '../src/host/project-metrics.js' import { fakeContext } from './fake-ctx.js' const HOUR = 60 * 60 * 1000 @@ -170,3 +171,108 @@ test('#1848: the done-task median is this board\'s cards, not the deployment\'s' await stop() } }) + +// #1880: the metrics cache is keyed by repository, time zone and five-minute bucket only, so a hit +// is shared by every reader of that repo. The done-task median is computed from the reader's own +// cards, like the running table, so a hit must re-project it and never answer with the cards of +// whoever filled the entry first. +test('#1880: a cache hit projects the done-task median from the reader\'s own cards', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-5' + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + // The other board fills this bucket: one card, nine hours from start to done. + const other = await rt.projectBoard('project-p2', { memberId: 'owner-2' }) + assert.equal(other.metrics.cache, 'miss') + assert.equal(other.metrics.metrics.taskDoneMedianMs, 9 * HOUR) + // Same repo, same bucket, a different board: the read is a hit, and the median is this board's + // hour — not the nine hours the first reader's cards produced. + const mine = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(mine.metrics.cache, 'hit') + assert.equal(mine.metrics.metrics.taskDoneMedianMs, HOUR) + // A hit belongs to its reader, not to whoever filled the entry: back on the other board the + // nine hours return. + const back = await rt.projectBoard('project-p2', { memberId: 'owner-2' }) + assert.equal(back.metrics.cache, 'hit') + assert.equal(back.metrics.metrics.taskDoneMedianMs, 9 * HOUR) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +test('#1880: a cache hit\'s running rows and done median come from the same board', async () => { + const { rt, stop } = await start() + const realFetch = globalThis.fetch + const realSecret = rt.secretValue + rt.githubRepo = 'test/scope-6' + rt.secretValue = name => (name === 'GITHUB_TOKEN' ? 'tok' : undefined) + globalThis.fetch = async () => ({ ok: true, json: async () => [] }) + try { + const miss = await rt.projectBoard('project-p1', { memberId: 'owner-1' }) + assert.equal(miss.metrics.cache, 'miss') + const hit = await rt.projectBoard('project-p2', { memberId: 'owner-2' }) + assert.equal(hit.metrics.cache, 'hit') + // One projection, one source: the rows are this board's card and the median beside it is this + // board's nine hours, never a row from one reader with a number from the other. + assert.deepEqual(hit.metrics.running.map(row => row.id), ['task-foreign']) + assert.equal(hit.metrics.metrics.taskDoneMedianMs, 9 * HOUR) + assert.equal(miss.metrics.metrics.taskDoneMedianMs, HOUR) + } finally { + globalThis.fetch = realFetch + rt.secretValue = realSecret + await stop() + } +}) + +// The shape, not only this afternoon's behaviour: the entry is shared by every Project on the repo, +// so a caller-scoped field in it is the same leak waiting for the next writer. The whitelist is the +// repository-level numbers and the three stamps beside them, and nothing computed from `tasks`. +test('#1880: the metrics cache holds repository-level numbers only', async () => { + const entries = new Map() + const cacheStore = { get: key => entries.get(key), set: (key, value) => entries.set(key, value) } + const tasks = Object.values(seed().tasks) + await readMetrics({ + repo: 'test/scope-7', + token: 'tok', + fetchImpl: async () => ({ ok: true, json: async () => [] }), + timeZone: 'UTC', + now: NOW, + tasks, + cacheStore, + }) + const [[key, entry]] = [...entries] + assert.equal(key, `test/scope-7|UTC|${Math.floor(NOW / (5 * 60 * 1000))}`, 'the key stays repo, time zone and bucket') + assert.deepEqual(Object.keys(entry.value).sort(), ['generatedAt', 'metrics', 'ok', 'timeZone']) + assert.deepEqual(Object.keys(entry.value.metrics).sort(), [ + 'issueCloseMedianMs', 'issuesClosedToday', 'issuesOpenedToday', + 'prMergeMedianMs', 'prsMergedToday', 'prsOpenedToday', + ]) + assert.equal(JSON.stringify(entry.value).includes('running'), false, 'the caller\'s rows are never cached') + assert.equal(JSON.stringify(entry.value).includes('taskDoneMedianMs'), false, 'nor the median computed from them') +}) + +// #1880: the projection that re-reads the caller's numbers on every read must not reach the +// unavailable branches. "Read it, could not" is answered with nulls, never with a zero that reads +// as "no card ever finished here" — the one thing #1848 and #1849 both keep. +test('#1880: an unavailable source keeps the done median null, never zero', async () => { + const tasks = Object.values(seed().tasks).filter(task => task.origin.roomId === 'r1') + const cacheStore = { get: () => undefined, set: () => {} } + const base = { repo: 'test/scope-8', timeZone: 'UTC', now: NOW, tasks, cacheStore } + const cases = [ + ['no-credentials', { ...base, token: '' }], + ['no-fetch', { ...base, token: 'tok' }], + ['read-failed', { ...base, token: 'tok', fetchImpl: async () => { throw new Error('socket hang up') } }], + ] + for (const [label, args] of cases) { + const read = await readMetrics(args) + assert.equal(read.ok, false, `${label} answers not ok`) + assert.match(read.reason, label === 'read-failed' ? /^read-failed: / : new RegExp(`^${label}$`)) + assert.equal(read.metrics.taskDoneMedianMs, null, `${label}: an unreadable source is not a zero median`) + assert.deepEqual(read.running.map(row => row.id), ['task-group'], `${label}: the local rows are still the board's`) + } +})