From 5d6f68ef0ec92057402b9e55559afb6484218a06 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Mon, 7 Sep 2026 20:08:08 +0200 Subject: [PATCH 1/2] feat: the review agent is told what CI found and never runs the toolchain; a pull request can wait for green CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inbox's `gh pr view` call now asks for `statusCheckRollup` and `files`, so every snapshot carries the head's checks (normalised, duplicates collapsed to the worst report) and its changed paths. The preparing agent's prompt names each check that reported with its verdict, counts the ones a workflow skipped, and tells it not to install, build, typecheck, lint or test — CI has done that, and the checkout is the author's code. Two settings follow from having the data: `waitForCi` holds a pull request in the queue while its checks run and sets one aside when they fail, re-decided at every poll so passing checks bring it back on their own; `alertPaths` marks a review as needing the reviewer now when a changed file matches one of their globs, whatever the agent made of `alertWhen`. Each card gets a CI dot, and the store keeps `ci_state` per row. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- README.md | 4 + package-lock.json | 12 +- packages/api/package.json | 2 +- packages/cli/package.json | 2 +- packages/cli/src/inbox/config.ts | 25 +++- packages/cli/src/inbox/daemon.ts | 7 +- packages/cli/src/inbox/page.ts | 22 ++++ packages/cli/src/inbox/paths-alert.ts | 52 ++++++++ packages/cli/src/inbox/prompt.ts | 32 ++++- packages/cli/src/inbox/reconcile.ts | 58 ++++++++- packages/cli/src/inbox/settings.ts | 2 +- packages/cli/src/inbox/store.ts | 26 ++-- packages/cli/src/inbox/tick.ts | 10 +- packages/cli/src/inbox/view.ts | 4 + packages/cli/tests/inbox-daemon.test.ts | 6 +- packages/cli/tests/inbox-open.test.ts | 14 ++- packages/cli/tests/inbox-page.test.ts | 22 ++++ packages/cli/tests/inbox-paths-alert.test.ts | 41 +++++++ packages/cli/tests/inbox-prepare.test.ts | 6 +- packages/cli/tests/inbox-reconcile.test.ts | 84 ++++++++++++- packages/cli/tests/inbox-runtime.test.ts | 4 +- packages/cli/tests/inbox-store.test.ts | 22 +++- packages/cli/tests/inbox-tick.test.ts | 40 +++++- packages/cli/tests/inbox-units.test.ts | 53 +++++++- packages/git/package.json | 2 +- packages/github/package.json | 2 +- packages/github/src/inbox.ts | 123 ++++++++++++++++++- packages/github/src/index.ts | 4 +- packages/github/tests/pr-checks.test.ts | 116 +++++++++++++++++ packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- 31 files changed, 748 insertions(+), 53 deletions(-) create mode 100644 packages/cli/src/inbox/paths-alert.ts create mode 100644 packages/cli/tests/inbox-paths-alert.test.ts create mode 100644 packages/github/tests/pr-checks.test.ts diff --git a/README.md b/README.md index 7c1bd332..c40bdbd0 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, none when nothing has reported. 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..71252066 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,32 @@ 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 (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..25de3fbd 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,41 @@ 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 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 +194,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 +226,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..e260945a 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,109 @@ 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'; + case 'NEUTRAL': + 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. */ +export function ciState(checks: PrCheck[]): CiState { + if (checks.length === 0) { + return 'none'; + } + if (checks.some(check => check.status === 'failure')) { + return 'failing'; + } + return checks.some(check => check.status === 'pending') ? 'running' : 'passing'; +} + 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..c4a1f9ce --- /dev/null +++ b/packages/github/tests/pr-checks.test.ts @@ -0,0 +1,116 @@ +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'), + 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: 'failure' }, + { name: 'vercel', status: 'success' }, + { name: 'legacy', status: 'failure' }, + { name: 'waiting', status: 'pending' }, + ]); + }); + + 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'); + expect(ciState([{ name: 'a', status: 'skipped' }, { name: 'b', status: 'neutral' }])).toBe('passing'); + 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": { From fcebc713b397aebe8e5cb2b81bfff546851ad57c Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Mon, 7 Sep 2026 20:15:17 +0200 Subject: [PATCH 2/2] fix: a cancelled run no longer makes a passing head read as failing, and an all-skipped CI line reads CANCELLED and STALE conclusions decided nothing, so they map to `neutral` rather than `failure`: the re-run's success now outranks the attempt that was cancelled, instead of a head whose CI passed showing a red dot, a "CI failed:" skip that never lifts, and a prompt telling the agent a check failed. TIMED_OUT, ACTION_REQUIRED, FAILURE, ERROR and STARTUP_FAILURE stay failures. `ciState` calls a head passing only when something actually passed: a set of nothing but skipped and undecided checks is `none`, so no dot claims green for a head where nothing ran. And the prompt's CI line says "19 checks skipped, none ran" instead of "19 more skipped" when there is nothing to put before "more". Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- README.md | 2 +- packages/cli/src/inbox/prompt.ts | 3 +++ packages/cli/tests/inbox-units.test.ts | 8 ++++++++ packages/github/src/inbox.ts | 18 +++++++++++++----- packages/github/tests/pr-checks.test.ts | 20 ++++++++++++++++++-- 5 files changed, 43 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index c40bdbd0..c2389565 100644 --- a/README.md +++ b/README.md @@ -369,7 +369,7 @@ 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, none when nothing has reported. 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. +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. diff --git a/packages/cli/src/inbox/prompt.ts b/packages/cli/src/inbox/prompt.ts index 71252066..7f65c1c0 100644 --- a/packages/cli/src/inbox/prompt.ts +++ b/packages/cli/src/inbox/prompt.ts @@ -111,6 +111,9 @@ function ciLine(checks: PrCheck[]): string { 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`); } diff --git a/packages/cli/tests/inbox-units.test.ts b/packages/cli/tests/inbox-units.test.ts index 25de3fbd..94b483f7 100644 --- a/packages/cli/tests/inbox-units.test.ts +++ b/packages/cli/tests/inbox-units.test.ts @@ -172,6 +172,14 @@ describe('composePrompt', () => { 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.'); diff --git a/packages/github/src/inbox.ts b/packages/github/src/inbox.ts index e260945a..2dd16588 100644 --- a/packages/github/src/inbox.ts +++ b/packages/github/src/inbox.ts @@ -175,7 +175,11 @@ function checkStatus(entry: Record): PrCheck['status'] { 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': @@ -208,15 +212,19 @@ export function parseFiles(raw: unknown): PrFile[] { return files; } -/** What CI amounts to for a head: one word for the whole set of checks. */ +/** + * 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.length === 0) { - return 'none'; - } if (checks.some(check => check.status === 'failure')) { return 'failing'; } - return checks.some(check => check.status === 'pending') ? 'running' : 'passing'; + 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 { diff --git a/packages/github/tests/pr-checks.test.ts b/packages/github/tests/pr-checks.test.ts index c4a1f9ce..5a538fb5 100644 --- a/packages/github/tests/pr-checks.test.ts +++ b/packages/github/tests/pr-checks.test.ts @@ -23,6 +23,7 @@ describe('parseChecks', () => { 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'), @@ -33,13 +34,26 @@ describe('parseChecks', () => { { name: 'integration-test-job', status: 'skipped' }, { name: 'lint', status: 'neutral' }, { name: 'build', status: 'failure' }, - { name: 'release', 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([ @@ -83,7 +97,9 @@ 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'); - expect(ciState([{ name: 'a', status: 'skipped' }, { name: 'b', status: 'neutral' }])).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');