From ab68fc49d07734dd0aa1ff260430ca94527399a8 Mon Sep 17 00:00:00 2001 From: HugoXK Date: Sun, 15 Nov 2026 07:31:29 +0000 Subject: [PATCH] feat: show what the change is on the job page (#373) Slice 1 of #373. A job page proved the checks passed but not what the change was, so a maintainer had to open the pull request to learn it. The page now opens with a "Why this change" block above the evidence rows: - `src/lib/changeStory.ts` (new, pure) splits the pull request body on its `## What changed` / `## Risks` / `## Ran` headings (any level, plus the bare `Ran:` line this repository's template uses). A heading the body does not carry renders `not stated` and is never filled from another section; a repeated heading keeps the last occurrence and the block says so, so an edited body cannot silently show the abandoned draft. - `PullRequestInfo.body` carries the body the agent already wrote, and `pullRequestDiffTotals` reduces the existing changed-file walk to `{ files, additions, deletions }`. A capped walk reports no totals rather than a total from part of the change, the same rule the path guard states. - `GET /repos/:id/jobs/:jobId` makes one cached read per job (60 s) to fill the block; a job with no pull request reads nothing at all. A read that fails leaves the page at 200 and names the unavailable fact. - The block is bilingual and every value is escaped; the body text is data on this page, never markup. Docs: docs/api.md (the new reader field and totals helper), docs/architecture.md (the block, the cache and the pure parser). Changelog: one `added` fragment. Fixes #373 Written-by: HugoXK --- changelog.d/373-change-story.added.md | 1 + docs/api.md | 7 + docs/architecture.md | 25 +++ src/app.ts | 42 ++++- src/lib/changeStory.ts | 175 +++++++++++++++++++ src/repos/github.ts | 37 +++- src/views/pages.ts | 32 ++++ test/changeStory.test.ts | 240 ++++++++++++++++++++++++++ 8 files changed, 557 insertions(+), 2 deletions(-) create mode 100644 changelog.d/373-change-story.added.md create mode 100644 src/lib/changeStory.ts create mode 100644 test/changeStory.test.ts diff --git a/changelog.d/373-change-story.added.md b/changelog.d/373-change-story.added.md new file mode 100644 index 00000000..3032c22b --- /dev/null +++ b/changelog.d/373-change-story.added.md @@ -0,0 +1 @@ +The job page now opens with a "Why this change" block: what the agent said it changed, the risks it named, what it ran, and the size of the diff, read from the pull request body and its changed-file totals. A section the body does not carry says "not stated" instead of guessing, a repeated heading shows the last one and says so, and a pull request that cannot be read leaves the page at 200 with the unavailable fact named. diff --git a/docs/api.md b/docs/api.md index b3db25a0..c55d1b9e 100644 --- a/docs/api.md +++ b/docs/api.md @@ -438,6 +438,13 @@ refused rather than judged, which is deliberate — an unread file is not a permitted one. Rows stored before these fields existed keep their old sentence and are upgraded on the next check, so there is no migration. +`PullRequestInfo` also carries the pull request `body`, which the reader never +interprets: the job page shows the three sections the agent wrote into it. +`pullRequestDiffTotals` reduces the same listing to `{ files, additions, +deletions }` for that page, and reports no totals when the walk was capped — the +same rule as the path guard, stated once: a partial listing is unavailable, never +rounded down and presented as the change. + ## Job endpoints These are called by the FlyBot bridge/agent, not by a browser. diff --git a/docs/architecture.md b/docs/architecture.md index 505c6699..984d17e0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1615,6 +1615,31 @@ makes the sweep idempotent: a rerun offers nothing and mails nobody, and an account restored by hand stays skipped. Operator output carries the login and the registration date, never an address. +### The job page's change story + +The job page answers "did it pass", and a maintainer's first question is "what +is the change". `GET /repos/:id/jobs/:jobId` therefore makes one read of the +job's pull request and hands the render a `ChangeStory`: the body split into +what changed, risks and ran, plus the diff totals. It is a read model over what +the agent already wrote — no model call and no summary of a summary. + +The read is cached per job for 60 seconds, the same shape as `inboxCache` above, +so refreshing the page spends no GitHub request; a job with no pull request +skips the read entirely and the block is absent. The body comes from +`PullRequestInfo.body`, and the totals from `pullRequestDiffTotals`, which sums +the existing changed-file walk. When that walk hits its own cap the totals are +reported as unavailable rather than summed: a number taken from part of a change +would understate it. + +`src/lib/changeStory.ts` is pure. It recognises `## What changed`, `## Risks` +and `## Ran` at any heading level, and the bare `Ran:` line this repository's PR +template uses; a repeated heading keeps the last occurrence and the block says +so, so an edited body cannot silently show the abandoned draft. A heading the +body does not carry renders `not stated` and is never filled from another +section. A read that fails — 404, no token, a capped list — leaves the page at +200 and names the unavailable fact instead, because a page that 500s on a read +it did not need is worse than no block. + ### Email notifications Fledge sends mail from the configured `MAIL_FROM` (production: diff --git a/src/app.ts b/src/app.ts index 1a5c4792..944169eb 100644 --- a/src/app.ts +++ b/src/app.ts @@ -29,8 +29,9 @@ import { parseAttributionJson } from './lib/attribution.js' import { failureBreakdown } from './lib/failure.js' import { fetchFailureLog, logFailureReason } from './actions/workflowLog.js' import { parseVerification } from './lib/verification.js' +import { readChangeStory, type ChangeStory } from './lib/changeStory.js' import { DEFAULT_TRIAGE_GOAL, Jobs } from './jobs/service.js' -import { parseIssueUrl } from './jobs/prUrl.js' +import { parseIssueUrl, parsePrUrl } from './jobs/prUrl.js' import { createRunner, type Runner } from './jobs/runner.js' import { log } from './lib/log.js' import type { PriorityLevel } from './lib/priority.js' @@ -835,6 +836,41 @@ export async function buildApp(options: BuildOptions): Promise { error: job.error === null ? null : localizeMessage(locale, job.error) ?? job.error, })) + // #373 slice 1: the job page's "Why this change" block reads the pull request + // body and the changed-file totals. One read per job with a 60 s bound, the + // same shape as `inboxCache` above: a page refresh must not spend a GitHub + // request, and a job without a pull request spends none at all. + const changeStoryTtlMs = 60_000 + const changeStoryCache = new Map() + const changeStoryFor = async (repo: RepoRow, job: JobRow): Promise => { + if (job.pr_url === null || job.pr_url === '') return undefined + const cached = changeStoryCache.get(job.id) + if (cached !== undefined && Date.now() - cached.at < changeStoryTtlMs) return cached.story + const parsed = parsePrUrl(repo, job.pr_url, config.github.apiBase) + let story: ChangeStory + if (!parsed.ok) { + story = readChangeStory({ body: null, totals: null, diffState: 'unreadable' }) + } else { + try { + const token = await repoGithubToken(repo, config, githubApp, store) + const client = new GithubClient(token, config.github.apiBase) + const pr = await client.getPullRequest(parsed.ref.owner, parsed.ref.name, parsed.ref.number) + const diff = await client.pullRequestDiffTotals(parsed.ref.owner, parsed.ref.name, parsed.ref.number, pr.changedFiles) + story = readChangeStory({ body: pr.body, totals: diff.totals, diffState: diff.complete ? 'ok' : 'capped' }) + } catch (error) { + // A read that failed is a fact the block reports, not a reason to fail + // the page: the evidence below it is what the maintainer came for. + log.warn('change story read failed', { repo: repo.id, job: job.id, error: error instanceof Error ? error.message : String(error) }) + story = readChangeStory({ body: null, totals: null, diffState: 'unreadable' }) + } + } + if (changeStoryCache.size >= 200) { + for (const [key, entry] of changeStoryCache) if (Date.now() - entry.at >= changeStoryTtlMs) changeStoryCache.delete(key) + } + changeStoryCache.set(job.id, { at: Date.now(), story }) + return story + } + const requireAdvanced = (viewer: { user: UserRow; preview?: boolean }): void => { if (viewer.user.role !== 'superadmin' || viewer.preview === true) throw httpError(403, '仅超级管理员可访问实时团队页面。', 'SUPERADMIN_ONLY') } @@ -2971,12 +3007,16 @@ export async function buildApp(options: BuildOptions): Promise { // label map keeps the viewer's own repositories readable without naming // anybody else's. const repoLabels = new Map(store.listRepos(viewer.user.id).map((entry) => [entry.id, `${entry.upstream_owner || entry.owner}/${entry.upstream_name || entry.name}`])) + // #373: one read of the pull request, cached per job, so the page can say + // what the change is and not only that its checks passed. + const changeStory = await changeStoryFor(repo, job) return reply.type('text/html').send(jobPage({ config, locale: request.locale ?? 'zh', viewer: viewOf(viewer), repo, job, + changeStory, queued, viewerActive: store.listActiveJobsForUser(viewer.user.id), repoLabels, diff --git a/src/lib/changeStory.ts b/src/lib/changeStory.ts new file mode 100644 index 00000000..c755d25b --- /dev/null +++ b/src/lib/changeStory.ts @@ -0,0 +1,175 @@ +// #373 slice 1: a job page is where a maintainer decides, and until now it +// showed what the checks proved but not what the agent said it changed, what it +// thought could go wrong, and what it actually ran — the three sections every +// PR body in this repository already carries. This module is the read model: +// it parses the body the agent wrote and adds the diff totals. Pure, no model +// call, no network. + +export const CHANGE_STORY_KEYS = ['whatChanged', 'risks', 'ran'] as const + +export type ChangeStoryKey = (typeof CHANGE_STORY_KEYS)[number] + +export interface DiffTotals { + files: number + additions: number + deletions: number +} + +export interface ChangeStorySection { + key: ChangeStoryKey + // `null` means the body does not carry that heading. It is never filled from + // another section: the block says "not stated" instead of guessing. + text: string | null + // The heading appeared more than once; the last occurrence is the text above. + duplicated: boolean +} + +export type ChangeStoryDiffState = 'ok' | 'capped' | 'unreadable' + +export interface ChangeStory { + // `unreadable` means GitHub could not be read at all (404, no token, a failed + // request): every fact is then unavailable rather than "not stated". + bodyState: 'ok' | 'unreadable' + sections: ChangeStorySection[] + totals: DiffTotals | null + diffState: ChangeStoryDiffState +} + +// Labels are the caller's: the parser is language-free and the page passes the +// reader's language in. +export interface ChangeStoryLabels { + notStated?: string + unavailable?: string + capped?: string + duplicate?: string + files?: (count: number) => string +} + +export interface ChangeStoryFact { + key: ChangeStoryKey | 'diff' + value: string + // A second sentence the page shows under the value; empty when there is none. + note: string +} + +const HEADINGS: Record = { + 'what changed': 'whatChanged', + risks: 'risks', + risk: 'risks', + ran: 'ran', +} + +// `## Risks`, `### Ran`, `# What changed`. The level does not matter: any heading +// the body carries is a boundary, and the ones that name a fact start one. +function headingKey(line: string): ChangeStoryKey | null { + const heading = /^#{1,6}\s+(.*)$/.exec(line.trim()) + if (heading === null) return null + const text = (heading[1] ?? '').replace(/[:\s]+$/, '').toLowerCase() + return HEADINGS[text] ?? null +} + +// A `Not run:` line is a fact of its own ("what the agent did not run"), so it +// ends the section above it instead of being read as part of `Ran`. +function isNotRun(line: string): boolean { + return /^not\s+run\s*:?\s*$/i.test(line.trim()) +} + +// The house PR template uses a bare `Ran:` line rather than a heading, so the +// block reads this repository's own pull requests and not only PRs that adopted +// heading style. +function isBareRan(line: string): boolean { + return /^ran\s*:?\s*$/i.test(line.trim()) +} + +function isFence(line: string): boolean { + return /^\s*(```|~~~)/.test(line) +} + +function endsSection(line: string): boolean { + return /^#{1,6}\s+/.test(line.trim()) || isNotRun(line) +} + +// A fenced block may hold `#` comments; they are text, not headings. +export function parseChangeStoryBody(body: string): ChangeStorySection[] { + const lines = body.replace(/\r\n/g, '\n').split('\n') + const found = new Map() + let current: ChangeStoryKey | null = null + let buffer: string[] = [] + let fenced = false + const flush = (): void => { + if (current === null) return + const text = buffer.join('\n').trim() + const prior = found.get(current) + found.set(current, { text: text === '' ? null : text, seen: (prior?.seen ?? 0) + 1 }) + buffer = [] + } + for (const line of lines) { + if (isFence(line)) { + fenced = !fenced + if (current !== null) buffer.push(line) + continue + } + if (!fenced) { + const key = headingKey(line) + if (key !== null) { + flush() + current = key + continue + } + if (isBareRan(line)) { + flush() + current = 'ran' + continue + } + if (endsSection(line)) { + flush() + current = null + continue + } + } + if (current !== null) buffer.push(line) + } + flush() + return CHANGE_STORY_KEYS.map((key) => { + const entry = found.get(key) + return { key, text: entry?.text ?? null, duplicated: (entry?.seen ?? 0) > 1 } + }) +} + +// `totals` is the caller's sum (the reader owns the file walk and its cap); +// `diffState` says whether that sum is the whole change. +export function readChangeStory(input: { + body: string | null + totals: DiffTotals | null + diffState: ChangeStoryDiffState +}): ChangeStory { + const bodyState: ChangeStory['bodyState'] = input.body === null ? 'unreadable' : 'ok' + const sections = bodyState === 'ok' + ? parseChangeStoryBody(input.body ?? '') + : CHANGE_STORY_KEYS.map((key) => ({ key, text: null, duplicated: false })) + // A sum that was not produced cannot be rendered as a state of its own. + const diffState: ChangeStoryDiffState = input.diffState === 'ok' && input.totals === null ? 'unreadable' : input.diffState + return { bodyState, sections, totals: diffState === 'ok' ? input.totals : null, diffState } +} + +const DEFAULT_LABELS: Required = { + notStated: 'not stated', + unavailable: 'not available', + capped: 'the file list exceeded the cap', + duplicate: 'a repeated heading; the last one is shown', + files: (count: number) => `${count} files`, +} + +export function changeStoryFacts(story: ChangeStory, labels: ChangeStoryLabels = {}): ChangeStoryFact[] { + const l = { ...DEFAULT_LABELS, ...labels } + const facts: ChangeStoryFact[] = story.sections.map((section) => ({ + key: section.key, + value: section.text ?? (story.bodyState === 'ok' ? l.notStated : l.unavailable), + note: section.duplicated ? l.duplicate : '', + })) + const diffValue = story.diffState === 'ok' && story.totals !== null + ? `${l.files(story.totals.files)} +${story.totals.additions} −${story.totals.deletions}` + : story.diffState === 'capped' ? l.capped : l.unavailable + facts.push({ key: 'diff', value: diffValue, note: '' }) + return facts +} diff --git a/src/repos/github.ts b/src/repos/github.ts index 6eb8a03e..78274e17 100644 --- a/src/repos/github.ts +++ b/src/repos/github.ts @@ -61,6 +61,24 @@ export interface PullRequestInfo { draft: boolean merged: boolean changedFiles: number + // #373: the body is what the agent already wrote about the change. The job + // page shows it; nothing here interprets it. + body: string +} + +// #373: the job page shows how large the change is. The per-file walk below +// already carries `additions` and `deletions`, so the sum is defined once, next +// to the reader that owns the walk and its cap. +export function summarizePullRequestFiles( + files: ReadonlyArray<{ additions: number; deletions: number }>, +): { files: number; additions: number; deletions: number } { + let additions = 0 + let deletions = 0 + for (const file of files) { + additions += Number.isFinite(file.additions) ? file.additions : 0 + deletions += Number.isFinite(file.deletions) ? file.deletions : 0 + } + return { files: files.length, additions, deletions } } export interface CheckRunInfo { @@ -506,7 +524,7 @@ export class GithubClient { name: string, input: { title: string; head: string; base: string; body: string }, ): Promise { - const pr = await this.request<{ number: number; html_url: string; title: string; state: string; head: { ref: string; sha: string }; base: { ref: string }; mergeable?: boolean | null; mergeable_state?: string; draft?: boolean; merged?: boolean; changed_files?: number }>( + const pr = await this.request<{ number: number; html_url: string; title: string; state: string; head: { ref: string; sha: string }; base: { ref: string }; mergeable?: boolean | null; mergeable_state?: string; draft?: boolean; merged?: boolean; changed_files?: number; body?: string | null }>( 'POST', `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/pulls`, input, @@ -524,6 +542,7 @@ export class GithubClient { draft: pr.draft === true, merged: pr.merged === true, changedFiles: Number(pr.changed_files ?? 0), + body: typeof pr.body === 'string' ? pr.body : '', } } @@ -540,6 +559,7 @@ export class GithubClient { draft: boolean merged: boolean changed_files?: number + body?: string | null }>( 'GET', `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/pulls/${number}`, @@ -557,6 +577,7 @@ export class GithubClient { draft: pr.draft, merged: pr.merged, changedFiles: Number(pr.changed_files ?? 0), + body: typeof pr.body === 'string' ? pr.body : '', } } @@ -836,6 +857,20 @@ export class GithubClient { return { files, complete } } + // #373: the job page needs the change size, not the file list. A capped walk is + // reported as such rather than summed: a total taken from part of the change + // would understate it, and the page says the fact is unavailable instead. + async pullRequestDiffTotals( + owner: string, + name: string, + number: number, + reportedFiles = 0, + ): Promise<{ totals: { files: number; additions: number; deletions: number } | null; complete: boolean }> { + const listed = await this.listPullRequestFiles(owner, name, number, reportedFiles) + if (!listed.complete) return { totals: null, complete: false } + return { totals: summarizePullRequestFiles(listed.files), complete: true } + } + async getBranchProtection(owner: string, name: string, branch: string): Promise<{ protected: boolean; requiredChecks: string[]; requiredApprovals: number } | null> { try { const result = await this.request<{ diff --git a/src/views/pages.ts b/src/views/pages.ts index 2020f48c..5a406d9d 100644 --- a/src/views/pages.ts +++ b/src/views/pages.ts @@ -23,6 +23,7 @@ import { WORK_ITEM_STATES, WORK_ITEM_TRANSITIONS, workItemStateLabel, type WorkI import { PAT_EXPIRY_CHOICES } from '../lib/tokens.js' import { ATTRIBUTION_FIELDS, attributionJourney, parseAttributionJson } from '../lib/attribution.js' import { parseEvidenceJson } from '../lib/evidence.js' +import { changeStoryFacts, type ChangeStory, type ChangeStoryFact } from '../lib/changeStory.js' import type { PreviewResult } from '../schedules/preview.js' import { intervalFieldText, type ScheduleConsoleState } from '../schedules/console.js' import { contractOf, resolveWorkType, workTypeLabel, workTypePurpose } from '../lib/workTypes.js' @@ -2699,6 +2700,15 @@ function priorityLevelLabel(t: Translate, level: string): string { return t('非核心打磨', 'non-core polish') } +// #373: the four facts the block names, in the reader's language. The values +// themselves come from the pull request and are never translated. +function changeStoryLabel(t: Translate, key: ChangeStoryFact['key']): string { + if (key === 'whatChanged') return t('改动了什么', 'What changed') + if (key === 'risks') return t('风险', 'Risks') + if (key === 'ran') return t('运行了什么', 'Ran') + return t('改动规模', 'Diff') +} + export function jobPage(input: { config: Config locale: Locale @@ -2715,6 +2725,9 @@ export function jobPage(input: { postmortem?: PostmortemRow | null // #639: relations are explicit; the page lists them and offers one write. relations?: TaskRelationRow[] + // #373 slice 1: the "Why this change" block, read from the pull request. Absent + // on a surface that has no pull request to read. + changeStory?: ChangeStory // #540: the revision this render saw and the key a double submit replays. // Absent on surfaces that cannot write (the admin support view). cancel?: { revision: number; idempotencyKey: string } @@ -2842,6 +2855,24 @@ export function jobPage(input: {
${escapeHtml(t('风险', 'Risk'))}${escapeHtml(riskLabel)}
${escapeHtml(t('回滚', 'Rollback'))}${escapeHtml(evidence.rollback ?? '—')}
` + // #373 slice 1: what the agent said it changed, what it said could go wrong, + // what it ran, and how large the diff is — above the evidence block, because + // it is the question a maintainer asks before reading the checks. Every value + // is data from the pull request: escaped, never markup. + const changeStoryCard = input.changeStory === undefined + ? '' + : `
+

${escapeHtml(t('为什么有这个改动', 'Why this change'))}

+
+ ${changeStoryFacts(input.changeStory, { + notStated: t('未说明', 'not stated'), + unavailable: t('读不到', 'not available'), + capped: t('读不到：改动文件超过读取上限', 'not available: the file list exceeded the read cap'), + duplicate: t('标题重复，显示最后一处', 'a repeated heading; the last one is shown'), + files: (count) => t(`${count} 个文件`, `${count} files`), + }).map((fact) => `
${escapeHtml(changeStoryLabel(t, fact.key))}${escapeHtml(fact.value)}${fact.note === '' ? '' : `${escapeHtml(fact.note)}`}
`).join('')} +
+
` const retry = job.status === 'failed' ? `
${csrf(viewer)}${retryHidden}
` : '' @@ -3037,6 +3068,7 @@ export function jobPage(input: { ${postmortemCard} ${supportMode ? '' : workTypeCard} ${supportMode ? '' : triageCard} + ${changeStoryCard}

${escapeHtml(t('结果', 'Result'))}

diff --git a/test/changeStory.test.ts b/test/changeStory.test.ts new file mode 100644 index 00000000..8f8f9c4c --- /dev/null +++ b/test/changeStory.test.ts @@ -0,0 +1,240 @@ +import test from 'node:test' +import assert from 'node:assert/strict' +import type { IncomingMessage, ServerResponse } from 'node:http' +import { buildApp } from '../src/app.js' +import { Store } from '../src/db.js' +import { MockRunner } from '../src/jobs/mock.js' +import { changeStoryFacts, readChangeStory } from '../src/lib/changeStory.js' +import { summarizePullRequestFiles } from '../src/repos/github.js' +import { encryptSecret } from '../src/lib/secrets.js' +import { json, makeUser, MemoryMailer, sessionFor, startFakeServer, testConfig } from './helpers.js' + +// #373 slice 1: the job page is where a maintainer decides. It showed the +// checks and the structured evidence, but not the three sentences the agent +// already wrote into the pull request body — what changed, what could go wrong, +// what it ran — nor how large the diff is. The block below is a read model over +// data GitHub already holds: no model call, no summary of a summary. + +const storyOf = ( + body: string | null, + files: Array<{ additions: number; deletions: number }> = [], + complete = true, +) => readChangeStory({ + body, + totals: complete ? summarizePullRequestFiles(files) : null, + diffState: complete ? 'ok' : 'capped', +}) + +const COMPLETE = [ + '## What changed', + 'The job page renders a read model over the pull request.', + '', + '## Risks', + 'One extra GitHub read per page view; it is cached for 60 seconds.', + '', + '## Ran', + '- `npm run check` — pass', +].join('\n') + +test('a complete body yields the three sections with their own text', () => { + const story = storyOf(COMPLETE) + assert.equal(story.bodyState, 'ok') + assert.deepEqual(story.sections.map((section) => section.key), ['whatChanged', 'risks', 'ran']) + const byKey = new Map(story.sections.map((section) => [section.key, section])) + assert.equal(byKey.get('whatChanged')?.text, 'The job page renders a read model over the pull request.') + assert.match(byKey.get('risks')?.text ?? '', /cached for 60 seconds/) + assert.match(byKey.get('ran')?.text ?? '', /npm run check/) + assert.equal(byKey.get('risks')?.duplicated, false) +}) + +test('a repeated heading keeps the last occurrence and reports the duplicate', () => { + const body = ['## Risks', 'first answer', '## Risks', 'second answer'].join('\n') + const story = storyOf(body) + const risks = story.sections.find((section) => section.key === 'risks') + assert.equal(risks?.text, 'second answer') + assert.equal(risks?.duplicated, true) + const fact = changeStoryFacts(story).find((entry) => entry.key === 'risks') + assert.match(fact?.note ?? '', /repeated/i, 'the block says the heading was repeated') +}) + +test('an empty body renders not stated three times and still lists every fact', () => { + const story = storyOf('') + const facts = changeStoryFacts(story) + assert.deepEqual(facts.map((fact) => fact.key), ['whatChanged', 'risks', 'ran', 'diff']) + assert.deepEqual(facts.slice(0, 3).map((fact) => fact.value), ['not stated', 'not stated', 'not stated']) +}) + +test('a heading the body does not carry is not stated, and is never filled from another section', () => { + const body = ['## What changed', 'only this one', '## Ran', '- tests'].join('\n') + const facts = changeStoryFacts(storyOf(body)) + const risks = facts.find((fact) => fact.key === 'risks') + assert.equal(risks?.value, 'not stated') + assert.doesNotMatch(risks?.value ?? '', /only this one/) +}) + +test('the diff line sums the file list', () => { + const files = [ + { additions: 10, deletions: 2 }, + { additions: 4, deletions: 0 }, + { additions: 1, deletions: 1 }, + ] + const totals = summarizePullRequestFiles(files) + assert.deepEqual(totals, { files: 3, additions: 15, deletions: 3 }) + const facts = changeStoryFacts(storyOf(COMPLETE, files)) + assert.equal(facts.find((fact) => fact.key === 'diff')?.value, '3 files +15 −3') +}) + +test('a capped file list makes the diff unavailable instead of wrong', () => { + const files = [{ additions: 10, deletions: 2 }] + const story = storyOf(COMPLETE, files, false) + assert.equal(story.totals, null) + assert.equal(story.diffState, 'capped') + const fact = changeStoryFacts(story).find((entry) => entry.key === 'diff') + assert.match(fact?.value ?? '', /cap/i) + // The three sections are still readable: one unavailable fact does not hide the others. + assert.deepEqual( + changeStoryFacts(story).slice(0, 3).map((entry) => entry.value.includes('not stated')), + [false, false, false], + ) +}) + +test('a pull request that could not be read says which fact is unavailable', () => { + const story = readChangeStory({ body: null, totals: null, diffState: 'unreadable' }) + assert.equal(story.bodyState, 'unreadable') + assert.equal(story.diffState, 'unreadable') + const facts = changeStoryFacts(story) + assert.equal(facts.length, 4) + assert.deepEqual(facts.map((fact) => fact.value), ['not available', 'not available', 'not available', 'not available']) +}) + +test('the bare Ran: line the house PR template uses is read like the heading', () => { + const body = ['## What changed', 'something', 'Ran:', '- `npm run check` — pass', 'Not run:', '- Docker'].join('\n') + const story = storyOf(body) + const ran = story.sections.find((section) => section.key === 'ran') + assert.match(ran?.text ?? '', /npm run check/) + assert.doesNotMatch(ran?.text ?? '', /Docker/, 'Not run: belongs to no section and must not be swallowed') +}) + +test('a value from the body is escaped by the page, never rendered as markup', async () => { + const github = await startFakeServer((request, response) => { + if ((request.url ?? '').startsWith('/repos/acme/demo/pulls/9/files')) { + json(response, 200, [{ filename: 'src/a.ts', additions: 1, deletions: 0 }]) + return + } + if ((request.url ?? '').startsWith('/repos/acme/demo/pulls/9')) { + json(response, 200, { + number: 9, + html_url: 'https://github.com/acme/demo/pull/9', + title: 't', + state: 'open', + head: { ref: 'dev1/x', sha: 'a'.repeat(40) }, + base: { ref: 'main' }, + mergeable: true, + mergeable_state: 'blocked', + draft: false, + merged: false, + changed_files: 1, + body: ['## What changed', 'bold', '## Risks', 'none', '## Ran', '- tests'].join('\n'), + }) + return + } + json(response, 404, { message: 'not found' }) + }) + const base = testConfig() + const config = testConfig({ github: { ...base.github, apiBase: github.url } }) + const store = new Store(':memory:') + const built = await buildApp({ config, store, mailer: new MemoryMailer(), runner: new MockRunner(), startScheduler: false }) + try { + const { repo, user } = fixture(store, config, { upstreamHtmlUrl: 'https://github.com' }) + const job = store.createJob({ id: 'job_story', repoId: repo.id, userId: user.id, trigger: 'manual', goal: 'g', callbackHash: 'cb-story', platformTokenHash: '' }) + store.updateJob(job.id, { status: 'succeeded', branch: 'dev1/x', pr_url: 'https://github.com/acme/demo/pull/9' }) + const { cookie } = sessionFor(built, user) + const page = await built.app.inject({ method: 'GET', url: `/repos/${repo.id}/jobs/${job.id}`, headers: { cookie: `${cookie}; fledge_lang=en` } }) + assert.equal(page.statusCode, 200) + assert.match(page.body, /Why this change/) + assert.match(page.body, /What changed/) + assert.match(page.body, /<b>bold<\/b>/, 'the body text is shown escaped') + assert.doesNotMatch(page.body, /bold<\/b>/) + assert.match(page.body, /1 files \+1 −0/) + } finally { + await built.app.close() + await github.close() + store.close() + } +}) + +test('a pull request read that 404s keeps the page at 200 and says the fact is unavailable', async () => { + let calls = 0 + const github = await startFakeServer((_request: IncomingMessage, response: ServerResponse) => { + calls += 1 + json(response, 404, { message: 'not found' }) + }) + const base = testConfig() + const config = testConfig({ github: { ...base.github, apiBase: github.url } }) + const store = new Store(':memory:') + const built = await buildApp({ config, store, mailer: new MemoryMailer(), runner: new MockRunner(), startScheduler: false }) + try { + const { repo, user } = fixture(store, config, { upstreamHtmlUrl: 'https://github.com' }) + const job = store.createJob({ id: 'job_story_404', repoId: repo.id, userId: user.id, trigger: 'manual', goal: 'g', callbackHash: 'cb-404', platformTokenHash: '' }) + store.updateJob(job.id, { status: 'succeeded', branch: 'dev1/x', pr_url: 'https://github.com/acme/demo/pull/9' }) + const { cookie } = sessionFor(built, user) + const page = await built.app.inject({ method: 'GET', url: `/repos/${repo.id}/jobs/${job.id}`, headers: { cookie: `${cookie}; fledge_lang=en` } }) + assert.equal(page.statusCode, 200, 'a GitHub hiccup must not take the page down') + assert.match(page.body, /Why this change/) + assert.match(page.body, /not available/) + assert.ok(calls > 0, 'the read happened and its failure is what the block reports') + } finally { + await built.app.close() + await github.close() + store.close() + } +}) + +test('a job without a pull request makes no GitHub read at all', async () => { + let calls = 0 + const github = await startFakeServer((_request: IncomingMessage, response: ServerResponse) => { + calls += 1 + json(response, 404, { message: 'not found' }) + }) + const base = testConfig() + const config = testConfig({ github: { ...base.github, apiBase: github.url } }) + const store = new Store(':memory:') + const built = await buildApp({ config, store, mailer: new MemoryMailer(), runner: new MockRunner(), startScheduler: false }) + try { + const { repo, user } = fixture(store, config, { upstreamHtmlUrl: 'https://github.com' }) + const job = store.createJob({ id: 'job_no_pr', repoId: repo.id, userId: user.id, trigger: 'manual', goal: 'g', callbackHash: 'cb-nopr', platformTokenHash: '' }) + store.updateJob(job.id, { status: 'succeeded' }) + const { cookie } = sessionFor(built, user) + const page = await built.app.inject({ method: 'GET', url: `/repos/${repo.id}/jobs/${job.id}`, headers: { cookie } }) + assert.equal(page.statusCode, 200) + assert.equal(calls, 0, 'no pull request means no read and no budget spent') + assert.doesNotMatch(page.body, /Why this change/) + } finally { + await built.app.close() + await github.close() + store.close() + } +}) + +function fixture( + store: Store, + config: ReturnType, + extra: { upstreamHtmlUrl?: string } = {}, +): { repo: ReturnType; user: ReturnType } { + const user = makeUser(store, { login: `story-${Math.random().toString(36).slice(2, 8)}`, status: 'approved' }) + const repo = store.createRepo({ + id: `repo_story_${Math.random().toString(36).slice(2, 8)}`, + userId: user.id, + owner: 'acme', + name: 'demo', + defaultBranch: 'main', + tokenEnc: encryptSecret('gh-token', config.tokenEncryptionKey), + permission: 'admin', + repoRole: 'maintainer', + upstreamHtmlUrl: extra.upstreamHtmlUrl ?? '', + goal: '', + autoIterate: false, + intervalMinutes: 1440, + }) + return { repo, user } +}