diff --git a/README.md b/README.md index 7c1bd332..c2389565 100644 --- a/README.md +++ b/README.md @@ -355,11 +355,13 @@ On first run it writes `~/.diffity/inbox/config.json`: | `worktreesDir` | Where each pull request gets its worktree. | | `filter` | Your own words on what does and doesn't need your attention, handed to the agent — it answers with a skip instead of reviewing when a PR matches (e.g. "Skip payments-focused PRs"). Editable from the page's Settings panel. | | `alertWhen` | Your own words on what needs you *now*. The agent judges each prepared review against them and flags the ones that match; the page notifies for those only — empty means every prepared review. Editable from the page's Settings panel. | +| `alertPaths` | Globs against the pull request's changed paths, one per line in the page — `packages/shared/src/model/**`, `**/dbref/**`. A changed file matching one marks the review as needing you now, whatever the agent made of `alertWhen`. Editable from the page's Settings panel. | | `agent.model` | `--model` for the review agent; `null` leaves its own default. Editable from the page. | | `agent.effort` | `--effort`: `low`, `medium`, `high`, `xhigh` or `max`; `null` leaves its own default. Editable from the page. | | `agent.mcpAllow` | The exact MCP tool names the agent may call, e.g. `mcp__claude_ai_Atlassian__getJiraIssue`. Empty (the default) means no MCP servers at all. Editable from the page. | | `agent.extraArgs` | Appended verbatim to the built command, for a flag this table does not cover. | | `agent.maxBudgetUsd` | `--max-budget-usd` for one run; `null` leaves it uncapped. A run that hits it is a failed attempt with that reason. Editable from the page. | +| `waitForCi` | Whether a pull request waits for green CI before an agent is spent on it (default false). Editable from the page. | | `prepareTimeoutMinutes` | How long one preparation may take before it's abandoned. Editable from the page. | | `maxPrepared` | How many prepared reviews may wait for you at once (default 5). Each preparation is an agent run; the rest of the queue waits until a prepared review is posted or dismissed. Editable from the page. | | `live` | Whether opening a prepared review also parks a live agent on it (default true). Questions asked in the page — the Ask button on a finding — each run the agent once to answer; the agent may answer and amend findings, never edit code, and never reaches GitHub. Editable from the page. | @@ -367,6 +369,8 @@ On first run it writes `~/.diffity/inbox/config.json`: The command itself is not configurable: the daemon builds `claude -p --output-format json` with the flags the review depends on. It runs with `--setting-sources ""`, so the agent gets none of your Claude settings — no MCP servers, no memory, no `CLAUDE.md`, none of your installed skills. Listing tools in `agent.mcpAllow` brings your MCP servers back and adds a `PreToolUse` hook (`diffity inbox mcp-gate`) that refuses every MCP call but those, by name; the prompt then tells the agent it may read the ticket or document the pull request refers to, and nothing else outside the checkout. A deny list keeps it off `gh pr review`, `gh pr comment`, `gh pr merge` and `gh api`, and off `pnpm`, `npm`, `npx`, `yarn`, `bun` and `make` — CI has already built and tested this head. The skill shipped with this build goes into the agent's system prompt — `diffity-review` for a preparation, `diffity-live` for an answer — so neither depends on what you have installed. +What CI made of the head goes into the prompt: every check that reported, with its verdict, a count of the ones a workflow skipped, and the instruction not to install, build, typecheck, lint or test — CI has done that, and the checkout is the author's code — so the agent reasons from the source and says in its summary when a check failed or is still running. Each card carries the same verdict as a dot before its title: green for passing, red for failing, amber while checks are still going, and no dot at all when nothing has decided — no checks, or only skipped and cancelled ones. With `waitForCi` on, a pull request whose checks are still running waits in the queue ("waiting: CI running (3 checks)") and one whose checks failed is set aside ("CI failed: check-job, pr-mgmt-job") — both re-decided at every poll, so passing checks bring one back to the queue on their own, unlike a filter skip. ↑ prepares it whatever CI says, and a review already prepared for an older head stays openable while its refresh waits. + Every agent run is logged: the pull request and head it was for, which pass it was (`prepare` for a preparation, `answer` for a question asked in the page), the models it actually used, how long it took, its turns, its cost and its tokens, and how it ended (`prepared`, `skipped`, `answered`, `failed`, `timeout`, `rate-limited`). `diffity inbox runs` prints that log with totals — the record of what the inbox costs you. A prepared review's card carries its own share of it, "· 8 min · $1.20", with each run behind that head listed on hover, and the page's footer keeps a running total for today and for the last seven days. A run that ends on your Claude session limit is not the pull request's fault, so it is waited out rather than retried: the row goes back in the queue as "waiting: Claude session limit until 14:00", no failed attempt is counted against it, and no further preparation starts until the limit lifts. The reset time is read out of the agent's own message ("resets 2pm (Europe/Stockholm)"), or set half an hour ahead when the message names none. Polling and reconciling carry on meanwhile, so the page stays current and says how long the pause has left; the pause is kept with the inbox, so restarting the daemon does not spend another run rediscovering the limit. diff --git a/package-lock.json b/package-lock.json index c88db160..1e84d07b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.23", + "version": "0.10.24", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.23", + "version": "0.10.24", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.23", + "version": "0.10.24", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.23", + "version": "0.10.24", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.23", + "version": "0.10.24", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.23", + "version": "0.10.24", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/packages/api/package.json b/packages/api/package.json index 58d95b06..731b50bc 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.23", + "version": "0.10.24", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index cf01ff0e..0e250306 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.23", + "version": "0.10.24", "description": "Agent-agnostic, GitHub-style diff viewer and code review tool with a live agent loop", "type": "module", "bin": { diff --git a/packages/cli/src/inbox/config.ts b/packages/cli/src/inbox/config.ts index c1d2ebac..aa8a31dd 100644 --- a/packages/cli/src/inbox/config.ts +++ b/packages/cli/src/inbox/config.ts @@ -43,7 +43,14 @@ export interface InboxConfig { * empty means every prepared review is worth a notification. */ alertWhen: string; + /** + * Globs against the pull request's changed paths. A pull request touching one of them is marked + * as needing the reviewer now, alongside whatever the agent made of `alertWhen`. + */ + alertPaths: string[]; agent: AgentConfig; + /** Whether a pull request waits for its CI to pass before an agent is spent on it. */ + waitForCi: boolean; prepareTimeoutMinutes: number; /** * How many prepared reviews may wait for the reviewer at once. Each preparation spends an agent @@ -66,7 +73,9 @@ export const DEFAULT_INBOX_CONFIG: InboxConfig = { worktreesDir: '~/.diffity/inbox/worktrees', filter: '', alertWhen: '', + alertPaths: [], agent: { model: null, effort: null, mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, + waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, @@ -98,7 +107,7 @@ export function parseInboxConfig(raw: unknown, source = 'inbox config'): InboxCo throw new Error(`${source} must be a JSON object`); } const obj = raw as Record; - const config: InboxConfig = { ...DEFAULT_INBOX_CONFIG, agent: defaultAgent() }; + const config: InboxConfig = { ...DEFAULT_INBOX_CONFIG, alertPaths: [], agent: defaultAgent() }; if (obj.prepare !== undefined) { throw new Error(`${source}: "prepare" was replaced by the "agent" block — delete it (the built-in command applies) and put extra flags in agent.extraArgs`); @@ -127,9 +136,21 @@ export function parseInboxConfig(raw: unknown, source = 'inbox config'): InboxCo } config.alertWhen = obj.alertWhen; } + if (obj.alertPaths !== undefined) { + if (!Array.isArray(obj.alertPaths) || !obj.alertPaths.every(glob => typeof glob === 'string' && glob.trim() !== '')) { + throw new Error(`${source}: alertPaths must be an array of non-empty globs`); + } + config.alertPaths = (obj.alertPaths as string[]).map(glob => glob.trim()); + } if (obj.agent !== undefined) { config.agent = parseAgentConfig(obj.agent, source); } + if (obj.waitForCi !== undefined) { + if (typeof obj.waitForCi !== 'boolean') { + throw new Error(`${source}: waitForCi must be true or false`); + } + config.waitForCi = obj.waitForCi; + } if (obj.prepareTimeoutMinutes !== undefined) { config.prepareTimeoutMinutes = positive(obj.prepareTimeoutMinutes, 'prepareTimeoutMinutes', source); } @@ -193,7 +214,7 @@ function parseAgentConfig(raw: unknown, source: string): AgentConfig { } /** The settings the inbox page edits, kept in the config file beside the keys only the file holds. */ -export type InboxSettings = Pick; +export type InboxSettings = Pick; /** * Writes the page-editable settings into the config file, leaving every other key as the reviewer diff --git a/packages/cli/src/inbox/daemon.ts b/packages/cli/src/inbox/daemon.ts index 832fe37a..22522a29 100644 --- a/packages/cli/src/inbox/daemon.ts +++ b/packages/cli/src/inbox/daemon.ts @@ -75,9 +75,10 @@ export interface SettingsHost { export function settingsHost(config: InboxConfig, configPath: string | undefined, onPollChanged: () => void = () => {}): SettingsHost { return { get: () => ({ - filter: config.filter, alertWhen: config.alertWhen, maxPrepared: config.maxPrepared, pollMinutes: config.pollMinutes, + filter: config.filter, alertWhen: config.alertWhen, alertPaths: config.alertPaths, + maxPrepared: config.maxPrepared, pollMinutes: config.pollMinutes, live: config.live, liveTimeoutMinutes: config.liveTimeoutMinutes, prepareTimeoutMinutes: config.prepareTimeoutMinutes, - agent: config.agent, + waitForCi: config.waitForCi, agent: config.agent, }), update: settings => { // The config object is the one the tick, the prepares and the opens read from, so the change @@ -131,6 +132,8 @@ export async function runDaemon( shouldContinue: () => !stopping, // Read at each tick, not copied: the page can change it while the daemon runs. get maxPrepared() { return config.maxPrepared; }, + get waitForCi() { return config.waitForCi; }, + get alertPaths() { return config.alertPaths; }, get agentModel() { return config.agent.model; }, pauseUntil: (until: string) => { store.pauseUntil(until); diff --git a/packages/cli/src/inbox/page.ts b/packages/cli/src/inbox/page.ts index cb4138e8..ac633d56 100644 --- a/packages/cli/src/inbox/page.ts +++ b/packages/cli/src/inbox/page.ts @@ -79,6 +79,10 @@ export function inboxPage(): string { .open-hint { color: var(--accent); font-size: 12px; font-weight: 600; white-space: nowrap; } .empty { color: var(--muted); padding: 12px 2px; } .dot { width: 7px; height: 7px; border-radius: 50%; background: var(--ready); flex: none; } + .ci { flex: none; font-size: 9px; line-height: 1; } + .ci-passing { color: var(--ready); } + .ci-failing { color: var(--bad); } + .ci-running { color: var(--stale); } .foot { color: var(--muted); font-size: 11.5px; margin-top: 22px; } a { color: inherit; text-decoration: none; } @@ -119,6 +123,9 @@ export function inboxPage(): string { + @@ -138,6 +145,7 @@ export function inboxPage(): string { +
@@ -161,6 +169,14 @@ export function inboxPage(): string { return Math.round(hours / 24) + ' d ago'; } + /** What CI made of the head, as one glyph; nothing when nothing has reported. */ + function ciDot(r) { + const labels = { passing: 'CI passing', failing: 'CI failing', running: 'CI running' }; + return labels[r.ciState] + ? '' + : ''; + } + function metaLine(parts, hover) { const text = parts.filter(Boolean).join(' \\u00b7 '); // The line is cut to the card; the full text, wrapped, is a hover away. @@ -203,6 +219,7 @@ export function inboxPage(): string { row.innerHTML = '' + '' + sizeLabel(r) + '' + + ciDot(r) + '
' + esc(r.repo) + '#' + r.number + ' ' + '' + esc(r.title) + '
' + metaLine(['by ' + esc(r.author), r.changedFiles + ' file(s)', esc(r.summary || ''), spendLabel(r), times(r)], @@ -218,6 +235,7 @@ export function inboxPage(): string { row.className = 'row'; row.innerHTML = '' + sizeLabel(r) + '' + + ciDot(r) + '
' + esc(r.repo) + '#' + r.number + ' ' + '' + esc(r.title) + '
' + metaLine([esc(r.statusReason || ''), times(r)], r.statusReason || '') + '
' + @@ -338,8 +356,10 @@ export function inboxPage(): string { settings = await res.json(); el('filter').value = settings.filter; el('alertWhen').value = settings.alertWhen; + el('alertPaths').value = (settings.alertPaths || []).join('\\n'); for (const key of ['maxPrepared', 'pollMinutes', 'prepareTimeoutMinutes', 'liveTimeoutMinutes']) el(key).value = settings[key]; el('live').checked = settings.live; + el('waitForCi').checked = settings.waitForCi; const agent = settings.agent || {}; el('agentModel').value = agent.model || ''; el('agentEffort').value = agent.effort || ''; @@ -355,11 +375,13 @@ export function inboxPage(): string { const next = { filter: el('filter').value, alertWhen: el('alertWhen').value, + alertPaths: el('alertPaths').value.split('\\n').map(line => line.trim()).filter(Boolean), maxPrepared: Number(el('maxPrepared').value), pollMinutes: Number(el('pollMinutes').value), prepareTimeoutMinutes: Number(el('prepareTimeoutMinutes').value), liveTimeoutMinutes: Number(el('liveTimeoutMinutes').value), live: el('live').checked, + waitForCi: el('waitForCi').checked, agent: { model: el('agentModel').value.trim() || null, effort: el('agentEffort').value || null, diff --git a/packages/cli/src/inbox/paths-alert.ts b/packages/cli/src/inbox/paths-alert.ts new file mode 100644 index 00000000..cb50c871 --- /dev/null +++ b/packages/cli/src/inbox/paths-alert.ts @@ -0,0 +1,52 @@ +/** + * The reviewer's own paths: a pull request touching one of them needs them now, whatever the agent + * made of the review. Where `alertWhen` asks an agent to judge, this is a fact about the diff and + * cannot be misjudged. + */ +export function alertForPaths(files: { path: string }[], globs: string[]): string | null { + if (globs.length === 0 || files.length === 0) { + return null; + } + const patterns = globs.map(globToRegExp); + const matched = files.filter(file => patterns.some(pattern => pattern.test(file.path))); + if (matched.length === 0) { + return null; + } + return `touches ${matched[0].path}${matched.length > 1 ? ` and ${matched.length - 1} more` : ''}`; +} + +/** + * The glob dialect a reviewer would expect of a path list: `**` spans directories, `*` and `?` stay + * within one segment, everything else is literal. `node:path`'s own `matchesGlob` would do this, + * but it still warns as experimental on the Node versions this runs on. + */ +function globToRegExp(glob: string): RegExp { + let source = ''; + for (let i = 0; i < glob.length; i++) { + const char = glob[i]; + if (char === '*' && glob[i + 1] === '*') { + i++; + if (glob[i + 1] === '/') { + // Any number of directories, the pattern's own next segment included at any depth. + i++; + source += '(?:[^/]*/)*'; + } else if (source.endsWith('/')) { + // A trailing `dir/**`: the directory itself and everything under it. + source = `${source.slice(0, -1)}(?:/.*)?`; + } else { + source += '.*'; + } + continue; + } + if (char === '*') { + source += '[^/]*'; + continue; + } + if (char === '?') { + source += '[^/]'; + continue; + } + source += /[.+^${}()|[\]\\]/.test(char) ? `\\${char}` : char; + } + return new RegExp(`^${source}$`); +} diff --git a/packages/cli/src/inbox/prompt.ts b/packages/cli/src/inbox/prompt.ts index 91eb221d..7f65c1c0 100644 --- a/packages/cli/src/inbox/prompt.ts +++ b/packages/cli/src/inbox/prompt.ts @@ -1,4 +1,4 @@ -import type { PrSnapshot } from '@diffity/github'; +import type { PrCheck, PrSnapshot } from '@diffity/github'; export interface PromptContext { snapshot: PrSnapshot; @@ -29,6 +29,10 @@ export function composePrompt(ctx: PromptContext): string { ` Author: ${oneLine(snapshot.author)}`, ` Repository: ${snapshot.owner}/${snapshot.repo}, base ${oneLine(snapshot.baseRef)}`, `Size: +${snapshot.additions} -${snapshot.deletions} across ${snapshot.changedFiles} file(s)`, + ciLine(snapshot.checks), + 'Do not install dependencies, build, typecheck, lint or run tests: CI has done that, and this', + 'checkout is the author\'s code. Reason from the source. If a check failed or is still running,', + 'say so in the summary.', '', 'A diffity review session for this pull request is already running. The checkout is at:', ` ${worktreePath}`, @@ -93,6 +97,35 @@ export function composePrompt(ctx: PromptContext): string { return lines.join('\n') + '\n'; } +/** + * What CI made of this head, as one line: every check that ran with its verdict, and a count for + * the ones a workflow condition skipped — a repository can skip dozens, and their names say + * nothing the review needs. + */ +function ciLine(checks: PrCheck[]): string { + if (checks.length === 0) { + return 'CI has not reported for this head.'; + } + // Check names are the repository's text — a workflow's own expression, sometimes — so they are + // held to one line and a length, like the title and the author above. + const reported = checks.filter(check => check.status !== 'skipped') + .map(check => `${checkName(check.name)} ${check.status.toUpperCase()}`); + const skipped = checks.length - reported.length; + if (reported.length === 0) { + return `CI at this head: ${skipped} checks skipped, none ran`; + } + if (skipped > 0) { + reported.push(`${skipped} more skipped`); + } + return `CI at this head: ${reported.join(' \u00b7 ')}`; +} + +const MAX_CHECK_NAME = 80; + +function checkName(name: string): string { + return oneLine(name).slice(0, MAX_CHECK_NAME); +} + /** * What the agent's run amounted to, read from the last verdict line it printed; a prepared review * carries the alert the agent raised on the way, if any. diff --git a/packages/cli/src/inbox/reconcile.ts b/packages/cli/src/inbox/reconcile.ts index a463db2e..022f027b 100644 --- a/packages/cli/src/inbox/reconcile.ts +++ b/packages/cli/src/inbox/reconcile.ts @@ -1,4 +1,4 @@ -import type { PrSnapshot } from '@diffity/github'; +import { ciState, type PrCheck, type PrSnapshot } from '@diffity/github'; import type { InboxPr, InboxStatus } from './store.js'; /** How many times preparation is retried at one head before the pull request is left as failed. */ @@ -20,8 +20,16 @@ export interface ReconcileInput { /** Whether this poll's search listed the PR as awaiting the reviewer. */ requested: boolean; viewerLogin: string | null; + /** + * Whether a pull request waits for its CI before an agent is spent on it; absent is off, as the + * config's default is. + */ + waitForCi?: boolean; } +/** The mark of a CI hold on a row, so the next poll re-decides it instead of leaving it settled. */ +const CI_FAILED = 'CI failed:'; + /** * The status a pull request should move to, given what the forge now says and what the inbox * already did — the whole decision in one pure function, so every branch is a plain test. @@ -33,6 +41,48 @@ export interface ReconcileInput { * apart. Everything else asked of the reviewer is queued. */ export function reconcile(input: ReconcileInput): Transition | null { + const transition = decide(input); + return transition && input.waitForCi ? heldForCi(transition, input) : transition; +} + +/** + * What CI does to a transition that was about to spend an agent: a run still going is waited for, + * a failure is left to the author. A pull request the reviewer bumped is prepared regardless — the + * ↑ overrides this as it overrides the filter — and a review already prepared for an older head + * stays openable while its refresh waits. + */ +function heldForCi(transition: Transition, input: ReconcileInput): Transition { + const { existing, snapshot } = input; + if (!transition.prepare || !snapshot || existing?.bumpedAt) { + return transition; + } + const refresh = existing?.status === 'prepared' || existing?.status === 'stale'; + switch (ciState(snapshot.checks)) { + case 'running': { + const running = snapshot.checks.filter(check => check.status === 'pending').length; + return { status: refresh ? 'stale' : 'queued', reason: `waiting: CI running (${running} checks)`, prepare: false }; + } + case 'failing': + return { status: refresh ? 'stale' : 'skipped', reason: ciFailedReason(snapshot.checks), prepare: false }; + default: + // Nothing has reported for this head, or everything that did passed: review it. + return transition; + } +} + +/** Which checks failed, by name — three of them, and a count for the rest. */ +function ciFailedReason(checks: PrCheck[]): string { + const failed = checks.filter(check => check.status === 'failure').map(check => oneLine(check.name)); + const named = failed.slice(0, 3).join(', '); + return `${CI_FAILED} ${failed.length > 3 ? `${named} and ${failed.length - 3} more` : named}`; +} + +/** A check name is the repository's own text; a newline in it would break the row it explains. */ +function oneLine(text: string): string { + return text.replace(/\s+/g, ' ').trim(); +} + +function decide(input: ReconcileInput): Transition | null { const { existing, snapshot, requested, viewerLogin } = input; // Listed by search but the detail view failed this tick: keep the row as it is and try next time. @@ -77,8 +127,10 @@ export function reconcile(input: ReconcileInput): Transition | null { } // A settled skip stays settled until its head moves; re-running the filter on every poll would - // just spend the same tokens on the same answer. - if (existing && existing.status === 'skipped' && existing.headSha === snapshot.headSha) { + // just spend the same tokens on the same answer. A CI failure is not the reviewer's verdict on + // the pull request, though: it is re-decided every poll, and green checks put it back in the queue. + if (existing && existing.status === 'skipped' && existing.headSha === snapshot.headSha + && !existing.statusReason?.startsWith(CI_FAILED)) { return null; } diff --git a/packages/cli/src/inbox/settings.ts b/packages/cli/src/inbox/settings.ts index c42de544..524a15e9 100644 --- a/packages/cli/src/inbox/settings.ts +++ b/packages/cli/src/inbox/settings.ts @@ -3,7 +3,7 @@ import { parseInboxConfig, type InboxSettings } from './config.js'; /** Longer than any sensible instruction to an agent; a guard on the request, not a design limit. */ export const MAX_SETTINGS_TEXT = 4000; -export const SETTINGS_KEYS = ['filter', 'alertWhen', 'maxPrepared', 'pollMinutes', 'live', 'liveTimeoutMinutes', 'prepareTimeoutMinutes', 'agent'] as const; +export const SETTINGS_KEYS = ['filter', 'alertWhen', 'alertPaths', 'maxPrepared', 'pollMinutes', 'live', 'liveTimeoutMinutes', 'prepareTimeoutMinutes', 'waitForCi', 'agent'] as const; export type SettingsPatch = | { ok: true; settings: InboxSettings } diff --git a/packages/cli/src/inbox/store.ts b/packages/cli/src/inbox/store.ts index 2d00a214..e138c6ff 100644 --- a/packages/cli/src/inbox/store.ts +++ b/packages/cli/src/inbox/store.ts @@ -1,7 +1,7 @@ import { DatabaseSync } from 'node:sqlite'; import { mkdirSync } from 'node:fs'; import { dirname } from 'node:path'; -import type { PrSnapshot } from '@diffity/github'; +import { ciState, type CiState, type PrSnapshot } from '@diffity/github'; import type { RunStats } from './agent-output.js'; export const INBOX_STATUSES = [ @@ -34,6 +34,8 @@ export interface InboxPr { additions: number; deletions: number; changedFiles: number; + /** What CI said about the current head at the last poll; null on a row from before it was kept. */ + ciState: CiState | null; /** The forge's own timestamps for the pull request; null on a row from before they were kept. */ createdAt: string | null; updatedAt: string | null; @@ -207,7 +209,8 @@ export class InboxStore { updated_at TEXT, bumped_at TEXT, summary TEXT, - alert TEXT + alert TEXT, + ci_state TEXT ) `); this.db.exec(` @@ -233,7 +236,7 @@ export class InboxStore { this.db.exec('CREATE INDEX IF NOT EXISTS inbox_runs_pr_started ON inbox_runs (pr_id, started_at)'); this.db.exec('CREATE TABLE IF NOT EXISTS inbox_state (key TEXT PRIMARY KEY, value TEXT)'); // A table from an earlier build gains the columns it lacks; a fresh one already has them. - for (const column of ['attempts INTEGER NOT NULL DEFAULT 0', 'created_at TEXT', 'updated_at TEXT', 'bumped_at TEXT', 'summary TEXT', 'alert TEXT']) { + for (const column of ['attempts INTEGER NOT NULL DEFAULT 0', 'created_at TEXT', 'updated_at TEXT', 'bumped_at TEXT', 'summary TEXT', 'alert TEXT', 'ci_state TEXT']) { try { this.db.exec(`ALTER TABLE inbox_prs ADD COLUMN ${column}`); } catch (err) { @@ -268,8 +271,8 @@ export class InboxStore { INSERT INTO inbox_prs ( id, owner, repo, number, title, url, author, is_draft, head_sha, base_ref, additions, deletions, changed_files, requested, status, status_reason, first_seen_at, last_seen_at, - created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 'queued', NULL, ?, ?, ?, ?) + created_at, updated_at, ci_state + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 'queued', NULL, ?, ?, ?, ?, ?) ON CONFLICT(id) DO UPDATE SET title = excluded.title, url = excluded.url, @@ -285,12 +288,13 @@ export class InboxStore { requested = excluded.requested, last_seen_at = excluded.last_seen_at, created_at = excluded.created_at, - updated_at = excluded.updated_at + updated_at = excluded.updated_at, + ci_state = excluded.ci_state `).run( id, snapshot.owner, snapshot.repo, snapshot.number, snapshot.title, snapshot.url, snapshot.author, snapshot.isDraft ? 1 : 0, snapshot.headSha, snapshot.baseRef, snapshot.additions, snapshot.deletions, snapshot.changedFiles, requested ? 1 : 0, now, now, - snapshot.createdAt || null, snapshot.updatedAt || null, + snapshot.createdAt || null, snapshot.updatedAt || null, ciState(snapshot.checks), ); return this.get(id)!; } @@ -423,6 +427,7 @@ interface Row { bumped_at: string | null; summary: string | null; alert: string | null; + ci_state: string | null; } function rowToPr(row: Row): InboxPr { @@ -440,6 +445,7 @@ function rowToPr(row: Row): InboxPr { additions: row.additions, deletions: row.deletions, changedFiles: row.changed_files, + ciState: normaliseCiState(row.ci_state), createdAt: row.created_at, updatedAt: row.updated_at, requested: row.requested === 1, @@ -503,6 +509,12 @@ function round1(value: number): number { return Math.round(value * 10) / 10; } +const CI_STATES: readonly string[] = ['passing', 'failing', 'running', 'none']; + +function normaliseCiState(value: string | null): CiState | null { + return value !== null && CI_STATES.includes(value) ? (value as CiState) : null; +} + /** A row from a build that knew other statuses is shown as needing work rather than crashing the list. */ function normaliseStatus(value: string): InboxStatus { return (INBOX_STATUSES as readonly string[]).includes(value) ? (value as InboxStatus) : 'queued'; diff --git a/packages/cli/src/inbox/tick.ts b/packages/cli/src/inbox/tick.ts index 9e2049d3..bb1cf4ec 100644 --- a/packages/cli/src/inbox/tick.ts +++ b/packages/cli/src/inbox/tick.ts @@ -1,4 +1,5 @@ import type { PrRef, PrSnapshot } from '@diffity/github'; +import { alertForPaths } from './paths-alert.js'; import { reconcile } from './reconcile.js'; import { isRetired, prId, runRecordOf, type InboxPr, type InboxStore, type RunOutcome } from './store.js'; import { localHhMm } from './runs.js'; @@ -22,6 +23,10 @@ export interface TickDeps { shouldContinue?(): boolean; /** How many prepared reviews may wait for the reviewer at once; the rest of the queue waits. */ maxPrepared: number; + /** Whether a pull request waits for its CI to pass before an agent is spent on it. */ + waitForCi: boolean; + /** The changed paths that make a review worth the reviewer's attention now. */ + alertPaths: string[]; /** `agent.model`, recorded for a run that did not report which models it spent on. */ agentModel: string | null; /** Holds preparation back until then — a session limit is waited out, not retried. */ @@ -50,7 +55,7 @@ export async function runTick(store: InboxStore, deps: TickDeps): Promise } const existing = store.get(prId(ref)); const pr = store.observe(snapshot, true, deps.now()); - const transition = reconcile({ existing, snapshot, requested: true, viewerLogin }); + const transition = reconcile({ existing, snapshot, requested: true, viewerLogin, waitForCi: deps.waitForCi }); if (transition) { store.setStatus(pr.id, transition.status, transition.reason); if (transition.prepare) { @@ -158,7 +163,8 @@ async function prepareOne(store: InboxStore, snapshot: PrSnapshot, deps: TickDep logPath: result.logPath, at: result.at, summary: result.summary, - alert: result.alert, + // The agent's judgement first; the reviewer's own paths stand in when it raised nothing. + alert: result.alert ?? alertForPaths(snapshot.files, deps.alertPaths), }); deps.log(`prepared ${id}`); return; diff --git a/packages/cli/src/inbox/view.ts b/packages/cli/src/inbox/view.ts index 287e97ae..9328cb82 100644 --- a/packages/cli/src/inbox/view.ts +++ b/packages/cli/src/inbox/view.ts @@ -1,3 +1,4 @@ +import type { CiState } from '@diffity/github'; import { isRetired, type InboxPr, type InboxStore, type RunTotals } from './store.js'; import { costOf, minutesOf, runDetail } from './runs.js'; import { BUMPABLE } from './open.js'; @@ -24,6 +25,8 @@ export interface InboxRow { changedFiles: number; additions: number; deletions: number; + /** What CI said about the current head, for the card's dot; null when nothing is known. */ + ciState: CiState | null; createdAt: string | null; updatedAt: string | null; /** A prepared review whose head has since moved: openable, but out of date. */ @@ -105,6 +108,7 @@ function toRow(pr: InboxPr, openBase: string, store: InboxStore): InboxRow { changedFiles: pr.changedFiles, additions: pr.additions, deletions: pr.deletions, + ciState: pr.ciState, createdAt: pr.createdAt, updatedAt: pr.updatedAt, stale, diff --git a/packages/cli/tests/inbox-daemon.test.ts b/packages/cli/tests/inbox-daemon.test.ts index 39351112..202cbbbc 100644 --- a/packages/cli/tests/inbox-daemon.test.ts +++ b/packages/cli/tests/inbox-daemon.test.ts @@ -44,7 +44,7 @@ function seedRegistry(pid: number): void { function config(port: number) { return { pollMinutes: 5, port, reposDir: join(root, 'repos'), worktreesDir: join(root, 'inbox', 'worktrees'), - filter: '', alertWhen: '', agent: agentConfig(), prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, + filter: '', alertWhen: '', alertPaths: [], agent: agentConfig(), waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, }; } @@ -118,7 +118,7 @@ describe('runDaemon singleton and reclaim ordering', () => { const snap: PrSnapshot = { owner: 'o', repo: 'r', number: 1, title: 'T', url: 'https://github.com/o/r/pull/1', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'aaa', baseRef: 'main', additions: 1, deletions: 0, changedFiles: 1, - createdAt: 'now', updatedAt: 'now', + createdAt: 'now', updatedAt: 'now', checks: [], files: [], }; store.observe(snap, true, 'now'); let searches = 0; @@ -157,7 +157,7 @@ describe('runDaemon singleton and reclaim ordering', () => { const snap: PrSnapshot = { owner: 'o', repo: 'r', number: 1, title: 'T', url: 'https://github.com/o/r/pull/1', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'aaa', baseRef: 'main', additions: 1, deletions: 0, changedFiles: 1, - createdAt: 'now', updatedAt: 'now', + createdAt: 'now', updatedAt: 'now', checks: [], files: [], }; store.observe(snap, true, 'now'); store.pauseUntil(new Date(Date.now() + 60_000).toISOString()); diff --git a/packages/cli/tests/inbox-open.test.ts b/packages/cli/tests/inbox-open.test.ts index 7b645c62..a0da9f96 100644 --- a/packages/cli/tests/inbox-open.test.ts +++ b/packages/cli/tests/inbox-open.test.ts @@ -27,7 +27,7 @@ function snapshot(): PrSnapshot { return { owner: 'o', repo: 'r', number: 4, title: 'A change', url: 'https://github.com/o/r/pull/4', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'aaa', baseRef: 'main', - additions: 3, deletions: 1, changedFiles: 2, createdAt: 'now', updatedAt: 'now', + additions: 3, deletions: 1, changedFiles: 2, createdAt: 'now', updatedAt: 'now', checks: [], files: [], }; } @@ -222,7 +222,7 @@ describe('the inbox server routes', () => { }; async function serve(store: InboxStore, logs: string[] = [], attendants: AttendantHost | null = null, onBump: (() => void) | null = null, configPath?: string, extra: Partial = {}) { - const config = { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', alertWhen: '', agent: agentConfig(), prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10 }; + const config = { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', alertWhen: '', alertPaths: [], agent: agentConfig(), waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10 }; const server = startInboxServer(store, config, m => logs.push(m), stubOpen, { attendants, onBump, settings: settingsHost(config, configPath), ...extra }); await new Promise(resolve => server.on('listening', resolve)); const { port } = server.address() as { port: number }; @@ -354,10 +354,14 @@ describe('the inbox server routes', () => { const { port, server } = await serve(store, [], null, null, configPath); try { const before = await (await fetch(`http://127.0.0.1:${port}/api/settings`)).json(); - expect(before).toEqual({ filter: '', alertWhen: '', maxPrepared: 5, pollMinutes: 5, live: true, liveTimeoutMinutes: 10, prepareTimeoutMinutes: 30, agent: agentConfig() }); + expect(before).toEqual({ + filter: '', alertWhen: '', alertPaths: [], maxPrepared: 5, pollMinutes: 5, live: true, + liveTimeoutMinutes: 10, prepareTimeoutMinutes: 30, waitForCi: false, agent: agentConfig(), + }); const next = { - filter: 'skip payments', alertWhen: 'a P1', maxPrepared: 2, pollMinutes: 3, live: false, liveTimeoutMinutes: 4, prepareTimeoutMinutes: 20, + filter: 'skip payments', alertWhen: 'a P1', alertPaths: ['packages/shared/src/model/**'], + maxPrepared: 2, pollMinutes: 3, live: false, liveTimeoutMinutes: 4, prepareTimeoutMinutes: 20, waitForCi: true, agent: { ...agentConfig(), model: 'opus', mcpAllow: ['mcp__atlassian__getJiraIssue'] }, }; const saved = await fetch(`http://127.0.0.1:${port}/api/settings`, { @@ -453,7 +457,7 @@ describe('the inbox server routes', () => { ensureServer: () => Promise.resolve(7788), importBundle: () => { throw new Error('head moved'); }, }; - const config = { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', alertWhen: '', agent: agentConfig(), prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10 }; + const config = { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', alertWhen: '', alertPaths: [], agent: agentConfig(), waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10 }; const server = startInboxServer(store, config, m => logs.push(m), failingOpen); await new Promise(resolve => server.on('listening', resolve)); const { port } = server.address() as { port: number }; diff --git a/packages/cli/tests/inbox-page.test.ts b/packages/cli/tests/inbox-page.test.ts index b55bd9bf..d409c05d 100644 --- a/packages/cli/tests/inbox-page.test.ts +++ b/packages/cli/tests/inbox-page.test.ts @@ -40,6 +40,28 @@ describe('the inbox page', () => { expect(inboxPage()).toContain('.title .meta { color: var(--muted); font-size: 12px; min-width: 0;'); }); + it('marks each card with what CI said, and keeps the card\'s width its own', () => { + const script = pageScript(); + expect(script).toContain("' { + const html = inboxPage(); + expect(html).toContain('id="waitForCi" type="checkbox"'); + expect(html).toContain('Hold a pull request until its CI has passed'); + expect(html).toContain('id="alertPaths"'); + expect(html).toContain('Alert me if a changed file matches (one glob per line)'); + const script = pageScript(); + expect(script).toContain("alertPaths: el('alertPaths').value.split('\\n')"); + expect(script).toContain("waitForCi: el('waitForCi').checked"); + }); + it('sends the agent block back with the settings, empty fields as null', () => { const script = pageScript(); expect(script).toContain("model: el('agentModel').value.trim() || null"); diff --git a/packages/cli/tests/inbox-paths-alert.test.ts b/packages/cli/tests/inbox-paths-alert.test.ts new file mode 100644 index 00000000..0261172f --- /dev/null +++ b/packages/cli/tests/inbox-paths-alert.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect } from 'vitest'; +import { alertForPaths } from '../src/inbox/paths-alert.js'; + +function files(...paths: string[]): { path: string }[] { + return paths.map(path => ({ path })); +} + +describe('alertForPaths', () => { + it('names the first changed file that matches, and counts the rest', () => { + expect(alertForPaths(files('README.md', 'packages/shared/src/model/x.ts'), ['packages/shared/src/model/**'])) + .toBe('touches packages/shared/src/model/x.ts'); + expect(alertForPaths( + files('packages/shared/src/model/x.ts', 'README.md', 'packages/shared/src/model/y.ts'), + ['packages/shared/src/model/**'], + )).toBe('touches packages/shared/src/model/x.ts and 1 more'); + }); + + it('spans directories with **, stays in one segment with * and ?', () => { + expect(alertForPaths(files('src/dbref/a.ts'), ['**/dbref/**'])).toBe('touches src/dbref/a.ts'); + // A leading **/ matches no directory at all as readily as several. + expect(alertForPaths(files('dbref/a.ts'), ['**/dbref/**'])).toBe('touches dbref/a.ts'); + expect(alertForPaths(files('a/b/c/dbref/deep/a.ts'), ['**/dbref/**'])).toBe('touches a/b/c/dbref/deep/a.ts'); + // The directory itself, with nothing under it named. + expect(alertForPaths(files('packages/shared/src/model'), ['packages/shared/src/model/**'])) + .toBe('touches packages/shared/src/model'); + expect(alertForPaths(files('src/nested/a.ts'), ['src/*.ts'])).toBeNull(); + expect(alertForPaths(files('src/a.ts'), ['src/*.ts'])).toBe('touches src/a.ts'); + expect(alertForPaths(files('src/a.tsx'), ['src/a.ts?'])).toBe('touches src/a.tsx'); + }); + + it('is null with nothing to match, nothing to match against, or no match', () => { + expect(alertForPaths(files('src/a.ts'), [])).toBeNull(); + expect(alertForPaths([], ['src/**'])).toBeNull(); + expect(alertForPaths(files('README.md'), ['src/**', '**/dbref/**'])).toBeNull(); + }); + + it('treats every other character in a glob as the literal it is', () => { + expect(alertForPaths(files('src/a.ts'), ['src/a+ts'])).toBeNull(); + expect(alertForPaths(files('src/a+ts'), ['src/a+ts'])).toBe('touches src/a+ts'); + }); +}); diff --git a/packages/cli/tests/inbox-prepare.test.ts b/packages/cli/tests/inbox-prepare.test.ts index 25e0bcd7..d6bd50bd 100644 --- a/packages/cli/tests/inbox-prepare.test.ts +++ b/packages/cli/tests/inbox-prepare.test.ts @@ -29,14 +29,14 @@ function snapshot(): PrSnapshot { return { owner: 'o', repo: 'demo', number: 4, title: 'A change', url: 'https://github.com/o/demo/pull/4', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: head, baseRef: 'main', - additions: 1, deletions: 0, changedFiles: 1, createdAt: 'now', updatedAt: 'now', + additions: 1, deletions: 0, changedFiles: 1, createdAt: 'now', updatedAt: 'now', checks: [], files: [], }; } function config(): InboxConfig { return { - pollMinutes: 5, port: 0, reposDir, worktreesDir, filter: '', alertWhen: '', - agent: agentConfig(), prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, + pollMinutes: 5, port: 0, reposDir, worktreesDir, filter: '', alertWhen: '', alertPaths: [], + agent: agentConfig(), waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, }; } diff --git a/packages/cli/tests/inbox-reconcile.test.ts b/packages/cli/tests/inbox-reconcile.test.ts index 57432307..8567dde1 100644 --- a/packages/cli/tests/inbox-reconcile.test.ts +++ b/packages/cli/tests/inbox-reconcile.test.ts @@ -1,13 +1,14 @@ import { describe, it, expect } from 'vitest'; import { reconcile } from '../src/inbox/reconcile.js'; import type { InboxPr } from '../src/inbox/store.js'; -import type { PrSnapshot } from '@diffity/github'; +import type { PrCheck, PrSnapshot } from '@diffity/github'; function snapshot(over: Partial = {}): PrSnapshot { return { owner: 'o', repo: 'r', number: 1, title: 'A change', url: 'https://github.com/o/r/pull/1', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'aaa', baseRef: 'main', - additions: 10, deletions: 2, changedFiles: 3, createdAt: '2026-09-02T10:00:00Z', updatedAt: '2026-09-02T10:00:00Z', ...over, + additions: 10, deletions: 2, changedFiles: 3, createdAt: '2026-09-02T10:00:00Z', updatedAt: '2026-09-02T10:00:00Z', + checks: [], files: [], ...over, }; } @@ -15,11 +16,15 @@ function existing(over: Partial = {}): InboxPr { return { id: 'o/r#1', owner: 'o', repo: 'r', number: 1, title: 'A change', url: 'https://github.com/o/r/pull/1', author: 'alice', isDraft: false, headSha: 'aaa', baseRef: 'main', additions: 10, deletions: 2, changedFiles: 3, - createdAt: null, updatedAt: null, bumpedAt: null, summary: null, alert: null, requested: true, status: 'prepared', statusReason: null, attempts: 0, preparedHeadSha: 'aaa', preparedAt: '2026-09-02T09:00:00Z', + ciState: null, createdAt: null, updatedAt: null, bumpedAt: null, summary: null, alert: null, requested: true, status: 'prepared', statusReason: null, attempts: 0, preparedHeadSha: 'aaa', preparedAt: '2026-09-02T09:00:00Z', bundlePath: '/b.json', worktreePath: '/wt', logPath: '/l.log', firstSeenAt: 'x', lastSeenAt: 'y', ...over, }; } +function checks(...pairs: [string, PrCheck['status']][]): PrCheck[] { + return pairs.map(([name, status]) => ({ name, status })); +} + describe('reconcile', () => { it('queues a new requested pull request for preparation', () => { expect(reconcile({ existing: null, snapshot: snapshot(), requested: true, viewerLogin: 'me' })) @@ -110,3 +115,76 @@ describe('reconcile', () => { .toEqual({ status: 'draft', reason: 'draft', prepare: false }); }); }); + +describe('reconcile with waitForCi', () => { + const running = snapshot({ checks: checks(['check-job', 'pending'], ['e2e', 'pending'], ['lint', 'success']) }); + const failing = snapshot({ checks: checks(['check-job', 'failure'], ['pr-mgmt-job', 'failure'], ['lint', 'success']) }); + + it('waits for a run still going rather than spending an agent on it', () => { + expect(reconcile({ existing: null, snapshot: running, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'queued', reason: 'waiting: CI running (2 checks)', prepare: false }); + }); + + it('leaves a failing pull request to its author', () => { + expect(reconcile({ existing: null, snapshot: failing, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'skipped', reason: 'CI failed: check-job, pr-mgmt-job', prepare: false }); + }); + + it('names three failing checks and counts the rest', () => { + const many = snapshot({ checks: checks(['a', 'failure'], ['b', 'failure'], ['c', 'failure'], ['d', 'failure'], ['e', 'failure']) }); + expect(reconcile({ existing: null, snapshot: many, requested: true, viewerLogin: 'me', waitForCi: true })!.reason) + .toBe('CI failed: a, b, c and 2 more'); + }); + + it('prepares when the checks pass, and when there are none to wait for', () => { + const green = snapshot({ checks: checks(['check-job', 'success'], ['integration-test-job', 'skipped']) }); + expect(reconcile({ existing: null, snapshot: green, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'queued', reason: null, prepare: true }); + expect(reconcile({ existing: null, snapshot: snapshot(), requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'queued', reason: null, prepare: true }); + }); + + it('holds nothing back while it is off', () => { + expect(reconcile({ existing: null, snapshot: failing, requested: true, viewerLogin: 'me' })) + .toEqual({ status: 'queued', reason: null, prepare: true }); + expect(reconcile({ existing: null, snapshot: running, requested: true, viewerLogin: 'me', waitForCi: false })) + .toEqual({ status: 'queued', reason: null, prepare: true }); + }); + + it('prepares a bumped pull request whatever CI says', () => { + const bumped = existing({ status: 'queued', preparedHeadSha: null, bumpedAt: '2026-09-07T10:00:00Z' }); + for (const snap of [running, failing]) { + expect(reconcile({ existing: bumped, snapshot: snap, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'queued', reason: 'bumped by the reviewer', prepare: true }); + } + }); + + it('keeps a review prepared for an older head openable while its refresh waits', () => { + const prepared = existing({ status: 'prepared', preparedHeadSha: 'aaa' }); + expect(reconcile({ existing: prepared, snapshot: { ...running, headSha: 'bbb' }, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'stale', reason: 'waiting: CI running (2 checks)', prepare: false }); + expect(reconcile({ existing: prepared, snapshot: { ...failing, headSha: 'bbb' }, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'stale', reason: 'CI failed: check-job, pr-mgmt-job', prepare: false }); + // And the row that is already stale stays where the reviewer can open it. + expect(reconcile({ existing: existing({ status: 'stale', preparedHeadSha: 'aaa', headSha: 'bbb' }), snapshot: { ...failing, headSha: 'bbb' }, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'stale', reason: 'CI failed: check-job, pr-mgmt-job', prepare: false }); + }); + + it('re-decides a CI skip every poll, and queues it once the checks pass', () => { + const held = existing({ status: 'skipped', statusReason: 'CI failed: check-job', preparedHeadSha: null, headSha: 'aaa' }); + expect(reconcile({ existing: held, snapshot: failing, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'skipped', reason: 'CI failed: check-job, pr-mgmt-job', prepare: false }); + expect(reconcile({ existing: held, snapshot: snapshot({ checks: checks(['check-job', 'success']) }), requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'queued', reason: null, prepare: true }); + // The reviewer's own filter skip is still settled at that head. + expect(reconcile({ existing: existing({ status: 'skipped', statusReason: 'payments PR', preparedHeadSha: null, headSha: 'aaa' }), snapshot: snapshot(), requested: true, viewerLogin: 'me', waitForCi: true })) + .toBeNull(); + }); + + it('says nothing about CI on a pull request it would not have prepared anyway', () => { + expect(reconcile({ existing: null, snapshot: { ...failing, isDraft: true }, requested: true, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'draft', reason: 'draft', prepare: false }); + expect(reconcile({ existing: existing(), snapshot: { ...failing, state: 'MERGED' }, requested: false, viewerLogin: 'me', waitForCi: true })) + .toEqual({ status: 'done', reason: 'merged', prepare: false }); + }); +}); diff --git a/packages/cli/tests/inbox-runtime.test.ts b/packages/cli/tests/inbox-runtime.test.ts index 4aef2b18..d50bee71 100644 --- a/packages/cli/tests/inbox-runtime.test.ts +++ b/packages/cli/tests/inbox-runtime.test.ts @@ -23,9 +23,9 @@ function attendedPr(): AttendedPr { function liveConfig(): InboxConfig { return { - pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', alertWhen: '', + pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', alertWhen: '', alertPaths: [], agent: { model: 'the-configured-model', effort: null, mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, - prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, + waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, }; } diff --git a/packages/cli/tests/inbox-store.test.ts b/packages/cli/tests/inbox-store.test.ts index 3a335a72..7c78271f 100644 --- a/packages/cli/tests/inbox-store.test.ts +++ b/packages/cli/tests/inbox-store.test.ts @@ -14,7 +14,7 @@ function snapshot(): PrSnapshot { return { owner: 'o', repo: 'r', number: 1, title: 'T', url: 'https://github.com/o/r/pull/1', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'aaa', baseRef: 'main', - additions: 1, deletions: 0, changedFiles: 1, createdAt: 'now', updatedAt: 'now', + additions: 1, deletions: 0, changedFiles: 1, createdAt: 'now', updatedAt: 'now', checks: [], files: [], }; } @@ -70,6 +70,26 @@ describe('InboxStore migration', () => { store.close(); }); + it('keeps what CI said about the head, on a table that predates the column', () => { + const seed = new DatabaseSync(path); + seed.exec(`CREATE TABLE inbox_prs ( + id TEXT PRIMARY KEY, owner TEXT NOT NULL, repo TEXT NOT NULL, number INTEGER NOT NULL, + title TEXT NOT NULL, url TEXT NOT NULL, author TEXT NOT NULL, is_draft INTEGER NOT NULL, + head_sha TEXT NOT NULL, base_ref TEXT NOT NULL, additions INTEGER NOT NULL, deletions INTEGER NOT NULL, + changed_files INTEGER NOT NULL, requested INTEGER NOT NULL, status TEXT NOT NULL, status_reason TEXT, + attempts INTEGER NOT NULL DEFAULT 0, prepared_head_sha TEXT, prepared_at TEXT, bundle_path TEXT, + worktree_path TEXT, log_path TEXT, first_seen_at TEXT NOT NULL, last_seen_at TEXT NOT NULL)`); + seed.close(); + + const store = new InboxStore(path); + expect(store.observe(snapshot(), true, 'now').ciState).toBe('none'); + expect(store.observe({ ...snapshot(), checks: [{ name: 'check-job', status: 'failure' }] }, true, 'now').ciState).toBe('failing'); + // A new commit with its checks still going: what the page shows follows the head. + expect(store.observe({ ...snapshot(), headSha: 'bbb', checks: [{ name: 'check-job', status: 'pending' }] }, true, 'now').ciState).toBe('running'); + expect(store.get('o/r#1')!.ciState).toBe('running'); + store.close(); + }); + it('bumps a row to queued and remembers when, until the bump is spent', () => { const store = new InboxStore(path); store.observe(snapshot(), true, 'now'); diff --git a/packages/cli/tests/inbox-tick.test.ts b/packages/cli/tests/inbox-tick.test.ts index 0a7a4756..fbdd3ec8 100644 --- a/packages/cli/tests/inbox-tick.test.ts +++ b/packages/cli/tests/inbox-tick.test.ts @@ -10,7 +10,8 @@ function snapshot(over: Partial = {}): PrSnapshot { return { owner: 'o', repo: 'r', number: 1, title: 'A change', url: 'https://github.com/o/r/pull/1', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'aaa', baseRef: 'main', - additions: 10, deletions: 2, changedFiles: 3, createdAt: '2026-09-02T10:00:00Z', updatedAt: '2026-09-02T10:00:00Z', ...over, + additions: 10, deletions: 2, changedFiles: 3, createdAt: '2026-09-02T10:00:00Z', updatedAt: '2026-09-02T10:00:00Z', + checks: [], files: [], ...over, }; } @@ -61,6 +62,8 @@ function deps(over: Partial = {}): TickDeps { log: () => {}, now: () => '2026-09-02T12:00:00.000Z', maxPrepared: 100, + waitForCi: false, + alertPaths: [], agentModel: 'the-configured-model', pauseUntil: until => { pauses.push(until); }, ...over, @@ -155,6 +158,41 @@ describe('runTick', () => { expect(view.ready[0].openUrl).toBe('http://localhost:5390/open/o%2Fr%232'); }); + it('alerts on the reviewer\'s own paths when the agent raised nothing, and defers to it when it did', async () => { + forge.set(snapshot({ number: 1, files: [{ path: 'README.md', additions: 1, deletions: 0 }, { path: 'packages/shared/src/model/user.ts', additions: 2, deletions: 1 }] })); + forge.set(snapshot({ number: 2, files: [{ path: 'packages/shared/src/model/user.ts', additions: 2, deletions: 1 }] })); + forge.set(snapshot({ number: 3, files: [{ path: 'README.md', additions: 1, deletions: 0 }] })); + await runTick(store, deps({ alertPaths: ['packages/shared/src/model/**'] })); + + expect(store.get('o/r#1')!.alert).toBe('touches packages/shared/src/model/user.ts'); + // Number 2 is the one the fake agent alerts on: its own words stand. + expect(store.get('o/r#2')!.alert).toBe('touches auth'); + expect(store.get('o/r#3')!.alert).toBeNull(); + }); + + it('carries what CI said about each head to the surface', async () => { + forge.set(snapshot({ number: 1, checks: [{ name: 'check-job', status: 'success' }] })); + forge.set(snapshot({ number: 2, checks: [{ name: 'check-job', status: 'pending' }] })); + forge.set(snapshot({ number: 3 })); + await runTick(store, deps()); + + const view = buildView(store, 'http://localhost:5390', '2026-09-02T12:00:00.000Z'); + expect(new Map(view.ready.map(row => [row.number, row.ciState]))) + .toEqual(new Map([[1, 'passing'], [2, 'running'], [3, 'none']])); + }); + + it('leaves a pull request queued while its CI runs, and skips one whose CI failed', async () => { + forge.set(snapshot({ number: 1, checks: [{ name: 'check-job', status: 'pending' }] })); + forge.set(snapshot({ number: 2, checks: [{ name: 'check-job', status: 'failure' }] })); + forge.set(snapshot({ number: 3, checks: [{ name: 'check-job', status: 'success' }] })); + await runTick(store, deps({ waitForCi: true })); + + expect(prepared).toEqual(['o/r#3']); + expect(store.get('o/r#1')!.statusReason).toBe('waiting: CI running (1 checks)'); + expect(store.get('o/r#2')!.status).toBe('skipped'); + expect(store.get('o/r#2')!.statusReason).toBe('CI failed: check-job'); + }); + it('holds a failed preparation with its reason and log, and stops after the attempt cap', async () => { forge.set(snapshot()); prepareResult = () => ({ kind: 'failed', failure: 'agent', reason: 'no local clone', worktree: null, logPath: '/l/1.log', run: run() }); diff --git a/packages/cli/tests/inbox-units.test.ts b/packages/cli/tests/inbox-units.test.ts index d78c812c..94b483f7 100644 --- a/packages/cli/tests/inbox-units.test.ts +++ b/packages/cli/tests/inbox-units.test.ts @@ -21,6 +21,16 @@ describe('parseInboxConfig', () => { expect(config.agent).toEqual(DEFAULT_INBOX_CONFIG.agent); }); + it('takes the CI hold and the alert paths, and refuses each by name', () => { + expect(parseInboxConfig({ waitForCi: true, alertPaths: [' packages/shared/src/model/** ', '**/dbref/**'] })) + .toMatchObject({ waitForCi: true, alertPaths: ['packages/shared/src/model/**', '**/dbref/**'] }); + expect(DEFAULT_INBOX_CONFIG.waitForCi).toBe(false); + expect(DEFAULT_INBOX_CONFIG.alertPaths).toEqual([]); + expect(() => parseInboxConfig({ waitForCi: 'yes' })).toThrow(/waitForCi must be true or false/); + expect(() => parseInboxConfig({ alertPaths: 'src/**' })).toThrow(/alertPaths must be an array of non-empty globs/); + expect(() => parseInboxConfig({ alertPaths: [' '] })).toThrow(/alertPaths must be an array of non-empty globs/); + }); + it('refuses a non-positive interval, by name', () => { expect(() => parseInboxConfig({ pollMinutes: 0 })).toThrow(/pollMinutes must be a positive number/); expect(() => parseInboxConfig([])).toThrow(/must be a JSON object/); @@ -69,7 +79,8 @@ describe('parseInboxConfig', () => { const path = join(dir, 'config.json'); writeFileSync(path, JSON.stringify({ port: 5399, filter: 'old', pollMinutes: 2 }, null, 2)); const settings = { - filter: 'skip payments', alertWhen: 'a P1', maxPrepared: 3, pollMinutes: 7, live: false, liveTimeoutMinutes: 4, prepareTimeoutMinutes: 20, + filter: 'skip payments', alertWhen: 'a P1', alertPaths: ['packages/shared/**'], maxPrepared: 3, pollMinutes: 7, + live: false, liveTimeoutMinutes: 4, prepareTimeoutMinutes: 20, waitForCi: true, agent: { model: 'opus', effort: 'high', mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, }; saveInboxSettings(path, settings); @@ -105,6 +116,7 @@ describe('composePrompt', () => { owner: 'o', repo: 'r', number: 7, title: 'Add a widget', url: 'https://github.com/o/r/pull/7', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'abc', baseRef: 'main', additions: 12, deletions: 3, changedFiles: 2, createdAt: '2026-09-02T10:00:00Z', updatedAt: '2026-09-02T10:00:00Z', + checks: [], files: [], }; it('tells the agent the worktree, forbids the forge, and asks for a verdict', () => { @@ -140,6 +152,49 @@ describe('composePrompt', () => { expect(some).toContain(' mcp__claude_ai_Atlassian__getJiraIssue\n mcp__claude_ai_Slack__slack_read_thread'); expect(some).toContain('Nothing else outside this checkout.'); }); + + it('reports what CI made of the head and tells the agent not to redo its work', () => { + const prompt = composePrompt({ + snapshot: { + ...snapshot, + checks: [ + { name: 'check-job', status: 'success' }, + { name: 'pr-ecosystem-test (admin3)', status: 'success' }, + { name: 'e2e', status: 'pending' }, + { name: 'integration-test-job', status: 'skipped' }, + { name: 'ncapp3-playwright-e2e-tests-job', status: 'skipped' }, + ], + }, + worktreePath: '/wt', port: 5555, filter: '', alertWhen: '', mcpAllow: [], + }); + expect(prompt).toContain('CI at this head: check-job SUCCESS \u00b7 pr-ecosystem-test (admin3) SUCCESS \u00b7 e2e PENDING \u00b7 2 more skipped'); + expect(prompt).toContain('Do not install dependencies, build, typecheck, lint or run tests'); + expect(prompt).toContain('If a check failed or is still running,\nsay so in the summary.'); + }); + + it('says so plainly when every check was skipped', () => { + const prompt = composePrompt({ + snapshot: { ...snapshot, checks: [{ name: 'a', status: 'skipped' }, { name: 'b', status: 'skipped' }] }, + worktreePath: '/wt', port: 5555, filter: '', alertWhen: '', mcpAllow: [], + }); + expect(prompt).toContain('CI at this head: 2 checks skipped, none ran'); + }); + + it('says CI has not reported when nothing has, and still forbids the toolchain', () => { + const prompt = composePrompt({ snapshot, worktreePath: '/wt', port: 5555, filter: '', alertWhen: '', mcpAllow: [] }); + expect(prompt).toContain('CI has not reported for this head.'); + expect(prompt).toContain('Do not install dependencies, build, typecheck, lint or run tests'); + }); + + it('holds a check name to one line and a length, as it does the author\'s other text', () => { + const name = `pr-feature-branch / ${'x'.repeat(200)}`; + const prompt = composePrompt({ + snapshot: { ...snapshot, checks: [{ name: `deploy\n${name}`, status: 'failure' }] }, + worktreePath: '/wt', port: 5555, filter: '', alertWhen: '', mcpAllow: [], + }); + const line = prompt.split('\n').find(l => l.startsWith('CI at this head:'))!; + expect(line).toBe(`CI at this head: ${`deploy ${name}`.slice(0, 80)} FAILURE`); + }); }); describe('composePrompt alerts', () => { @@ -147,6 +202,7 @@ describe('composePrompt alerts', () => { owner: 'o', repo: 'r', number: 7, title: 'Add a widget', url: 'https://github.com/o/r/pull/7', author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'abc', baseRef: 'main', additions: 12, deletions: 3, changedFiles: 2, createdAt: '2026-09-02T10:00:00Z', updatedAt: '2026-09-02T10:00:00Z', + checks: [], files: [], }; it('asks for an ALERT line only when the reviewer said what matters', () => { @@ -178,7 +234,8 @@ describe('summarizeFindings', () => { describe('parseSettingsPatch', () => { const full = { - filter: 'a', alertWhen: 'b', maxPrepared: 2, pollMinutes: 3, live: false, liveTimeoutMinutes: 5, prepareTimeoutMinutes: 15, + filter: 'a', alertWhen: 'b', alertPaths: ['src/**'], maxPrepared: 2, pollMinutes: 3, live: false, + liveTimeoutMinutes: 5, prepareTimeoutMinutes: 15, waitForCi: false, agent: { model: null, effort: null, mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, }; it('takes every editable key, validated as the config file is, and refuses anything else by name', () => { diff --git a/packages/git/package.json b/packages/git/package.json index f90e93ae..ebdea7d4 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.10.23", + "version": "0.10.24", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index d68bf310..aab2c12a 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.10.23", + "version": "0.10.24", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/src/inbox.ts b/packages/github/src/inbox.ts index ebc2c707..2dd16588 100644 --- a/packages/github/src/inbox.ts +++ b/packages/github/src/inbox.ts @@ -10,6 +10,24 @@ export interface PrRef { export const PR_STATES = ['OPEN', 'CLOSED', 'MERGED'] as const; export type PrState = (typeof PR_STATES)[number]; +/** One CI check on a head, whatever the forge calls it: a check run or a commit status. */ +export interface PrCheck { + name: string; + status: 'success' | 'failure' | 'pending' | 'skipped' | 'neutral'; +} + +export interface PrFile { + path: string; + additions: number; + deletions: number; +} + +/** What CI amounts to for a head, taken as a whole. */ +export type CiState = 'passing' | 'failing' | 'running' | 'none'; + +/** Beyond this a pull request's file list says nothing more that a prompt or an alert needs. */ +export const MAX_SNAPSHOT_FILES = 300; + /** What one `gh pr view` says about a pull request, as far as the inbox cares. */ export interface PrSnapshot extends PrRef { title: string; @@ -25,6 +43,9 @@ export interface PrSnapshot extends PrRef { changedFiles: number; createdAt: string; updatedAt: string; + /** What CI says about this head; empty when nothing has reported, or when gh cannot say. */ + checks: PrCheck[]; + files: PrFile[]; } /** The open pull requests asking the authenticated user for a review. */ @@ -65,7 +86,7 @@ export async function viewPr(ref: PrRef): Promise { const json = await ghAsync([ 'pr', 'view', String(ref.number), '--repo', `${ref.owner}/${ref.repo}`, - '--json', 'number,title,url,author,isDraft,state,headRefOid,baseRefName,additions,deletions,changedFiles,createdAt,updatedAt', + '--json', 'number,title,url,author,isDraft,state,headRefOid,baseRefName,additions,deletions,changedFiles,createdAt,updatedAt,statusCheckRollup,files', ]); return parsePrSnapshot(ref, json); } catch { @@ -95,9 +116,117 @@ export function parsePrSnapshot(ref: PrRef, json: string): PrSnapshot | null { changedFiles: Number(data.changedFiles ?? 0), createdAt: String(data.createdAt ?? ''), updatedAt: String(data.updatedAt ?? ''), + checks: parseChecks(data.statusCheckRollup), + files: parseFiles(data.files), }; } +/** Worst first: which of two reports on the same check name is the one to keep. */ +const CHECK_SEVERITY: Record = { + failure: 4, + pending: 3, + success: 2, + neutral: 1, + skipped: 0, +}; + +/** + * `statusCheckRollup` mixes check runs (a `status` and, once done, a `conclusion`) with commit + * statuses (a `state`), and a check name can appear several times — a re-run, or one workflow + * triggered more than once. Each name is kept once, at its worst report. + */ +export function parseChecks(raw: unknown): PrCheck[] { + if (!Array.isArray(raw)) { + return []; + } + const byName = new Map(); + for (const item of raw) { + if (typeof item !== 'object' || item === null) { + continue; + } + const entry = item as Record; + const name = typeof entry.name === 'string' && entry.name !== '' + ? entry.name + : typeof entry.context === 'string' ? entry.context : ''; + if (!name) { + continue; + } + const status = checkStatus(entry); + const seen = byName.get(name); + if (!seen || CHECK_SEVERITY[status] > CHECK_SEVERITY[seen.status]) { + byName.set(name, { name, status }); + } + } + return [...byName.values()]; +} + +function checkStatus(entry: Record): PrCheck['status'] { + // A check run that has not completed, whatever it calls that (IN_PROGRESS, QUEUED, WAITING), has + // no conclusion yet; a commit status has only its state. + const runStatus = typeof entry.status === 'string' ? entry.status.toUpperCase() : ''; + if (runStatus && runStatus !== 'COMPLETED') { + return 'pending'; + } + const outcome = typeof entry.conclusion === 'string' && entry.conclusion !== '' + ? entry.conclusion.toUpperCase() + : typeof entry.state === 'string' ? entry.state.toUpperCase() : ''; + switch (outcome) { + case 'SUCCESS': + return 'success'; + case 'SKIPPED': + return 'skipped'; + // A run that was cancelled or superseded never decided anything, so it must not outrank the + // re-run of the same check that did: neutral loses to that run's success. + case 'NEUTRAL': + case 'CANCELLED': + case 'STALE': + return 'neutral'; + case 'PENDING': + case 'EXPECTED': + case '': + return 'pending'; + default: + return 'failure'; + } +} + +export function parseFiles(raw: unknown): PrFile[] { + if (!Array.isArray(raw)) { + return []; + } + const files: PrFile[] = []; + for (const item of raw) { + const entry = item as Record | null; + if (typeof entry?.path !== 'string' || entry.path === '') { + continue; + } + files.push({ + path: entry.path, + additions: Number(entry.additions ?? 0), + deletions: Number(entry.deletions ?? 0), + }); + if (files.length === MAX_SNAPSHOT_FILES) { + break; + } + } + return files; +} + +/** + * What CI amounts to for a head: one word for the whole set of checks. Nothing passed unless + * something actually ran — a set with no verdict in it at all, skipped or undecided, is as good as + * no checks. + */ +export function ciState(checks: PrCheck[]): CiState { + if (checks.some(check => check.status === 'failure')) { + return 'failing'; + } + if (checks.some(check => check.status === 'pending')) { + return 'running'; + } + return checks.some(check => check.status === 'success') ? 'passing' : 'none'; +} + function isPrState(value: unknown): value is PrState { return typeof value === 'string' && (PR_STATES as readonly string[]).includes(value); } diff --git a/packages/github/src/index.ts b/packages/github/src/index.ts index 40c1e8c5..bfd47ad1 100644 --- a/packages/github/src/index.ts +++ b/packages/github/src/index.ts @@ -7,5 +7,5 @@ export { commentableLines, isAlreadyCommented } from './comment-targets.js'; export { matchCreatedComments } from './comment-ids.js'; export type { CreatedComment, SentComment } from './comment-ids.js'; export { isGitHubPrUrl, parseGitHubPrUrl, checkoutPr, getPrBase, parsePrBase } from './pr-url.js'; -export { searchReviewRequested, viewPr, parseReviewRequested, parsePrSnapshot } from './inbox.js'; -export type { PrRef, PrSnapshot, PrState } from './inbox.js'; +export { searchReviewRequested, viewPr, parseReviewRequested, parsePrSnapshot, parseChecks, parseFiles, ciState, MAX_SNAPSHOT_FILES } from './inbox.js'; +export type { PrRef, PrSnapshot, PrState, PrCheck, PrFile, CiState } from './inbox.js'; diff --git a/packages/github/tests/pr-checks.test.ts b/packages/github/tests/pr-checks.test.ts new file mode 100644 index 00000000..5a538fb5 --- /dev/null +++ b/packages/github/tests/pr-checks.test.ts @@ -0,0 +1,132 @@ +import { describe, it, expect } from 'vitest'; +import { ciState, parseChecks, parseFiles, parsePrSnapshot, MAX_SNAPSHOT_FILES } from '../src/inbox.js'; + +const ref = { owner: 'o', repo: 'r', number: 1 }; + +/** A check run as `gh pr view --json statusCheckRollup` gives one. */ +function run(name: string, status: string, conclusion: string | null) { + return { __typename: 'CheckRun', name, status, conclusion, workflowName: 'ci' }; +} + +/** A commit status, which the same field mixes in with the check runs. */ +function context(name: string, state: string) { + return { __typename: 'StatusContext', context: name, state }; +} + +describe('parseChecks', () => { + it('reads check runs and commit statuses alike', () => { + expect(parseChecks([ + run('check-job', 'COMPLETED', 'SUCCESS'), + run('e2e', 'IN_PROGRESS', null), + run('deploy', 'QUEUED', null), + run('integration-test-job', 'COMPLETED', 'SKIPPED'), + run('lint', 'COMPLETED', 'NEUTRAL'), + run('build', 'COMPLETED', 'TIMED_OUT'), + run('release', 'COMPLETED', 'CANCELLED'), + run('docs', 'COMPLETED', 'STALE'), + context('vercel', 'SUCCESS'), + context('legacy', 'ERROR'), + context('waiting', 'PENDING'), + ])).toEqual([ + { name: 'check-job', status: 'success' }, + { name: 'e2e', status: 'pending' }, + { name: 'deploy', status: 'pending' }, + { name: 'integration-test-job', status: 'skipped' }, + { name: 'lint', status: 'neutral' }, + { name: 'build', status: 'failure' }, + { name: 'release', status: 'neutral' }, + { name: 'docs', status: 'neutral' }, + { name: 'vercel', status: 'success' }, + { name: 'legacy', status: 'failure' }, + { name: 'waiting', status: 'pending' }, + ]); + }); + + it('lets a re-run\'s success outrank the attempt that was cancelled or superseded', () => { + // A cancelled or stale run decided nothing, so the run that did decide is the head's verdict. + for (const undecided of ['CANCELLED', 'STALE']) { + const checks = parseChecks([run('check-job', 'COMPLETED', undecided), run('check-job', 'COMPLETED', 'SUCCESS')]); + expect(checks).toEqual([{ name: 'check-job', status: 'success' }]); + expect(ciState(checks)).toBe('passing'); + } + // A real failure still outranks a success at the same name: something is wrong with the code. + expect(parseChecks([run('check-job', 'COMPLETED', 'TIMED_OUT'), run('check-job', 'COMPLETED', 'SUCCESS')])) + .toEqual([{ name: 'check-job', status: 'failure' }]); + }); + + it('keeps one entry per name, at its worst report', () => { + // A workflow triggered several times, or re-run: the failure is the one that matters. + expect(parseChecks([ + run('check-job', 'COMPLETED', 'SUCCESS'), + run('check-job', 'COMPLETED', 'FAILURE'), + run('check-job', 'COMPLETED', 'SUCCESS'), + run('doc', 'COMPLETED', 'SKIPPED'), + run('doc', 'IN_PROGRESS', null), + ])).toEqual([ + { name: 'check-job', status: 'failure' }, + { name: 'doc', status: 'pending' }, + ]); + }); + + it('takes nothing from a field that is missing or malformed', () => { + expect(parseChecks(undefined)).toEqual([]); + expect(parseChecks(null)).toEqual([]); + expect(parseChecks('SUCCESS')).toEqual([]); + expect(parseChecks([null, 42, {}, { name: '' }])).toEqual([]); + }); +}); + +describe('parseFiles', () => { + it('reads the changed paths and their counts, and caps a huge diff', () => { + expect(parseFiles([{ path: 'src/a.ts', additions: 3, deletions: 1 }, { path: 'b.ts' }])) + .toEqual([{ path: 'src/a.ts', additions: 3, deletions: 1 }, { path: 'b.ts', additions: 0, deletions: 0 }]); + + const many = Array.from({ length: 500 }, (_, i) => ({ path: `f${i}.ts`, additions: 1, deletions: 0 })); + const files = parseFiles(many); + expect(files).toHaveLength(MAX_SNAPSHOT_FILES); + expect(files.at(-1)!.path).toBe(`f${MAX_SNAPSHOT_FILES - 1}.ts`); + }); + + it('takes nothing from a field that is missing or malformed', () => { + expect(parseFiles(undefined)).toEqual([]); + expect(parseFiles([null, { path: 1 }, { path: '' }])).toEqual([]); + }); +}); + +describe('ciState', () => { + it('is one word for the whole set of checks', () => { + expect(ciState([])).toBe('none'); + expect(ciState([{ name: 'a', status: 'success' }, { name: 'b', status: 'skipped' }])).toBe('passing'); + // Nothing here decided anything, so there is nothing to call green. + expect(ciState([{ name: 'a', status: 'skipped' }, { name: 'b', status: 'neutral' }])).toBe('none'); + expect(ciState(parseChecks([run('check-job', 'COMPLETED', 'CANCELLED')]))).toBe('none'); + expect(ciState([{ name: 'a', status: 'success' }, { name: 'b', status: 'pending' }])).toBe('running'); + // A failure outranks a run still going: there is already something to fix. + expect(ciState([{ name: 'a', status: 'pending' }, { name: 'b', status: 'failure' }])).toBe('failing'); + }); +}); + +describe('parsePrSnapshot', () => { + const base = { + title: 'T', url: 'https://github.com/o/r/pull/1', author: { login: 'alice' }, + isDraft: false, state: 'OPEN', headRefOid: 'abc', baseRefName: 'main', + additions: 1, deletions: 0, changedFiles: 1, createdAt: 'now', updatedAt: 'now', + }; + + it('carries what CI said and which files changed', () => { + const snapshot = parsePrSnapshot(ref, JSON.stringify({ + ...base, + statusCheckRollup: [run('check-job', 'COMPLETED', 'FAILURE')], + files: [{ path: 'src/a.ts', additions: 1, deletions: 0 }], + })); + expect(snapshot?.checks).toEqual([{ name: 'check-job', status: 'failure' }]); + expect(snapshot?.files).toEqual([{ path: 'src/a.ts', additions: 1, deletions: 0 }]); + }); + + it('leaves both empty when gh does not report them', () => { + // An older gh, or a repository with no checks at all. + const snapshot = parsePrSnapshot(ref, JSON.stringify(base)); + expect(snapshot?.checks).toEqual([]); + expect(snapshot?.files).toEqual([]); + }); +}); diff --git a/packages/parser/package.json b/packages/parser/package.json index ddebcbfb..08d4cbd7 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.10.23", + "version": "0.10.24", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 6d2c18fd..080bff92 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.10.23", + "version": "0.10.24", "type": "module", "private": true, "scripts": {