From 9597d611b9a056813263bb9cd94a93fd576223c1 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Tue, 8 Sep 2026 17:19:19 +0200 Subject: [PATCH 1/2] feat: with postAlerts on, the daemon posts the findings behind an alert to the pull request, prefixed, and keeps the pull request as the reviewer's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn `postAlerts` on and the daemon puts the findings the agent named as the reason for an alert on the pull request itself, the moment the review is prepared: one COMMENT review in the reviewer's name, its body the alert reason and each inline comment opening with `postPrefix`, at most once per head. A path alert posts nothing, and the post is the daemon's own `gh` call after the agent has finished — the agent still never gets the reviewer's credentials. The posted findings are marked as sent in the prepared session (a hidden `agent mark-posted`), travel that way in the bundle, and are left out of the reviewer's own submit. A submitted review withdraws the review request, so a row the daemon posted to is reconciled as though the search still listed it: it stays openable, a push re-prepares and posts again at the new head, a dismissal holds, and the reviewer's own review moves it to Handled. A post that fails is logged and leaves the review prepared with its alert. 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/api/src/bundle.ts | 28 ++- packages/api/tests/bundle.test.ts | 36 ++++ packages/cli/package.json | 2 +- packages/cli/src/agent.ts | 33 +++ packages/cli/src/bundle.ts | 17 +- packages/cli/src/inbox/config.ts | 30 ++- packages/cli/src/inbox/daemon.ts | 5 +- packages/cli/src/inbox/page.ts | 24 ++- packages/cli/src/inbox/prepare.ts | 145 ++++++++++++- packages/cli/src/inbox/runtime.ts | 26 ++- packages/cli/src/inbox/settings.ts | 4 +- packages/cli/src/inbox/store.ts | 32 ++- packages/cli/src/inbox/tick.ts | 40 +++- packages/cli/src/inbox/view.ts | 5 +- packages/cli/tests/bundle-roundtrip.test.ts | 33 +++ packages/cli/tests/inbox-daemon.test.ts | 5 +- packages/cli/tests/inbox-open.test.ts | 10 +- packages/cli/tests/inbox-page.test.ts | 23 ++ packages/cli/tests/inbox-prepare.test.ts | 224 +++++++++++++++++++- packages/cli/tests/inbox-reconcile.test.ts | 2 +- packages/cli/tests/inbox-runtime.test.ts | 61 ++++++ packages/cli/tests/inbox-store.test.ts | 40 ++++ packages/cli/tests/inbox-tick.test.ts | 144 ++++++++++++- packages/cli/tests/inbox-units.test.ts | 34 ++- packages/cli/tests/server-routes.test.ts | 35 +++ packages/cli/tests/submitted-state.test.ts | 62 +++++- packages/git/package.json | 2 +- packages/github/package.json | 2 +- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- 33 files changed, 1074 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 19e1701e..c7aee08e 100644 --- a/README.md +++ b/README.md @@ -336,7 +336,7 @@ running past the end would otherwise be counted and highlighted with nothing to Every review you post from diffity is noted against its pull request, with the commit it was posted against, and the pull request then stays under **Handled** instead of vanishing when GitHub withdraws the review request. The card links to the pull request itself — the worktree is reclaimed once the review is out — and says what you said: "you approved", "you requested changes", "you commented", and when. When the author pushes after your review, the card moves to the top of the list, reads "new commits since you approved" and is badged `updated`, so a pull request that has come back to you is not something you have to remember; ↑ prepares a fresh review of the current head, re-request or not, and × sets the row aside until the next push. A pull request leaves the list when the author asks for a new review — the search lists it again and it goes back in the queue like anything else — or when it is merged or closed. This counts reviews posted from any diffity, so one you posted from your own clone brings its pull request into the list at the next poll, at the cost of one `gh pr view`. `diffity inbox status` prints the same list. -The daemon never posts your prepared reviews to GitHub — they are local drafts you open and submit yourself — and it runs the review agent with your GitHub credentials stripped from its environment. Its own git calls run with hooks disabled, so a checkout's hook scripts — the author's code — never run with your credentials. That said, the agent executes the pull request's own repository code (see the warning below), so treat the "never posts" behaviour as the daemon's design, not a sandbox. +The daemon never posts your prepared reviews to GitHub unless you turn `postAlerts` on — they are local drafts you open and submit yourself. With it on, the only thing that goes out is the findings the agent named as the reason for an alert: one `COMMENT` review in your name, never an approval or a request for changes, every comment opening with `postPrefix` so nobody reads it as your verdict, and at most once per head. Those findings stay in the prepared review marked as already sent, so your own submit does not send them twice, and the pull request stays listed as awaiting you even though a submitted review withdraws the request — the daemon's post is what consumed it — until you review it yourself, dismiss it, or it is merged or closed. The post is the daemon's own call to `gh`, made after the agent has finished: the review agent still runs with your GitHub credentials stripped from its environment, and its own git calls run with hooks disabled, so a checkout's hook scripts — the author's code — never run with your credentials. That said, the agent executes the pull request's own repository code (see the warning below), so treat all of this as the daemon's design, not a sandbox. ```bash diffity inbox # run the watcher and a small status server @@ -359,6 +359,8 @@ On first run it writes `~/.diffity/inbox/config.json`: | `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"). The agent has to load the skill and read the diff to decide, so every skip costs an agent run; `skipTitles` above costs nothing. 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, naming the findings behind the flag; the flagged ones are listed under **Alerted**, above Ready, and 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. | +| `postAlerts` | Whether the daemon posts the findings behind an `alertWhen` alert to the pull request itself, as a `COMMENT` review in your name at most once per head (default false). An alert raised by `alertPaths` posts nothing — that is your own rule about the paths, with nothing in it to tell the author. Editable from the page's Settings panel. | +| `postPrefix` | What every posted comment opens with, so nobody reads one as a verdict you have stood behind (default `[Automated AI pre-review, not yet checked by human]`). Must not be empty while `postAlerts` is on. 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. | diff --git a/package-lock.json b/package-lock.json index 100a4a18..501afb73 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.30", + "version": "0.10.31", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.30", + "version": "0.10.31", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.30", + "version": "0.10.31", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.30", + "version": "0.10.31", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.30", + "version": "0.10.31", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.30", + "version": "0.10.31", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/packages/api/package.json b/packages/api/package.json index f338e50b..f76e34a8 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.30", + "version": "0.10.31", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/api/src/bundle.ts b/packages/api/src/bundle.ts index a139332a..545db09d 100644 --- a/packages/api/src/bundle.ts +++ b/packages/api/src/bundle.ts @@ -27,8 +27,9 @@ import { /** * A prepared review in portable form: the threads and walkthrough tours of one session, pinned to - * the commit whose working tree their line numbers mean. Everything machine- and forge-local — - * ids, live state, what was submitted where — stays behind; an import mints its own. + * the commit whose working tree their line numbers mean. Machine-local state — ids, live state — + * stays behind; an import mints its own. What a finding was already posted to the forge as travels + * with it, so the session it opens in does not offer to send it a second time. */ export interface ReviewBundle { formatVersion: number; @@ -53,9 +54,18 @@ export interface BundleThread { endLine: number; status: ThreadStatus; anchorContent: string | null; + /** Where this finding already is on the forge; absent for one that has never been sent. */ + posted?: BundlePosted; comments: BundleComment[]; } +/** A finding's forge review, as the machine that sent it knew it. */ +export interface BundlePosted { + reviewUrl: string | null; + headSha: string | null; + githubCommentId: number | null; +} + export interface BundleComment { author: CommentAuthor; body: string; @@ -121,6 +131,7 @@ function bundleThread(value: unknown, label: string): BundleThread { if (comments.length === 0) { throw new FieldError(`${label}.comments must not be empty`); } + const posted = bundlePosted(obj.posted, `${label}.posted`); return { filePath: str(obj.filePath, `${label}.filePath`), side: member(obj.side, `${label}.side`, COMMENT_SIDES), @@ -128,10 +139,23 @@ function bundleThread(value: unknown, label: string): BundleThread { ...lineRange(obj, 0, label), status: member(obj.status, `${label}.status`, THREAD_STATUSES), anchorContent: optStr(obj.anchorContent, `${label}.anchorContent`) ?? null, + ...(posted ? { posted } : {}), comments, }; } +function bundlePosted(value: unknown, label: string): BundlePosted | null { + if (value == null) { + return null; + } + const obj = record(value, label); + return { + reviewUrl: optStr(obj.reviewUrl, `${label}.reviewUrl`) ?? null, + headSha: optStr(obj.headSha, `${label}.headSha`) ?? null, + githubCommentId: optInt(obj.githubCommentId, `${label}.githubCommentId`, 1) ?? null, + }; +} + function bundleComment(value: unknown, label: string): BundleComment { const obj = record(value, label); return { diff --git a/packages/api/tests/bundle.test.ts b/packages/api/tests/bundle.test.ts index 91a52a9e..87b18228 100644 --- a/packages/api/tests/bundle.test.ts +++ b/packages/api/tests/bundle.test.ts @@ -84,6 +84,42 @@ describe('parseReviewBundle', () => { expect(result.value.tours[0].steps[0].annotation).toBe(''); }); + it('carries what a finding was already posted as, and takes a thread without it', () => { + const input = validBundle(); + (input.threads as Record[])[0].posted = { + reviewUrl: 'https://github.com/o/r/pull/12#pullrequestreview-9', + headSha: 'a'.repeat(40), + githubCommentId: 900, + }; + const result = parseReviewBundle(input); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.value.threads[0].posted).toEqual({ + reviewUrl: 'https://github.com/o/r/pull/12#pullrequestreview-9', + headSha: 'a'.repeat(40), + githubCommentId: 900, + }); + // A thread nobody has posted says nothing about a review at all. + expect('posted' in result.value.threads[1]).toBe(false); + }); + + it('takes a posted review that has forgotten the details, and refuses a broken one', () => { + const bare = validBundle(); + (bare.threads as Record[])[0].posted = {}; + const result = parseReviewBundle(bare); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.value.threads[0].posted).toEqual({ reviewUrl: null, headSha: null, githubCommentId: null }); + + const wrong = validBundle(); + (wrong.threads as Record[])[0].posted = { githubCommentId: 0 }; + expect(errorOf(wrong)).toBe('threads[0].posted.githubCommentId must be an integer >= 1'); + + const notAnObject = validBundle(); + (notAnObject.threads as Record[])[0].posted = 'yes'; + expect(errorOf(notAnObject)).toBe('threads[0].posted must be an object'); + }); + it('rejects a bundle from a newer format', () => { expect(errorOf({ ...validBundle(), formatVersion: BUNDLE_FORMAT_VERSION + 1 })) .toContain('newer than this diffity understands'); diff --git a/packages/cli/package.json b/packages/cli/package.json index f0834a40..efc1cffe 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.30", + "version": "0.10.31", "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/agent.ts b/packages/cli/src/agent.ts index 9eb906e2..764e76f9 100644 --- a/packages/cli/src/agent.ts +++ b/packages/cli/src/agent.ts @@ -11,6 +11,7 @@ import { addReply, updateThreadStatus, editComment, + markThreadsSubmitted, type Thread, } from './threads.js'; import { @@ -705,6 +706,24 @@ Examples: console.log(pc.green('Tour marked as ready')); }); + // Not for a person to run: the inbox daemon calls this after posting the findings behind an + // alert itself, so the session it hands the reviewer shows those as already sent. + agent + .command('mark-posted', { hidden: true }) + .description('Record that these threads are already on the forge') + .argument('', 'Thread ids, each optionally =') + .option('--review-url ', 'The review they went out in') + .option('--head-sha ', 'The commit they were posted against') + .action(async (threads: string[], opts: { reviewUrl?: string; headSha?: string }) => { + await requireSession(agent.opts().session); + const sent = threads.map(parsePostedThread).map(({ id, githubCommentId }) => ({ + threadId: resolveThreadId(id).id, + ...(githubCommentId === null ? {} : { githubCommentId }), + })); + markThreadsSubmitted(sent, { reviewUrl: opts.reviewUrl ?? null, headSha: opts.headSha ?? null }); + console.log(pc.green(`Marked ${sent.length} thread(s) as posted`)); + }); + agent .command('export-bundle') .description('Write the session\'s threads and tours as a portable review bundle (JSON)') @@ -772,6 +791,20 @@ Examples: }); } +/** `` or `=`, as `mark-posted` takes its arguments. */ +function parsePostedThread(argument: string): { id: string; githubCommentId: number | null } { + const [id, commentId] = argument.split('='); + if (commentId === undefined) { + return { id, githubCommentId: null }; + } + const parsed = Number(commentId); + if (!Number.isInteger(parsed) || parsed < 1) { + console.error(pc.red(`Error: "${argument}" does not name a forge comment id`)); + process.exit(1); + } + return { id, githubCommentId: parsed }; +} + function positiveInteger(value: string): number { const parsed = Number(value); if (!Number.isInteger(parsed) || parsed < 1) { diff --git a/packages/cli/src/bundle.ts b/packages/cli/src/bundle.ts index 23ad9d72..dca0d2d5 100644 --- a/packages/cli/src/bundle.ts +++ b/packages/cli/src/bundle.ts @@ -9,7 +9,7 @@ import { getCommitHash, resolveBaseRef } from '@diffity/git'; import { detectRemote } from '@diffity/github'; import { getDb } from './db.js'; import type { Session } from './session.js'; -import { addReply, createThread, getThreadsForSession, updateThreadStatus, type Thread } from './threads.js'; +import { addReply, createThread, getThreadsForSession, markThreadsSubmitted, updateThreadStatus, type Thread } from './threads.js'; import { addTourStep, createTour, getToursForSession, updateTourStatus, type Tour } from './tours.js'; export interface BundleOrigin { @@ -38,6 +38,15 @@ export function buildBundle(session: Session, origin: BundleOrigin): ReviewBundl endLine: thread.endLine, status: thread.status, anchorContent: thread.anchorContent, + ...(thread.submittedAt + ? { + posted: { + reviewUrl: thread.submittedReviewUrl, + headSha: thread.submittedHeadSha, + githubCommentId: thread.githubCommentId, + }, + } + : {}), comments: thread.comments.map(comment => ({ author: comment.author, body: comment.body, @@ -181,6 +190,12 @@ function addBundle(session: Session, bundle: ReviewBundle): ImportOutcome { if (incoming.status !== 'open') { updateThreadStatus(thread.id, incoming.status); } + if (incoming.posted) { + markThreadsSubmitted( + [{ threadId: thread.id, githubCommentId: incoming.posted.githubCommentId ?? undefined }], + { reviewUrl: incoming.posted.reviewUrl, headSha: incoming.posted.headSha }, + ); + } outcome.threadsCreated++; } diff --git a/packages/cli/src/inbox/config.ts b/packages/cli/src/inbox/config.ts index 0de3a24a..85e551ae 100644 --- a/packages/cli/src/inbox/config.ts +++ b/packages/cli/src/inbox/config.ts @@ -66,6 +66,15 @@ export interface InboxConfig { * as needing the reviewer now, alongside whatever the agent made of `alertWhen`. */ alertPaths: string[]; + /** + * Whether the daemon posts the findings the agent named behind an `alertWhen` alert to the pull + * request itself, as a comment review in the reviewer's name, at most once per head. An alert + * raised by `alertPaths` posts nothing: it is the reviewer's own rule about the paths, and there + * is nothing in it to tell the author. + */ + postAlerts: boolean; + /** Opens every posted comment, so nobody reads one as a verdict a human has stood behind. */ + postPrefix: string; agent: AgentConfig; validate: ValidateConfig; /** Whether a pull request waits for its CI to pass before an agent is spent on it. */ @@ -94,6 +103,8 @@ export const DEFAULT_INBOX_CONFIG: InboxConfig = { skipTitles: [], alertWhen: '', alertPaths: [], + postAlerts: false, + postPrefix: '[Automated AI pre-review, not yet checked by human]', agent: { model: null, effort: null, mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, validate: { model: null, timeoutMinutes: 15, maxBudgetUsd: null }, waitForCi: false, @@ -166,6 +177,23 @@ export function parseInboxConfig(raw: unknown, source = 'inbox config'): InboxCo } config.alertPaths = (obj.alertPaths as string[]).map(glob => glob.trim()); } + if (obj.postAlerts !== undefined) { + if (typeof obj.postAlerts !== 'boolean') { + throw new Error(`${source}: postAlerts must be true or false`); + } + config.postAlerts = obj.postAlerts; + } + if (obj.postPrefix !== undefined) { + if (typeof obj.postPrefix !== 'string') { + throw new Error(`${source}: postPrefix must be a string`); + } + config.postPrefix = obj.postPrefix; + } + // Nothing goes to a pull request unprefixed: the prefix is what tells the author no human has + // stood behind the finding yet. + if (config.postAlerts && config.postPrefix.trim() === '') { + throw new Error(`${source}: postPrefix must not be empty when postAlerts is on`); + } if (obj.agent !== undefined) { config.agent = parseAgentConfig(obj.agent, source); } @@ -279,7 +307,7 @@ function parseValidateConfig(raw: unknown, source: string): ValidateConfig { } /** 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 ccbf3edb..40600fc1 100644 --- a/packages/cli/src/inbox/daemon.ts +++ b/packages/cli/src/inbox/daemon.ts @@ -45,7 +45,7 @@ export interface DaemonOptions { /** Who parks on an opened review; defaults to the real attendants. Tests override it. */ attendants?: AttendantHost; /** How one pull request is prepared; defaults to the real preparation. Tests override it. */ - prepare?: (snapshot: PrSnapshot, opts: { bumped: boolean }) => Promise; + prepare?: (snapshot: PrSnapshot, opts: { bumped: boolean; alreadyPostedHead: string | null }) => Promise; /** Where the prepares register what they have running, for the shutdown to stop; its own by default. */ inflight?: Inflight; /** Where the page's settings are written; without it they change the running daemon only. */ @@ -80,6 +80,7 @@ export function settingsHost(config: InboxConfig, configPath: string | undefined return { get: () => ({ filter: config.filter, skipTitles: config.skipTitles, alertWhen: config.alertWhen, alertPaths: config.alertPaths, + postAlerts: config.postAlerts, postPrefix: config.postPrefix, maxPrepared: config.maxPrepared, pollMinutes: config.pollMinutes, live: config.live, liveTimeoutMinutes: config.liveTimeoutMinutes, prepareTimeoutMinutes: config.prepareTimeoutMinutes, waitForCi: config.waitForCi, agent: config.agent, validate: config.validate, @@ -129,7 +130,7 @@ export async function runDaemon( const pausedUntil = () => store.pausedUntil(new Date().toISOString()); const deps = { forge: options.forge ?? realForge, - prepare: options.prepare ?? ((snapshot: PrSnapshot, opts: { bumped: boolean }) => preparePr(snapshot, config, prepareDeps, opts)), + prepare: options.prepare ?? ((snapshot: PrSnapshot, opts: { bumped: boolean; alreadyPostedHead: string | null }) => preparePr(snapshot, config, prepareDeps, opts)), removeWorktree: (worktree: string, repo: string) => reclaimWorktree(config, worktree, repo), log, now: () => new Date().toISOString(), diff --git a/packages/cli/src/inbox/page.ts b/packages/cli/src/inbox/page.ts index a13a856d..b5dcd157 100644 --- a/packages/cli/src/inbox/page.ts +++ b/packages/cli/src/inbox/page.ts @@ -41,6 +41,9 @@ export function inboxPage(): string { .settings textarea { display: block; width: 100%; margin-top: 4px; font: inherit; font-size: 13px; color: var(--ink); background: var(--panel); border: 1px solid var(--line); border-radius: 8px; padding: 8px 10px; resize: vertical; } .settings-row { display: flex; align-items: center; gap: 12px; margin-top: 10px; } + .settings-row .check { display: flex; align-items: center; gap: 8px; margin-top: 0; white-space: nowrap; } + .settings-row input[type=text] { flex: 1; min-width: 120px; font: inherit; font-size: 13px; color: var(--ink); + background: var(--panel); border: 1px solid var(--line); border-radius: 8px; padding: 6px 10px; } .settings-grid { display: grid; grid-template-columns: repeat(auto-fit, minmax(180px, 1fr)); gap: 10px 16px; margin-top: 4px; } .settings-grid input, .settings-grid select { display: block; width: 100%; margin-top: 4px; font: inherit; font-size: 13px; color: var(--ink); background: var(--panel); border: 1px solid var(--line); border-radius: 8px; padding: 6px 10px; } @@ -149,6 +152,10 @@ export function inboxPage(): string { +
+ + +
@@ -225,6 +232,11 @@ export function inboxPage(): string { return named ? named + ' finding' + (named === 1 ? '' : 's') : ''; } + /** That the daemon has already put the alert findings on the pull request, and when. */ + function postedLabel(r) { + return r.autoPosted ? 'posted to the pull request \\u00b7 ' + hhmm(r.autoPosted.at) : ''; + } + /** What the agent runs behind a prepared review came to; the hover has them one by one. */ function spendLabel(r) { return r.spend ? Math.round(r.spend.minutes) + ' min \\u00b7 ' + money(r.spend.costUsd) : ''; @@ -251,6 +263,9 @@ export function inboxPage(): string { row.href = r.openUrl; row.target = '_blank'; row.rel = 'noopener'; + // The card is already a link to the review, so the posted review's own URL goes in the hover + // rather than into a link inside a link. + if (r.autoPosted && r.autoPosted.url) row.title = 'the alert findings were posted to ' + r.autoPosted.url; row.innerHTML = '' + '' + sizeLabel(r) + '' + @@ -258,7 +273,7 @@ export function inboxPage(): string { '
' + esc(r.repo) + '#' + r.number + ' ' + '' + esc(r.title) + '
' + metaLine(['by ' + esc(r.author), r.changedFiles + ' file(s)', esc(r.summary || ''), esc(r.alert || ''), - findingsLabel(r), spendLabel(r), times(r)], r.spend ? r.spend.detail : '') + '
' + + findingsLabel(r), postedLabel(r), spendLabel(r), times(r)], r.spend ? r.spend.detail : '') + '' + (r.alert ? 'alert' : '') + (r.stale ? 'stale' : '') + 'open \\u2197'; @@ -388,7 +403,8 @@ export function inboxPage(): string { if (known.has(key)) continue; // With words on what matters, only what the agent flagged is worth interrupting for. if (settings.alertWhen.trim() && !r.alert) continue; - const body = [r.title, r.summary, r.alert].filter(Boolean).join('\\n'); + const body = [r.title, r.summary, r.alert, r.autoPosted ? 'posted to the pull request' : ''] + .filter(Boolean).join('\\n'); const n = new Notification(r.repo + '#' + r.number + ' is ready to review', { body, tag: r.id }); n.onclick = () => { window.open(r.openUrl, '_blank'); n.close(); }; } @@ -434,6 +450,8 @@ export function inboxPage(): string { el('skipTitles').value = (settings.skipTitles || []).join('\\n'); el('alertWhen').value = settings.alertWhen; el('alertPaths').value = (settings.alertPaths || []).join('\\n'); + el('postAlerts').checked = settings.postAlerts; + el('postPrefix').value = settings.postPrefix || ''; for (const key of ['maxPrepared', 'pollMinutes', 'prepareTimeoutMinutes', 'liveTimeoutMinutes']) el(key).value = settings[key]; el('live').checked = settings.live; el('waitForCi').checked = settings.waitForCi; @@ -459,6 +477,8 @@ export function inboxPage(): string { skipTitles: el('skipTitles').value.split('\\n').map(line => line.trim()).filter(Boolean), alertWhen: el('alertWhen').value, alertPaths: el('alertPaths').value.split('\\n').map(line => line.trim()).filter(Boolean), + postAlerts: el('postAlerts').checked, + postPrefix: el('postPrefix').value.trim(), maxPrepared: Number(el('maxPrepared').value), pollMinutes: Number(el('pollMinutes').value), prepareTimeoutMinutes: Number(el('prepareTimeoutMinutes').value), diff --git a/packages/cli/src/inbox/prepare.ts b/packages/cli/src/inbox/prepare.ts index 5494e945..b1274359 100644 --- a/packages/cli/src/inbox/prepare.ts +++ b/packages/cli/src/inbox/prepare.ts @@ -1,6 +1,8 @@ import { join } from 'node:path'; -import type { PrSnapshot } from '@diffity/github'; +import { GENERAL_THREAD_FILE_PATH } from '@diffity/api'; +import type { PrComment, PrSnapshot, ReviewResult, ReviewSubmission } from '@diffity/github'; import type { InboxConfig } from './config.js'; +import { prId } from './store.js'; import { parseAgentOutput, rateLimitOf, type RunStats } from './agent-output.js'; import { inboxDir } from './paths.js'; import { localHhMm } from './runs.js'; @@ -31,6 +33,25 @@ export interface ExportOpts { outPath: string; } +/** One review for the forge, as the daemon itself posts it — never through the agent. */ +export interface PostReviewOpts { + owner: string; + repo: string; + prNumber: number; + headSha: string; + submission: ReviewSubmission; +} + +/** What reached the forge, so the session the reviewer opens shows those findings as sent. */ +export interface MarkPostedOpts { + worktree: string; + threadIds: string[]; + /** The forge comment each thread exists as, where the forge said which. */ + commentIds: { threadId: string; githubCommentId: number }[]; + reviewUrl: string | null; + headSha: string; +} + /** The side effects the preparer needs, injected so the orchestration itself is testable. */ export interface PrepareDeps { startServer(worktree: string, diffRef: string): Promise; @@ -41,7 +62,11 @@ export interface PrepareDeps { runAgent(opts: RunAgentOpts): Promise<{ stdout: string; timedOut: boolean }>; /** The threads the drafting agent left in the session over this worktree. */ listThreads(worktree: string): Promise; + /** The daemon's own call to the forge, with its credentials — this is never the agent's. */ + postReview(opts: PostReviewOpts): Promise; + markPosted(opts: MarkPostedOpts): void | Promise; exportBundle(opts: ExportOpts): void | Promise; + log(message: string): void; now(): string; } @@ -71,8 +96,17 @@ export interface ValidateRun extends RunLog { note: string | null; } +/** The review the daemon put the alert findings on the pull request in. */ +export interface PostedReview { + at: string; + headSha: string; + url: string | null; + /** How many of the posted comments the forge gave an id back for. */ + commentIds: number; +} + export type PrepareResult = - | { kind: 'prepared'; headSha: string; bundlePath: string; worktree: string; logPath: string; at: string; summary: string | null; alert: string | null; alertFindings: string[]; run: RunLog; validation: Validation; validateRun: ValidateRun | null } + | { kind: 'prepared'; headSha: string; bundlePath: string; worktree: string; logPath: string; at: string; summary: string | null; alert: string | null; alertFindings: string[]; posted: PostedReview | null; run: RunLog; validation: Validation; validateRun: ValidateRun | null } | { kind: 'skipped'; reason: string; logPath: string; run: RunLog } | { kind: 'failed'; reason: string; failure: PrepareFailure; worktree: string | null; logPath: string | null; run: RunLog; resetsAt?: string | null }; @@ -99,6 +133,8 @@ export interface PrepareOpts { * an earlier review sets it; the daemon always takes the current head. */ pinHead?: string; + /** The head the alert findings have already been posted for, so no head is posted to twice. */ + alreadyPostedHead?: string | null; } export async function preparePr(snapshot: PrSnapshot, config: InboxConfig, deps: PrepareDeps, opts: PrepareOpts = {}): Promise { @@ -180,6 +216,13 @@ export async function preparePr(snapshot: PrSnapshot, config: InboxConfig, deps: ? 'not-needed' : validateRun.outcome === 'validated' ? 'validated' : 'unchecked'; + // Before the bundle, so the threads it carries already know they are on the pull request; the + // worktree's own server is still up, which is what the thread listing reads through. + const posted = await postAlertFindings(snapshot, config, deps, { + worktree: dest, head, alert: verdict.alert, alertFindings: verdict.alertFindings, + alreadyPostedHead: opts.alreadyPostedHead ?? null, + }); + // The head actually checked out, which may be newer than the snapshot if the author pushed // between the search and the fetch; recording it keeps the next tick from calling it stale. const bundlePath = join(bundlesDir(), `${snapshot.owner}-${snapshot.repo}-${snapshot.number}-${head.slice(0, 12)}.json`); @@ -192,7 +235,7 @@ export async function preparePr(snapshot: PrSnapshot, config: InboxConfig, deps: return { kind: 'prepared', headSha: head, bundlePath, worktree: dest, logPath, at: deps.now(), summary: withValidation(summarizeBundleFile(bundlePath), validation), alert: verdict.alert, - alertFindings: verdict.alertFindings, run, validation, validateRun, + alertFindings: verdict.alertFindings, posted, run, validation, validateRun, }; } catch (err) { return { kind: 'failed', failure: 'agent', reason: err instanceof Error ? err.message : String(err), worktree: dest, logPath, run }; @@ -201,6 +244,102 @@ export async function preparePr(snapshot: PrSnapshot, config: InboxConfig, deps: } } +/** + * Puts the findings the agent named behind its alert on the pull request, as one `COMMENT` review + * in the reviewer's name, every comment opening with the configured prefix so nobody reads it as a + * verdict a human has stood behind. Null when nothing was posted: the setting is off, the agent + * raised no alert of its own, this head has been posted to already, or the post did not go through + * — the review is prepared either way, and the reviewer still has the alert and the findings. + */ +async function postAlertFindings( + snapshot: PrSnapshot, + config: InboxConfig, + deps: PrepareDeps, + ctx: { worktree: string; head: string; alert: string | null; alertFindings: string[]; alreadyPostedHead: string | null }, +): Promise { + if (!config.postAlerts || ctx.alert === null || ctx.alreadyPostedHead === ctx.head) { + return null; + } + const id = prId(snapshot); + try { + const comments = alertComments(await deps.listThreads(ctx.worktree), ctx.alertFindings, config.postPrefix); + const result = await deps.postReview({ + owner: snapshot.owner, + repo: snapshot.repo, + prNumber: snapshot.number, + headSha: ctx.head, + // Never a verdict: the reviewer has not read this yet, and only they approve or request changes. + submission: { event: 'COMMENT', body: `${config.postPrefix} ${ctx.alert}`, comments }, + }); + if (result.reviewUrl === null) { + deps.log(`could not post alert findings to ${id}: ${result.errors.join('; ') || 'the forge created no review'}`); + return null; + } + deps.log(`posted ${result.submitted} alert finding(s) to ${id} — ${result.reviewUrl}`); + if (result.errors.length > 0) { + deps.log(`${id}: ${result.errors.length} alert finding(s) were left off the review — ${result.errors.join('; ')}`); + } + await markPostedThreads(deps, { worktree: ctx.worktree, headSha: ctx.head, id, result }); + return { at: deps.now(), headSha: ctx.head, url: result.reviewUrl, commentIds: result.commentIds.length }; + } catch (err) { + deps.log(`could not post alert findings to ${id}: ${err instanceof Error ? err.message : err}`); + return null; + } +} + +/** + * The named findings as forge comments, in the order the agent named them: the ones still open + * after the checking pass — a dismissed or resolved finding is settled and does not go out — each + * body opening with the prefix on a line of its own. The general summary is not a finding and is + * never posted as one; the review's body carries the reason instead. + */ +function alertComments(threads: ReviewThread[], named: string[], prefix: string): PrComment[] { + const postable = threads.filter(thread => thread.status === 'open' && thread.filePath !== GENERAL_THREAD_FILE_PATH); + const comments: PrComment[] = []; + for (const id of named) { + // The agent names findings by the 8-character prefix `agent comment` printed, or in full. + const thread = postable.find(one => one.threadId === id || one.threadId.startsWith(id)); + const body = thread?.comments[0]?.body; + if (!thread || !body || comments.some(already => already.threadId === thread.threadId)) { + continue; + } + comments.push({ + threadId: thread.threadId, + filePath: thread.filePath, + side: thread.side === 'old' ? 'LEFT' : 'RIGHT', + startLine: thread.startLine === thread.endLine ? null : thread.startLine, + endLine: thread.endLine, + body: `${prefix}\n\n${body}`, + }); + } + return comments; +} + +/** + * Marks what left the machine as sent, so the session the reviewer opens does not offer to send it + * again. A marking that fails is worth saying so and no more: the comments are on the pull request + * whether or not the local session knows it. + */ +async function markPostedThreads( + deps: PrepareDeps, + ctx: { worktree: string; headSha: string; id: string; result: ReviewResult }, +): Promise { + if (ctx.result.submittedThreadIds.length === 0) { + return; + } + try { + await deps.markPosted({ + worktree: ctx.worktree, + threadIds: ctx.result.submittedThreadIds, + commentIds: ctx.result.commentIds, + reviewUrl: ctx.result.reviewUrl, + headSha: ctx.headSha, + }); + } catch (err) { + deps.log(`${ctx.id}: the posted findings could not be marked as sent — ${err instanceof Error ? err.message : err}`); + } +} + /** * The second pass over the draft: the checking model reads the findings that would hold up a merge * against the code, and amends or dismisses the ones that do not stand. Null when there was diff --git a/packages/cli/src/inbox/runtime.ts b/packages/cli/src/inbox/runtime.ts index 1452fd86..cf32f0ea 100644 --- a/packages/cli/src/inbox/runtime.ts +++ b/packages/cli/src/inbox/runtime.ts @@ -1,9 +1,10 @@ import { spawn, execFile } from 'node:child_process'; import { promisify } from 'node:util'; import type { LiveRequest } from '@diffity/api'; +import { createReview } from '@diffity/github'; import { createWriteStream, mkdirSync, readFileSync, rmSync, type WriteStream } from 'node:fs'; import { dirname, join } from 'node:path'; -import type { ExportOpts, PrepareDeps, RunAgentOpts, ServerHandle } from './prepare.js'; +import type { ExportOpts, MarkPostedOpts, PrepareDeps, RunAgentOpts, ServerHandle } from './prepare.js'; import type { InboxConfig } from './config.js'; import { buildAgentArgv, skillBody } from './agent-argv.js'; import { parseAgentOutput } from './agent-output.js'; @@ -51,7 +52,12 @@ export function realPrepareDeps(nodePath: string, entry: string, dataDirFor: (wo }), runAgent: opts => runAgent(opts, dataDirFor(opts.cwd), config.agent.mcpAllow, inflight), listThreads: worktree => listThreads(nodePath, entry, worktree, dataDirFor(worktree)), + // In this process, with the reviewer's own credentials: posting the alert findings is the + // daemon's own act, after the agent has finished, and never something the agent can reach. + postReview: opts => createReview(opts.owner, opts.repo, opts.prNumber, opts.headSha, opts.submission), + markPosted: opts => markPosted(nodePath, entry, opts, dataDirFor(opts.worktree)), exportBundle: opts => exportBundle(nodePath, entry, opts, dataDirFor(opts.worktree)), + log, now: () => new Date().toISOString(), }; } @@ -320,6 +326,24 @@ async function listThreads(nodePath: string, entry: string, worktree: string, da return parseThreadList(stdout); } +/** The posted findings marked as sent in the pull request's own diffity data directory. */ +async function markPosted(nodePath: string, entry: string, opts: MarkPostedOpts, dataDir: string): Promise { + const idOf = new Map(opts.commentIds.map(comment => [comment.threadId, comment.githubCommentId])); + const args = opts.threadIds.map(threadId => { + const commentId = idOf.get(threadId); + return commentId === undefined ? threadId : `${threadId}=${commentId}`; + }); + await promisify(execFile)( + nodePath, + [ + entry, '--repo', opts.worktree, 'agent', 'mark-posted', '--head-sha', opts.headSha, + ...(opts.reviewUrl === null ? [] : ['--review-url', opts.reviewUrl]), + ...args, + ], + { env: { ...process.env, DIFFITY_DATA_DIR: dataDir } }, + ); +} + async function exportBundle(nodePath: string, entry: string, opts: ExportOpts, dataDir: string): Promise { mkdirSync(dirname(opts.outPath), { recursive: true }); await promisify(execFile)( diff --git a/packages/cli/src/inbox/settings.ts b/packages/cli/src/inbox/settings.ts index 1d323af1..87284cd7 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', 'skipTitles', 'alertWhen', 'alertPaths', 'maxPrepared', 'pollMinutes', 'live', 'liveTimeoutMinutes', 'prepareTimeoutMinutes', 'waitForCi', 'agent', 'validate'] as const; +export const SETTINGS_KEYS = ['filter', 'skipTitles', 'alertWhen', 'alertPaths', 'postAlerts', 'postPrefix', 'maxPrepared', 'pollMinutes', 'live', 'liveTimeoutMinutes', 'prepareTimeoutMinutes', 'waitForCi', 'agent', 'validate'] as const; export type SettingsPatch = | { ok: true; settings: InboxSettings } @@ -29,7 +29,7 @@ export function parseSettingsPatch(body: string): SettingsPatch { return { ok: false, message: `${key} is missing` }; } } - for (const key of ['filter', 'alertWhen'] as const) { + for (const key of ['filter', 'alertWhen', 'postPrefix'] as const) { if (typeof obj[key] !== 'string') { return { ok: false, message: `${key} must be a string` }; } diff --git a/packages/cli/src/inbox/store.ts b/packages/cli/src/inbox/store.ts index 8ca88126..00b60b10 100644 --- a/packages/cli/src/inbox/store.ts +++ b/packages/cli/src/inbox/store.ts @@ -56,6 +56,8 @@ export interface InboxPr { alert: string | null; /** The threads the agent named as the reason for the alert; empty when it named none. */ alertFindings: string[]; + /** The review the daemon posted the alert findings in, or null when it has posted none. */ + autoPosted: AutoPosted | null; bundlePath: string | null; worktreePath: string | null; logPath: string | null; @@ -77,6 +79,16 @@ export interface Prepared { alertFindings: string[]; } +/** + * The daemon's own posting of the findings behind an alert: when it went out, the head it was + * posted against — nothing is posted twice for one head — and the review it created. + */ +export interface AutoPosted { + at: string; + headSha: string; + url: string | null; +} + /** A review diffity posted to the forge: the head it was posted against, and what it said. */ export interface Handled { headSha: string; @@ -238,6 +250,9 @@ export class InboxStore { summary TEXT, alert TEXT, alert_findings TEXT, + auto_posted_at TEXT, + auto_posted_head_sha TEXT, + auto_posted_url TEXT, ci_state TEXT ) `); @@ -275,7 +290,7 @@ export class InboxStore { this.db.exec('CREATE INDEX IF NOT EXISTS inbox_handled_pr_at ON inbox_handled (pr_id, 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', 'alert_findings TEXT', 'ci_state TEXT']) { + for (const column of ['attempts INTEGER NOT NULL DEFAULT 0', 'created_at TEXT', 'updated_at TEXT', 'bumped_at TEXT', 'summary TEXT', 'alert TEXT', 'alert_findings TEXT', 'auto_posted_at TEXT', 'auto_posted_head_sha TEXT', 'auto_posted_url TEXT', 'ci_state TEXT']) { try { this.db.exec(`ALTER TABLE inbox_prs ADD COLUMN ${column}`); } catch (err) { @@ -370,6 +385,15 @@ export class InboxStore { ); } + /** + * Records that the daemon itself put the alert findings on the pull request. One row, overwritten + * at the next head: what it answers is "has this head been posted to already". + */ + markAutoPosted(id: string, posted: AutoPosted): void { + this.db.prepare('UPDATE inbox_prs SET auto_posted_at = ?, auto_posted_head_sha = ?, auto_posted_url = ? WHERE id = ?') + .run(posted.at, posted.headSha, posted.url, id); + } + /** * A review diffity posted for a pull request. The log is append-only: a pull request can be * handled at one head and then again at the next. @@ -502,6 +526,9 @@ interface Row { summary: string | null; alert: string | null; alert_findings: string | null; + auto_posted_at: string | null; + auto_posted_head_sha: string | null; + auto_posted_url: string | null; ci_state: string | null; } @@ -533,6 +560,9 @@ function rowToPr(row: Row): InboxPr { summary: row.summary, alert: row.alert, alertFindings: parseAlertFindings(row.alert_findings), + autoPosted: row.auto_posted_at != null && row.auto_posted_head_sha != null + ? { at: row.auto_posted_at, headSha: row.auto_posted_head_sha, url: row.auto_posted_url ?? null } + : null, bundlePath: row.bundle_path, worktreePath: row.worktree_path, logPath: row.log_path, diff --git a/packages/cli/src/inbox/tick.ts b/packages/cli/src/inbox/tick.ts index dbecc99e..c6d5cfe9 100644 --- a/packages/cli/src/inbox/tick.ts +++ b/packages/cli/src/inbox/tick.ts @@ -1,7 +1,7 @@ import type { PrRef, PrSnapshot } from '@diffity/github'; import { alertForPaths } from './paths-alert.js'; import { reconcile } from './reconcile.js'; -import { isRetired, prId, prIdToRef, runRecordOf, type InboxPr, type InboxStore, type RunOutcome } from './store.js'; +import { isRetired, prId, prIdToRef, runRecordOf, type Handled, type InboxPr, type InboxStore, type RunOutcome } from './store.js'; import { localHhMm } from './runs.js'; import type { PrepareResult } from './prepare.js'; @@ -15,7 +15,7 @@ export interface Forge { export interface TickDeps { forge: Forge; /** Prepares one pull request; the daemon passes the real preparer, a test a fake. */ - prepare(snapshot: PrSnapshot, opts: { bumped: boolean }): Promise; + prepare(snapshot: PrSnapshot, opts: { bumped: boolean; alreadyPostedHead: string | null }): Promise; removeWorktree(worktree: string, repo: string): void | Promise; log(message: string): void; now(): string; @@ -84,6 +84,9 @@ export async function runTick(store: InboxStore, deps: TickDeps): Promise // Rows the search no longer returns: retired against their latest detail, and their worktrees // reclaimed. A closed pull request may not be searchable at all, so it is asked about directly. + // One the daemon posted the alert findings to itself is the exception: that post is what + // withdrew the review request, so the pull request is still the reviewer's to review and is + // reconciled as though the search had listed it. for (const pr of store.all()) { if (requestedIds.has(pr.id) || isRetired(pr.status) || deps.inFlight.has(pr.id)) { continue; @@ -92,11 +95,23 @@ export async function runTick(store: InboxStore, deps: TickDeps): Promise if (!snapshot) { continue; } - store.observe(snapshot, false, deps.now()); - const transition = reconcile({ existing: pr, snapshot, requested: false, viewerLogin, handled: store.latestHandled(pr.id) }); + const handled = store.latestHandled(pr.id); + const requested = autoPostConsumedRequest(pr, handled) && snapshot.state === 'OPEN'; + store.observe(snapshot, requested, deps.now()); + // The CI hold and the title patterns matter only where a preparation could follow, which is + // the row still asking for the reviewer. + const transition = reconcile({ + existing: pr, snapshot, requested, viewerLogin, handled, + waitForCi: deps.waitForCi, skipTitles: deps.skipTitles, + }); if (transition) { store.setStatus(pr.id, transition.status, transition.reason); - if (pr.worktreePath) { + if (transition.prepare) { + toPrepare.push(snapshot); + } + // The worktree goes only when nothing will be opened from it again: a row that stays + // openable, or is about to be prepared afresh, needs the checkout it has. + if (pr.worktreePath && (isRetired(transition.status) || transition.status === 'handled')) { await deps.removeWorktree(pr.worktreePath, pr.repo); store.setPaths(pr.id, { worktreePath: null }); } @@ -182,6 +197,15 @@ export async function runTick(store: InboxStore, deps: TickDeps): Promise } } +/** + * Whether the reviewer's own review request was consumed by the daemon's post rather than by the + * reviewer. Once they have posted a review of their own, the pull request is theirs no longer and + * follows the ordinary handled rules. + */ +function autoPostConsumedRequest(pr: InboxPr, handled: Handled | null): boolean { + return pr.autoPosted !== null && (handled === null || handled.at < pr.autoPosted.at); +} + /** Prepared reviews waiting for the reviewer, stale ones included: they are still openable. */ function countReady(store: InboxStore): number { return store.all().filter(pr => pr.status === 'prepared' || pr.status === 'stale').length; @@ -243,13 +267,14 @@ async function prepareOne(store: InboxStore, snapshot: PrSnapshot, deps: TickDep if (deps.inFlight.has(id)) { return; } + const existing = store.get(id); deps.inFlight.add(id); store.setStatus(id, 'preparing', null); deps.log(`preparing ${id} — ${snapshot.title}`); let result: PrepareResult; try { - result = await deps.prepare(snapshot, { bumped }); + result = await deps.prepare(snapshot, { bumped, alreadyPostedHead: existing?.autoPosted?.headSha ?? null }); } finally { deps.inFlight.delete(id); } @@ -270,6 +295,9 @@ async function prepareOne(store: InboxStore, snapshot: PrSnapshot, deps: TickDep // Only the agent names findings, so a path alert stands on its own with none. alertFindings: result.alertFindings, }); + if (result.posted) { + store.markAutoPosted(id, result.posted); + } deps.log(`prepared ${id}`); if (result.validateRun?.note) { deps.log(`${id}: the drafted findings went unchecked — ${result.validateRun.note}`); diff --git a/packages/cli/src/inbox/view.ts b/packages/cli/src/inbox/view.ts index 19d852c3..562a93bb 100644 --- a/packages/cli/src/inbox/view.ts +++ b/packages/cli/src/inbox/view.ts @@ -1,5 +1,5 @@ import type { CiState } from '@diffity/github'; -import { isRetired, type Handled, type InboxPr, type InboxStore, type RunTotals } from './store.js'; +import { isRetired, type AutoPosted, type Handled, type InboxPr, type InboxStore, type RunTotals } from './store.js'; import { costOf, minutesOf, runDetail } from './runs.js'; import { BUMPABLE } from './open.js'; @@ -38,6 +38,8 @@ export interface InboxRow { alert: string | null; /** The findings it named as that reason, by thread id; empty when it named none. */ alertFindings: string[]; + /** The review the daemon posted those findings in, when it did; null when it posted none. */ + autoPosted: AutoPosted | null; openUrl: string | null; /** Where a POST dismisses it; null while it is being prepared, and once it is retired. */ dismissUrl: string | null; @@ -150,6 +152,7 @@ function toRow(pr: InboxPr, openBase: string, store: InboxStore): InboxRow { summary: pr.summary, alert: pr.alert, alertFindings: pr.alertFindings, + autoPosted: pr.autoPosted, openUrl: openable ? `${openBase}/open/${encodeURIComponent(pr.id)}` : null, dismissUrl: pr.status === 'preparing' || pr.status === 'dismissed' || isRetired(pr.status) ? null : `${openBase}/dismiss/${encodeURIComponent(pr.id)}`, prepareUrl: bumpable && pr.bumpedAt === null ? `${openBase}/prepare/${encodeURIComponent(pr.id)}` : null, diff --git a/packages/cli/tests/bundle-roundtrip.test.ts b/packages/cli/tests/bundle-roundtrip.test.ts index bdd8001a..1efb00d2 100644 --- a/packages/cli/tests/bundle-roundtrip.test.ts +++ b/packages/cli/tests/bundle-roundtrip.test.ts @@ -151,6 +151,39 @@ describe('a review bundle', () => { expect(getToursForSession(target.id)).toHaveLength(1); }); + it('carries a posted finding to the session it is imported into, marked as already sent', async () => { + const { buildBundle, importBundle } = await import('../src/bundle.js'); + const { findOrCreateSession } = await import('../src/session.js'); + const { getThreadsForSession, markThreadsSubmitted } = await import('../src/threads.js'); + const session = await preparedSession(); + const posted = getThreadsForSession(session.id).find(thread => thread.startLine === 2)!; + markThreadsSubmitted( + [{ threadId: posted.id, githubCommentId: 900 }], + { reviewUrl: 'https://github.com/o/r/pull/7#pullrequestreview-9', headSha }, + ); + + const bundle = buildBundle(session, { prNumber: 7, generator: 'test' }); + expect(bundle.threads.find(thread => thread.startLine === 2)!.posted).toEqual({ + reviewUrl: 'https://github.com/o/r/pull/7#pullrequestreview-9', headSha, githubCommentId: 900, + }); + // Only what went out carries it; the rest of the review is still the reviewer's to send. + expect(bundle.threads.filter(thread => thread.posted)).toHaveLength(1); + + const cloneDir = join(root, `clone-${repoCount++}`); + execFileSync('git', ['clone', '--quiet', repoDir, cloneDir], { stdio: 'pipe' }); + process.chdir(cloneDir); + const target = findOrCreateSession('work'); + importBundle(target, bundle); + + const threads = getThreadsForSession(target.id); + const imported = threads.find(thread => thread.startLine === 2)!; + expect(imported.submittedAt).toBeTruthy(); + expect(imported.submittedReviewUrl).toBe('https://github.com/o/r/pull/7#pullrequestreview-9'); + expect(imported.submittedHeadSha).toBe(headSha); + expect(imported.githubCommentId).toBe(900); + expect(threads.filter(thread => thread.submittedAt)).toHaveLength(1); + }); + it('keeps the comments\' own timestamps, so replies read back in their original order', async () => { const { buildBundle, importBundle } = await import('../src/bundle.js'); const { findOrCreateSession } = await import('../src/session.js'); diff --git a/packages/cli/tests/inbox-daemon.test.ts b/packages/cli/tests/inbox-daemon.test.ts index eca41e78..56b38bfd 100644 --- a/packages/cli/tests/inbox-daemon.test.ts +++ b/packages/cli/tests/inbox-daemon.test.ts @@ -38,7 +38,7 @@ function snapshot(number: number, additions: number): PrSnapshot { function preparedResult(snapshot: PrSnapshot): PrepareResult { return { kind: 'prepared', headSha: snapshot.headSha, bundlePath: '/b.json', worktree: '/wt', logPath: '/l.log', - at: 'now', summary: '1 P2', alert: null, alertFindings: [], validation: 'not-needed', validateRun: null, + at: 'now', summary: '1 P2', alert: null, alertFindings: [], posted: null, validation: 'not-needed', validateRun: null, run: { startedAt: 'now', endedAt: 'now', stats: null }, }; } @@ -63,7 +63,8 @@ function seedRegistry(pid: number): void { function config(port: number) { return { pollMinutes: 5, port, reposDir: join(root, 'repos'), worktreesDir: join(root, 'inbox', 'worktrees'), - filter: '', skipTitles: [], alertWhen: '', alertPaths: [], agent: agentConfig(), validate: { model: null, timeoutMinutes: 15, maxBudgetUsd: null }, + filter: '', skipTitles: [], alertWhen: '', alertPaths: [], postAlerts: false, + postPrefix: '[not yet checked by human]', agent: agentConfig(), validate: { model: null, timeoutMinutes: 15, maxBudgetUsd: null }, waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, }; } diff --git a/packages/cli/tests/inbox-open.test.ts b/packages/cli/tests/inbox-open.test.ts index cd4199ed..4c30a6b3 100644 --- a/packages/cli/tests/inbox-open.test.ts +++ b/packages/cli/tests/inbox-open.test.ts @@ -21,6 +21,8 @@ function agentConfig(): AgentConfig { return { model: null, effort: null, mcpAllow: [], extraArgs: [], maxBudgetUsd: null }; } +const PREFIX = '[not yet checked by human]'; + function validateConfig(): ValidateConfig { return { model: null, timeoutMinutes: 15, maxBudgetUsd: null }; } @@ -226,7 +228,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: '', skipTitles: [], alertWhen: '', alertPaths: [], agent: agentConfig(), validate: validateConfig(), waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10 }; + const config = { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', skipTitles: [], alertWhen: '', alertPaths: [], postAlerts: false, postPrefix: PREFIX, agent: agentConfig(), validate: validateConfig(), 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 }; @@ -359,12 +361,14 @@ describe('the inbox server routes', () => { try { const before = await (await fetch(`http://127.0.0.1:${port}/api/settings`)).json(); expect(before).toEqual({ - filter: '', skipTitles: [], alertWhen: '', alertPaths: [], maxPrepared: 5, pollMinutes: 5, live: true, + filter: '', skipTitles: [], alertWhen: '', alertPaths: [], postAlerts: false, postPrefix: PREFIX, + maxPrepared: 5, pollMinutes: 5, live: true, liveTimeoutMinutes: 10, prepareTimeoutMinutes: 30, waitForCi: false, agent: agentConfig(), validate: validateConfig(), }); const next = { filter: 'skip payments', skipTitles: ['\\(payments\\)'], alertWhen: 'a P1', alertPaths: ['packages/shared/src/model/**'], + postAlerts: true, postPrefix: '[a machine wrote this]', maxPrepared: 2, pollMinutes: 3, live: false, liveTimeoutMinutes: 4, prepareTimeoutMinutes: 20, waitForCi: true, validate: validateConfig(), agent: { ...agentConfig(), model: 'opus', mcpAllow: ['mcp__atlassian__getJiraIssue'] }, }; @@ -461,7 +465,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: '', skipTitles: [], alertWhen: '', alertPaths: [], agent: agentConfig(), validate: validateConfig(), waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10 }; + const config = { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', skipTitles: [], alertWhen: '', alertPaths: [], postAlerts: false, postPrefix: PREFIX, agent: agentConfig(), validate: validateConfig(), 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 4c26091e..7e5b830d 100644 --- a/packages/cli/tests/inbox-page.test.ts +++ b/packages/cli/tests/inbox-page.test.ts @@ -140,6 +140,29 @@ describe('the inbox page', () => { expect(script).toContain('view.handled.length'); }); + it('has the posting checkbox and its prefix beside the alert words, and sends both back', () => { + const html = inboxPage(); + expect(html).toContain('id="postAlerts" type="checkbox"'); + expect(html).toContain('Also post the alert findings to the pull request, each prefixed with'); + expect(html).toContain('id="postPrefix" type="text"'); + // Next to the words that decide what an alert is, not among the numbers. + expect(html.indexOf('id="alertWhen"')).toBeLessThan(html.indexOf('id="postAlerts"')); + expect(html.indexOf('id="postPrefix"')).toBeLessThan(html.indexOf('id="alertPaths"')); + const script = pageScript(); + expect(script).toContain("postAlerts: el('postAlerts').checked"); + expect(script).toContain("postPrefix: el('postPrefix').value.trim()"); + expect(script).toContain("el('postAlerts').checked = settings.postAlerts"); + }); + + it('says on a posted card that the findings are already on the pull request, and where', () => { + const script = pageScript(); + expect(script).toContain("'posted to the pull request \\u00b7 ' + hhmm(r.autoPosted.at)"); + expect(script).toContain('findingsLabel(r), postedLabel(r)'); + // The card is a link already, so the review's own URL is a hover rather than a nested link. + expect(script).toContain("row.title = 'the alert findings were posted to ' + r.autoPosted.url"); + expect(script).toContain("r.autoPosted ? 'posted to the pull request' : ''"); + }); + it('lists the alerted pull requests above ready, with the reason and how many findings it names', () => { const html = inboxPage(); expect(html).toContain('

Alerted

'); diff --git a/packages/cli/tests/inbox-prepare.test.ts b/packages/cli/tests/inbox-prepare.test.ts index 2997a4af..70682076 100644 --- a/packages/cli/tests/inbox-prepare.test.ts +++ b/packages/cli/tests/inbox-prepare.test.ts @@ -3,14 +3,14 @@ import { execFileSync } from 'node:child_process'; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { tmpdir } from 'node:os'; -import { preparePr, type PrepareDeps } from '../src/inbox/prepare.js'; +import { preparePr, type MarkPostedOpts, type PostReviewOpts, type PrepareDeps } from '../src/inbox/prepare.js'; import type { ReviewThread } from '../src/inbox/validate.js'; import { worktreePath } from '../src/inbox/worktree.js'; import { startInboxServer } from '../src/inbox/daemon.js'; import { InboxStore } from '../src/inbox/store.js'; import { buildView } from '../src/inbox/view.js'; import type { AgentConfig, InboxConfig, ValidateConfig } from '../src/inbox/config.js'; -import type { PrSnapshot } from '@diffity/github'; +import type { PrSnapshot, ReviewResult } from '@diffity/github'; /** The built-in agent settings, fresh each call so a test cannot leak into the next. */ function agentConfig(): AgentConfig { @@ -42,6 +42,7 @@ function snapshot(): PrSnapshot { function config(): InboxConfig { return { pollMinutes: 5, port: 0, reposDir, worktreesDir, filter: '', skipTitles: [], alertWhen: '', alertPaths: [], + postAlerts: false, postPrefix: '[not yet checked by human]', agent: agentConfig(), validate: validateConfig(), waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, }; } @@ -79,12 +80,26 @@ beforeEach(() => { argvs = []; logs = []; timeouts = []; + daemonLog = []; + submissions = []; + marked = []; + reviewResult = opts => ({ + submitted: opts.submission.comments.length, + submittedThreadIds: opts.submission.comments.map(comment => comment.threadId!), + commentIds: opts.submission.comments.map((comment, index) => ({ threadId: comment.threadId!, githubCommentId: 500 + index })), + skipped: 0, failed: 0, errors: [], + reviewUrl: 'https://github.com/o/demo/pull/4#pullrequestreview-9', + }); }); let prompts: string[] = []; let argvs: string[][] = []; let logs: string[] = []; let timeouts: number[] = []; +let daemonLog: string[] = []; +let submissions: PostReviewOpts[] = []; +let marked: MarkPostedOpts[] = []; +let reviewResult: (opts: PostReviewOpts) => ReviewResult; /** A drafted P1 finding, as `listThreads` reports one. */ function draftedThread(over: Partial = {}): ReviewThread { @@ -119,6 +134,9 @@ function deps(over: Partial = {}): PrepareDeps { mkdirSync(dirname(outPath), { recursive: true }); writeFileSync(outPath, '{"bundle":true}\n'); }, + postReview: opts => { submissions.push(opts); return Promise.resolve(reviewResult(opts)); }, + markPosted: opts => { marked.push(opts); }, + log: message => { daemonLog.push(message); }, now: () => '2026-09-02T12:00:00.000Z', ...over, }; @@ -391,6 +409,208 @@ describe('preparePr', () => { }); }); +describe('posting the findings behind an alert', () => { + const FULL_ID = 'cf15e689-1111-2222-3333-444455556666'; + + /** The agent's output for a review it flagged, naming the findings behind the flag. */ + function alerting(...findings: string[]): Partial { + return { + runAgent: () => Promise.resolve({ + stdout: `reviewing\nALERT: touches auth\nALERT-FINDINGS: ${findings.join(' ')}\nPREPARED\n`, + timedOut: false, + }), + }; + } + + /** The setting on, with a prefix short enough to read in an assertion. */ + function posting(): InboxConfig { + return { ...config(), postAlerts: true, postPrefix: '[not yet checked by human]' }; + } + + function named(over: Partial = {}): ReviewThread { + return draftedThread({ threadId: FULL_ID, ...over }); + } + + it('posts one comment review, the reason as its body and each finding prefixed', async () => { + const result = await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + })); + + expect(submissions).toHaveLength(1); + expect(submissions[0]).toMatchObject({ owner: 'o', repo: 'demo', prNumber: 4, headSha: head }); + expect(submissions[0].submission.event).toBe('COMMENT'); + expect(submissions[0].submission.body).toBe('[not yet checked by human] touches auth'); + expect(submissions[0].submission.comments).toEqual([{ + threadId: FULL_ID, filePath: 'a.ts', side: 'RIGHT', startLine: null, endLine: 1, + body: '[not yet checked by human]\n\nP1: this leaks the token', + }]); + expect(result.kind === 'prepared' && result.posted).toEqual({ + at: '2026-09-02T12:00:00.000Z', headSha: head, + url: 'https://github.com/o/demo/pull/4#pullrequestreview-9', commentIds: 1, + }); + expect(daemonLog).toContain('posted 1 alert finding(s) to o/demo#4 — https://github.com/o/demo/pull/4#pullrequestreview-9'); + }); + + it('marks what went out as sent, with the forge comment id it went out as', async () => { + await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + })); + + expect(marked).toEqual([{ + worktree: worktreePath(worktreesDir, snapshot()), + threadIds: [FULL_ID], + commentIds: [{ threadId: FULL_ID, githubCommentId: 500 }], + reviewUrl: 'https://github.com/o/demo/pull/4#pullrequestreview-9', + headSha: head, + }]); + }); + + it('posts nothing while the setting is off, however loud the alert', async () => { + const result = await preparePr(snapshot(), config(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + })); + + expect(submissions).toEqual([]); + expect(result.kind === 'prepared' && [result.alert, result.posted]).toEqual(['touches auth', null]); + }); + + it('posts nothing when the agent raised no alert of its own', async () => { + const result = await preparePr(snapshot(), posting(), deps({ listThreads: () => Promise.resolve([named()]) })); + + expect(submissions).toEqual([]); + expect(result.kind === 'prepared' && result.posted).toBeNull(); + }); + + it('posts nothing a second time for a head it has already posted for', async () => { + const result = await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + }), { alreadyPostedHead: head }); + + expect(submissions).toEqual([]); + expect(result.kind === 'prepared' && result.posted).toBeNull(); + }); + + it('posts for a bumped preparation, and again once the head has moved on', async () => { + await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + }), { bumped: true, alreadyPostedHead: 'an-older-head' }); + + expect(submissions).toHaveLength(1); + }); + + it('leaves out a finding that is settled, the general summary, and one nobody drafted', async () => { + const result = await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689', 'aaaabbbb', '11112222', '99999999'), + listThreads: () => Promise.resolve([ + named({ status: 'dismissed' }), + named({ threadId: 'aaaabbbb-1111-2222-3333-444455556666', status: 'resolved' }), + named({ threadId: '11112222-1111-2222-3333-444455556666', filePath: '__general__', startLine: 0, endLine: 0 }), + ]), + })); + + expect(submissions[0].submission.comments).toEqual([]); + expect(submissions[0].submission.body).toBe('[not yet checked by human] touches auth'); + expect(result.kind === 'prepared' && result.posted?.commentIds).toBe(0); + expect(daemonLog).toContain('posted 0 alert finding(s) to o/demo#4 — https://github.com/o/demo/pull/4#pullrequestreview-9'); + }); + + it('names a finding by its printed prefix or in full, and posts it once either way', async () => { + await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689', FULL_ID), + listThreads: () => Promise.resolve([named({ startLine: 3, endLine: 5, side: 'old' })]), + })); + + expect(submissions[0].submission.comments).toEqual([{ + threadId: FULL_ID, filePath: 'a.ts', side: 'LEFT', startLine: 3, endLine: 5, + body: '[not yet checked by human]\n\nP1: this leaks the token', + }]); + }); + + it('leaves the review prepared and the alert standing when the forge refuses the post', async () => { + reviewResult = () => ({ + submitted: 0, submittedThreadIds: [], commentIds: [], skipped: 0, failed: 1, + errors: ['gh: 422 Unprocessable Entity'], reviewUrl: null, + }); + const result = await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + })); + + expect(result.kind).toBe('prepared'); + expect(result.kind === 'prepared' && [result.alert, result.posted]).toEqual(['touches auth', null]); + expect(marked).toEqual([]); + expect(daemonLog).toContain('could not post alert findings to o/demo#4: gh: 422 Unprocessable Entity'); + }); + + it('says so when the post threw, or when the forge made no review and said nothing', async () => { + const failed = async (over: Partial) => { + daemonLog = []; + const result = await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), listThreads: () => Promise.resolve([named()]), ...over, + })); + return [result.kind === 'prepared' && result.posted, daemonLog[0]]; + }; + + expect(await failed({ postReview: () => Promise.reject(new Error('gh is not logged in')) })) + .toEqual([null, 'could not post alert findings to o/demo#4: gh is not logged in']); + expect(await failed({ listThreads: () => Promise.reject(new Error('no session')) })) + .toEqual([null, 'could not post alert findings to o/demo#4: no session']); + + reviewResult = () => ({ submitted: 0, submittedThreadIds: [], commentIds: [], skipped: 0, failed: 0, errors: [], reviewUrl: null }); + expect(await failed({})) + .toEqual([null, 'could not post alert findings to o/demo#4: the forge created no review']); + }); + + it('says which findings the forge would not take, without losing the ones it did', async () => { + reviewResult = opts => ({ + submitted: 1, + submittedThreadIds: [FULL_ID], + commentIds: [{ threadId: FULL_ID, githubCommentId: 500 }], + skipped: 0, failed: 1, errors: ['b.ts:9 — not in PR diff'], + reviewUrl: 'https://github.com/o/demo/pull/4#pullrequestreview-9', + }); + const result = await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + })); + + expect(result.kind === 'prepared' && result.posted?.url).toBe('https://github.com/o/demo/pull/4#pullrequestreview-9'); + expect(daemonLog).toContain('o/demo#4: 1 alert finding(s) were left off the review — b.ts:9 — not in PR diff'); + }); + + it('keeps the post when the session could not be told about it', async () => { + const result = await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + markPosted: () => Promise.reject(new Error('the session is gone')), + })); + + expect(result.kind === 'prepared' && result.posted?.commentIds).toBe(1); + expect(daemonLog).toContain('o/demo#4: the posted findings could not be marked as sent — the session is gone'); + }); + + it('posts before the bundle is written, so the bundle carries what went out', async () => { + const order: string[] = []; + await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689'), + listThreads: () => Promise.resolve([named()]), + markPosted: () => { order.push('marked'); }, + exportBundle: ({ outPath }) => { + order.push('exported'); + mkdirSync(dirname(outPath), { recursive: true }); + writeFileSync(outPath, '{"bundle":true}\n'); + }, + })); + + expect(order).toEqual(['marked', 'exported']); + }); +}); + describe('the inbox JSON server', () => { it('answers /api/inbox with the current view', async () => { const store = new InboxStore(':memory:'); diff --git a/packages/cli/tests/inbox-reconcile.test.ts b/packages/cli/tests/inbox-reconcile.test.ts index 843dc337..2840067c 100644 --- a/packages/cli/tests/inbox-reconcile.test.ts +++ b/packages/cli/tests/inbox-reconcile.test.ts @@ -16,7 +16,7 @@ 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, - ciState: null, createdAt: null, updatedAt: null, bumpedAt: null, summary: null, alert: null, alertFindings: [], 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, alertFindings: [], autoPosted: 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, }; } diff --git a/packages/cli/tests/inbox-runtime.test.ts b/packages/cli/tests/inbox-runtime.test.ts index 441509e6..7c6af97b 100644 --- a/packages/cli/tests/inbox-runtime.test.ts +++ b/packages/cli/tests/inbox-runtime.test.ts @@ -31,6 +31,7 @@ function attendedPr(): AttendedPr { function liveConfig(): InboxConfig { return { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', skipTitles: [], alertWhen: '', alertPaths: [], + postAlerts: false, postPrefix: '[not yet checked by human]', agent: { model: 'the-configured-model', effort: null, mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, validate: { model: null, timeoutMinutes: 15, maxBudgetUsd: null }, waitForCi: false, prepareTimeoutMinutes: 30, maxPrepared: 5, live: true, liveTimeoutMinutes: 10, @@ -310,6 +311,66 @@ describe('the real listThreads', () => { }, 40_000); }); +describe('the real markPosted', () => { + it('marks the findings that went out in the pull request\'s own session, ids and all', async () => { + const repo = join(root, 'posted-repo'); + const dataDir = join(root, 'posted-data'); + execFileSync('git', ['init', '-b', 'main', repo], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repo, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: repo, stdio: 'pipe' }); + writeFileSync(join(repo, 'a.ts'), 'const a = 1;\n'); + execFileSync('git', ['add', '.'], { cwd: repo, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'init'], { cwd: repo, stdio: 'pipe' }); + execFileSync('git', ['checkout', '-q', '-b', 'work'], { cwd: repo, stdio: 'pipe' }); + writeFileSync(join(repo, 'a.ts'), 'const a = 1;\nconst token = leak();\nconst b = 2;\n'); + execFileSync('git', ['commit', '-qam', 'change'], { cwd: repo, stdio: 'pipe' }); + + const server = await startDiffityServer(process.execPath, ENTRY, repo, 'main', dataDir, 20_000); + try { + const agent = (args: string[]) => execFileSync(process.execPath, [ENTRY, '--repo', repo, 'agent', ...args], { + stdio: 'pipe', encoding: 'utf-8', env: { ...process.env, DIFFITY_DATA_DIR: dataDir }, + }); + agent(['comment', '--file', 'a.ts', '--line', '2', '--body', 'P1: this leaks the token']); + agent(['comment', '--file', 'a.ts', '--line', '3', '--body', 'P3: a nit']); + + const deps = realPrepareDeps(process.execPath, ENTRY, () => dataDir, liveConfig(), () => {}); + const before = await deps.listThreads(repo); + const sent = before.find(thread => thread.startLine === 2)!; + const kept = before.find(thread => thread.startLine === 3)!; + + await deps.markPosted({ + worktree: repo, + threadIds: [sent.threadId], + commentIds: [{ threadId: sent.threadId, githubCommentId: 901 }], + reviewUrl: 'https://github.com/o/r/pull/1#pullrequestreview-9', + headSha: 'abc1234', + }); + + const listed = JSON.parse(agent(['list', '--json'])) as { + id: string; submittedAt: string | null; submittedReviewUrl: string | null; + submittedHeadSha: string | null; githubCommentId: number | null; + }[]; + expect(listed.find(thread => thread.id === sent.threadId)).toMatchObject({ + submittedReviewUrl: 'https://github.com/o/r/pull/1#pullrequestreview-9', + submittedHeadSha: 'abc1234', + githubCommentId: 901, + }); + expect(listed.find(thread => thread.id === kept.threadId)?.submittedAt).toBeNull(); + + // A review the forge gave no URL for still marks what went out in it. + await deps.markPosted({ + worktree: repo, threadIds: [kept.threadId], commentIds: [], reviewUrl: null, headSha: 'abc1234', + }); + const again = JSON.parse(agent(['list', '--json'])) as { id: string; submittedAt: string | null; submittedReviewUrl: string | null }[]; + expect(again.find(thread => thread.id === kept.threadId)).toMatchObject({ submittedReviewUrl: null }); + expect(again.find(thread => thread.id === kept.threadId)?.submittedAt).toBeTruthy(); + } finally { + server.stop(); + await gone(dataDir); + } + }, 40_000); +}); + /** Waits for the server registered in a data directory to be gone, so its files stop moving. */ async function gone(dataDir: string, waitMs = 10_000): Promise { const registry = join(dataDir, 'registry.json'); diff --git a/packages/cli/tests/inbox-store.test.ts b/packages/cli/tests/inbox-store.test.ts index 11337c8a..3d311da1 100644 --- a/packages/cli/tests/inbox-store.test.ts +++ b/packages/cli/tests/inbox-store.test.ts @@ -143,6 +143,46 @@ describe('InboxStore migration', () => { store.close(); }); + it('remembers the head the alert findings were posted for, and the review they went out in', () => { + const store = new InboxStore(path); + store.observe(snapshot(), true, 'now'); + expect(store.get('o/r#1')!.autoPosted).toBeNull(); + + store.markAutoPosted('o/r#1', { + at: '2026-09-02T12:00:00.000Z', headSha: 'aaa', + url: 'https://github.com/o/r/pull/1#pullrequestreview-9', + }); + expect(store.get('o/r#1')!.autoPosted).toEqual({ + at: '2026-09-02T12:00:00.000Z', headSha: 'aaa', + url: 'https://github.com/o/r/pull/1#pullrequestreview-9', + }); + + // The next head overwrites it: what the column answers is whether this head has been posted to. + store.markAutoPosted('o/r#1', { at: '2026-09-02T13:00:00.000Z', headSha: 'bbb', url: null }); + expect(store.get('o/r#1')!.autoPosted).toEqual({ at: '2026-09-02T13:00:00.000Z', headSha: 'bbb', url: null }); + store.close(); + }); + + it('takes a posted review on a table that predates the columns', () => { + 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, + summary TEXT, alert TEXT, alert_findings TEXT)`); + seed.close(); + + const store = new InboxStore(path); + store.observe(snapshot(), true, 'now'); + expect(store.get('o/r#1')!.autoPosted).toBeNull(); + store.markAutoPosted('o/r#1', { at: '2026-09-02T12:00:00.000Z', headSha: 'aaa', url: null }); + expect(store.get('o/r#1')!.autoPosted).toEqual({ at: '2026-09-02T12:00:00.000Z', headSha: 'aaa', url: null }); + store.close(); + }); + it('reads a findings column that does not name a list of ids as naming none', () => { 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 56d943b6..699f665f 100644 --- a/packages/cli/tests/inbox-tick.test.ts +++ b/packages/cli/tests/inbox-tick.test.ts @@ -88,7 +88,7 @@ beforeEach(() => { kind: 'prepared', headSha: snap.headSha, bundlePath: `/b/${snap.number}.json`, worktree: `/wt/${snap.number}`, logPath: `/l/${snap.number}.log`, at: '2026-09-02T12:00:00.000Z', summary: '1 P2', alert: snap.number === 2 ? 'touches auth' : null, - alertFindings: snap.number === 2 ? ['cf15e689', '7b2a10c4'] : [], run: run(), + alertFindings: snap.number === 2 ? ['cf15e689', '7b2a10c4'] : [], posted: null, run: run(), validation: 'not-needed', validateRun: null, }); }); @@ -449,7 +449,7 @@ describe('runTick', () => { const logged: string[] = []; prepareResult = snap => ({ kind: 'prepared', headSha: snap.headSha, bundlePath: '/b.json', worktree: '/wt', logPath: '/l.log', - at: '2026-09-02T12:00:00.000Z', summary: '1 P1', alert: null, alertFindings: [], run: run(), + at: '2026-09-02T12:00:00.000Z', summary: '1 P1', alert: null, alertFindings: [], posted: null, run: run(), validation: 'validated', validateRun: { startedAt: '2026-09-02T12:00:00.000Z', endedAt: '2026-09-02T12:03:00.000Z', @@ -472,7 +472,7 @@ describe('runTick', () => { const logged: string[] = []; prepareResult = snap => ({ kind: 'prepared', headSha: snap.headSha, bundlePath: '/b.json', worktree: '/wt', logPath: '/l.log', - at: '2026-09-02T12:00:00.000Z', summary: '1 P1 \u00b7 unchecked', alert: null, alertFindings: [], run: run(), + at: '2026-09-02T12:00:00.000Z', summary: '1 P1 \u00b7 unchecked', alert: null, alertFindings: [], posted: null, run: run(), validation: 'unchecked', validateRun: { startedAt: '2026-09-02T12:00:00.000Z', endedAt: '2026-09-02T12:15:00.000Z', stats: null, @@ -492,7 +492,7 @@ describe('runTick', () => { forge.set(snapshot()); prepareResult = snap => ({ kind: 'prepared', headSha: snap.headSha, bundlePath: '/b.json', worktree: '/wt', logPath: '/l.log', - at: '2026-09-02T12:00:00.000Z', summary: null, alert: null, alertFindings: [], run: run({ stats: null }), + at: '2026-09-02T12:00:00.000Z', summary: null, alert: null, alertFindings: [], posted: null, run: run({ stats: null }), validation: 'not-needed', validateRun: null, }); await runTick(store, deps()); @@ -761,7 +761,7 @@ describe('a posted review', () => { prepareResult = snap => ({ kind: 'prepared', headSha: snap.headSha, bundlePath: '/b/1.json', worktree: '/wt/1', logPath: '/l/1.log', at: '2026-09-02T13:05:00.000Z', summary: '1 P2', alert: null, - alertFindings: [], run: run(), validation: 'not-needed', validateRun: null, + alertFindings: [], posted: null, run: run(), validation: 'not-needed', validateRun: null, }); await prepareBumped(store, deps(), 'o/r#1'); expect(prepared).toEqual(['o/r#1']); @@ -871,3 +871,137 @@ describe('a posted review', () => { expect(forge.views).toEqual(['o/r#9']); }); }); + +describe('a pull request the daemon posted the alert findings to', () => { + const POSTED_AT = '2026-09-02T12:00:00.000Z'; + const REVIEW = 'https://github.com/o/r/pull/1#pullrequestreview-9'; + + /** A review prepared with an alert, and the findings behind it posted at that head. */ + function alerting(): void { + prepareResult = snap => ({ + kind: 'prepared', headSha: snap.headSha, bundlePath: `/b/${snap.number}.json`, + worktree: `/wt/${snap.number}`, logPath: `/l/${snap.number}.log`, at: POSTED_AT, + summary: '1 P1', alert: 'touches auth', alertFindings: ['cf15e689'], + posted: { at: POSTED_AT, headSha: snap.headSha, url: REVIEW, commentIds: 1 }, + run: run(), validation: 'not-needed', validateRun: null, + }); + } + + it('records the review against the row, so the next head is the only one posted to again', async () => { + alerting(); + forge.set(snapshot()); + await runTick(store, deps()); + + expect(store.get('o/r#1')!.autoPosted).toEqual({ at: POSTED_AT, headSha: 'aaa', url: REVIEW }); + }); + + it('tells the next preparation which head has already been posted to', async () => { + alerting(); + forge.set(snapshot()); + const heads: (string | null)[] = []; + const record = (over = {}) => deps({ + prepare: (snap, opts) => { heads.push(opts.alreadyPostedHead); return Promise.resolve(prepareResult(snap)); }, + ...over, + }); + await runTick(store, record()); + + // The next push is a head nobody has posted to, so the refreshed review posts again. + forge.snapshots.set('o/r#1', snapshot({ headSha: 'bbb' })); + await runTick(store, record()); + + expect(heads).toEqual([null, 'aaa']); + expect(store.get('o/r#1')!.autoPosted!.headSha).toBe('bbb'); + }); + + it('stays the reviewer\'s to review once the post has withdrawn the request', async () => { + alerting(); + forge.set(snapshot()); + await runTick(store, deps()); + + // GitHub drops it from review-requested:@me the moment the review lands. + forge.requested = []; + await runTick(store, deps()); + + const pr = store.get('o/r#1')!; + expect(pr.status).toBe('prepared'); + expect(pr.worktreePath).toBe('/wt/1'); + expect(removed).toEqual([]); + const view = buildView(store, 'http://localhost:5390', POSTED_AT); + expect(view.alerted.map(row => row.id)).toEqual(['o/r#1']); + expect(view.alerted[0].autoPosted).toEqual({ at: POSTED_AT, headSha: 'aaa', url: REVIEW }); + }); + + it('re-prepares and posts again when the author pushes, without losing the worktree', async () => { + alerting(); + forge.set(snapshot()); + await runTick(store, deps()); + + forge.requested = []; + forge.snapshots.set('o/r#1', snapshot({ headSha: 'bbb' })); + prepared = []; + await runTick(store, deps()); + + expect(prepared).toEqual(['o/r#1']); + expect(store.get('o/r#1')!.preparedHeadSha).toBe('bbb'); + expect(store.get('o/r#1')!.autoPosted!.headSha).toBe('bbb'); + expect(removed).toEqual([]); + }); + + it('holds at its head once the reviewer has dismissed it', async () => { + alerting(); + forge.set(snapshot()); + await runTick(store, deps()); + store.setStatus('o/r#1', 'dismissed', 'dismissed by the reviewer'); + + forge.requested = []; + prepared = []; + await runTick(store, deps()); + + expect(store.get('o/r#1')!.status).toBe('dismissed'); + expect(prepared).toEqual([]); + }); + + it('becomes handled, and gives up its worktree, once the reviewer posts their own review', async () => { + alerting(); + forge.set(snapshot()); + await runTick(store, deps()); + + store.recordHandled({ prId: 'o/r#1', headSha: 'aaa', event: 'APPROVE', reviewUrl: REVIEW, at: '2026-09-02T12:30:00.000Z' }); + forge.requested = []; + await runTick(store, deps()); + + const pr = store.get('o/r#1')!; + expect(pr.status).toBe('handled'); + expect(pr.statusReason).toBe('you approved'); + expect(removed).toEqual(['/wt/1']); + expect(pr.worktreePath).toBeNull(); + }); + + it('is retired like any other once it is merged', async () => { + alerting(); + forge.set(snapshot()); + await runTick(store, deps()); + + forge.requested = []; + forge.snapshots.set('o/r#1', snapshot({ state: 'MERGED' })); + await runTick(store, deps()); + + expect(store.get('o/r#1')!.status).toBe('done'); + expect(removed).toEqual(['/wt/1']); + }); + + it('waits for CI like a requested row when its refresh is held back', async () => { + alerting(); + forge.set(snapshot()); + await runTick(store, deps()); + + forge.requested = []; + forge.snapshots.set('o/r#1', snapshot({ headSha: 'bbb', checks: [{ name: 'build', status: 'pending' }] })); + prepared = []; + await runTick(store, deps({ waitForCi: true })); + + expect(prepared).toEqual([]); + expect(store.get('o/r#1')!.statusReason).toBe('waiting: CI running (1 checks)'); + expect(removed).toEqual([]); + }); +}); diff --git a/packages/cli/tests/inbox-units.test.ts b/packages/cli/tests/inbox-units.test.ts index 28f4c0ae..78c4d271 100644 --- a/packages/cli/tests/inbox-units.test.ts +++ b/packages/cli/tests/inbox-units.test.ts @@ -116,7 +116,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', skipTitles: ['\\(payments\\)'], alertWhen: 'a P1', alertPaths: ['packages/shared/**'], maxPrepared: 3, pollMinutes: 7, + filter: 'skip payments', skipTitles: ['\\(payments\\)'], alertWhen: 'a P1', alertPaths: ['packages/shared/**'], + postAlerts: true, postPrefix: '[a machine wrote this]', maxPrepared: 3, pollMinutes: 7, live: false, liveTimeoutMinutes: 4, prepareTimeoutMinutes: 20, waitForCi: true, agent: { model: 'opus', effort: 'high', mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, validate: { model: null, timeoutMinutes: 15, maxBudgetUsd: null }, @@ -141,6 +142,19 @@ describe('parseInboxConfig', () => { expect(() => parseInboxConfig({ liveTimeoutMinutes: 0 })).toThrow(/liveTimeoutMinutes must be a positive number/); }); + it('takes postAlerts as a boolean and refuses an empty prefix while it is on', () => { + expect([parseInboxConfig({}).postAlerts, parseInboxConfig({}).postPrefix]) + .toEqual([false, '[Automated AI pre-review, not yet checked by human]']); + expect(parseInboxConfig({ postAlerts: true, postPrefix: '[a machine wrote this]' })) + .toMatchObject({ postAlerts: true, postPrefix: '[a machine wrote this]' }); + // Off, so an empty prefix says nothing about anything. + expect(parseInboxConfig({ postPrefix: '' }).postPrefix).toBe(''); + expect(() => parseInboxConfig({ postAlerts: 'yes' })).toThrow(/postAlerts must be true or false/); + expect(() => parseInboxConfig({ postPrefix: 3 })).toThrow(/postPrefix must be a string/); + expect(() => parseInboxConfig({ postAlerts: true, postPrefix: ' ' })) + .toThrow(/postPrefix must not be empty when postAlerts is on/); + }); + it('takes maxPrepared as a positive integer only', () => { expect(parseInboxConfig({}).maxPrepared).toBe(5); expect(parseInboxConfig({ maxPrepared: 2 }).maxPrepared).toBe(2); @@ -291,7 +305,8 @@ describe('summarizeFindings', () => { describe('parseSettingsPatch', () => { const full = { - filter: 'a', skipTitles: ['Release$'], alertWhen: 'b', alertPaths: ['src/**'], maxPrepared: 2, pollMinutes: 3, live: false, + filter: 'a', skipTitles: ['Release$'], alertWhen: 'b', alertPaths: ['src/**'], + postAlerts: false, postPrefix: '[not yet checked by human]', maxPrepared: 2, pollMinutes: 3, live: false, liveTimeoutMinutes: 5, prepareTimeoutMinutes: 15, waitForCi: false, agent: { model: null, effort: null, mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, validate: { model: null, timeoutMinutes: 15, maxBudgetUsd: null }, @@ -316,6 +331,21 @@ describe('parseSettingsPatch', () => { .toMatchObject({ ok: false, message: expect.stringContaining('skipTitles[1] is not a valid regular expression') }); }); + it('takes the posting checkbox and its prefix, and refuses posting with nothing to prefix with', () => { + const edited = { ...full, postAlerts: true, postPrefix: '[a machine wrote this]' }; + expect(parseSettingsPatch(JSON.stringify(edited))).toEqual({ ok: true, settings: edited }); + expect(parseSettingsPatch(JSON.stringify({ ...full, postAlerts: undefined }))) + .toMatchObject({ ok: false, message: 'postAlerts is missing' }); + expect(parseSettingsPatch(JSON.stringify({ ...full, postPrefix: undefined }))) + .toMatchObject({ ok: false, message: 'postPrefix is missing' }); + expect(parseSettingsPatch(JSON.stringify({ ...full, postPrefix: 3 }))) + .toMatchObject({ ok: false, message: 'postPrefix must be a string' }); + expect(parseSettingsPatch(JSON.stringify({ ...full, postAlerts: true, postPrefix: '' }))) + .toMatchObject({ ok: false, message: 'postPrefix must not be empty when postAlerts is on' }); + expect(parseSettingsPatch(JSON.stringify({ ...full, postPrefix: 'x'.repeat(5000) }))) + .toMatchObject({ ok: false, message: expect.stringContaining('postPrefix is longer than') }); + }); + it('takes the validate fields the page edits and refuses a bad one by name', () => { const edited = { ...full, validate: { model: 'opus', timeoutMinutes: 20, maxBudgetUsd: 3 } }; expect(parseSettingsPatch(JSON.stringify(edited))).toEqual({ ok: true, settings: edited }); diff --git a/packages/cli/tests/server-routes.test.ts b/packages/cli/tests/server-routes.test.ts index a02b6c91..784e7fdb 100644 --- a/packages/cli/tests/server-routes.test.ts +++ b/packages/cli/tests/server-routes.test.ts @@ -328,3 +328,38 @@ describe('one file at a time', () => { expect(status).toBe(400); }); }); + +describe('a finding the daemon already posted', () => { + it('reads back as sent, so the page does not offer it to the forge again', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { importBundle } = await import('../src/bundle.js'); + const headSha = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: repoDir, encoding: 'utf-8' }).trim(); + const session = findOrCreateSession('work'); + + importBundle(session, { + formatVersion: 1, headSha, ref: 'work', baseSha: null, repo: null, prNumber: 1, + createdAt: '2026-09-02T12:00:00.000Z', generator: 'the inbox daemon', + threads: [{ + filePath: 'a.ts', side: 'new', startLine: 1, endLine: 1, status: 'open', anchorContent: null, + posted: { reviewUrl: 'https://github.com/o/r/pull/1#pullrequestreview-9', headSha, githubCommentId: 901 }, + comments: [{ + author: { name: 'Agent', type: 'agent' }, body: 'P1: this leaks the token', + kind: 'review', createdAt: '2026-09-02T12:00:00.000Z', + }], + }], + tours: [], + }); + + const { status, text } = await req('/api/threads'); + const threads = JSON.parse(text) as { comments: { body: string }[]; submittedAt: string | null; submittedReviewUrl: string | null; githubCommentId: number | null }[]; + const imported = threads.filter(thread => thread.comments[0]?.body === 'P1: this leaks the token'); + + expect(status).toBe(200); + expect(imported).toHaveLength(1); + expect(imported[0].submittedAt).toBeTruthy(); + expect(imported[0].submittedReviewUrl).toBe('https://github.com/o/r/pull/1#pullrequestreview-9'); + expect(imported[0].githubCommentId).toBe(901); + // Nothing else the session holds was touched by the import. + expect(threads.filter(thread => thread.submittedAt)).toHaveLength(1); + }); +}); diff --git a/packages/cli/tests/submitted-state.test.ts b/packages/cli/tests/submitted-state.test.ts index ae292443..02cac449 100644 --- a/packages/cli/tests/submitted-state.test.ts +++ b/packages/cli/tests/submitted-state.test.ts @@ -1,9 +1,12 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import { execFileSync } from 'node:child_process'; +import { execFileSync, spawn } from 'node:child_process'; import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { tmpdir } from 'node:os'; +const DIST_ENTRY = join(dirname(fileURLToPath(import.meta.url)), '..', 'dist', 'index.js'); + let root: string; let repoDir: string; let origCwd: string; @@ -129,3 +132,58 @@ describe('what a sent thread remembers about the review', () => { expect(sent?.submittedHeadSha).toBeNull(); }); }); + +describe('the daemon marking what it posted itself', () => { + /** `diffity agent mark-posted`, run the way the daemon runs it: the built CLI, in the checkout. */ + function markPosted(args: string[]): Promise<{ code: number | null; stdout: string; stderr: string }> { + return new Promise(resolve => { + const child = spawn(process.execPath, [DIST_ENTRY, 'agent', 'mark-posted', ...args], { + cwd: repoDir, stdio: ['ignore', 'pipe', 'pipe'], env: process.env, + }); + let stdout = ''; + let stderr = ''; + child.stdout.setEncoding('utf-8'); + child.stderr.setEncoding('utf-8'); + child.stdout.on('data', chunk => { stdout += chunk; }); + child.stderr.on('data', chunk => { stderr += chunk; }); + child.on('close', code => resolve({ code, stdout, stderr })); + }); + } + + it('marks each named thread, by its printed prefix or in full, with the comment it went out as', async () => { + const { getThread } = await import('../src/threads.js'); + const withId = await newThread('P1: posted with an id'); + const withoutId = await newThread('P1: posted without one'); + const untouched = await newThread('P2: nobody posted this'); + + const { code, stdout } = await markPosted([ + '--review-url', 'https://github.com/o/r/pull/1#pullrequestreview-9', + '--head-sha', 'abc1234', + `${withId.id.slice(0, 8)}=901`, + withoutId.id, + ]); + + expect(code).toBe(0); + expect(stdout).toContain('Marked 2 thread(s) as posted'); + expect(getThread(withId.id)).toMatchObject({ + submittedReviewUrl: 'https://github.com/o/r/pull/1#pullrequestreview-9', + submittedHeadSha: 'abc1234', + githubCommentId: 901, + }); + expect(getThread(withoutId.id)?.submittedAt).toBeTruthy(); + expect(getThread(withoutId.id)?.githubCommentId).toBeNull(); + expect(getThread(untouched.id)?.submittedAt).toBeNull(); + }); + + it('refuses a thread it cannot find and a comment id that is not one', async () => { + const thread = await newThread('P1: posted once more'); + + const unknown = await markPosted(['--head-sha', 'abc1234', 'ffffffff']); + expect(unknown.code).toBe(1); + expect(unknown.stderr).toContain('Thread not found: ffffffff'); + + const notAnId = await markPosted(['--head-sha', 'abc1234', `${thread.id}=nine`]); + expect(notAnId.code).toBe(1); + expect(notAnId.stderr).toContain('does not name a forge comment id'); + }); +}); diff --git a/packages/git/package.json b/packages/git/package.json index fecbfd7c..5cd02627 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.10.30", + "version": "0.10.31", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index 74318cb9..71f22838 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.10.30", + "version": "0.10.31", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index 7d1d6f55..1dce6220 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.10.30", + "version": "0.10.31", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 79c24661..71555e15 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.10.30", + "version": "0.10.31", "type": "module", "private": true, "scripts": { From cb6e3f86aee6b328117d4e3ac24bc85158ae034c Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Tue, 8 Sep 2026 17:28:51 +0200 Subject: [PATCH 2/2] fix: an alert whose named findings were all settled reaches the reviewer but not the pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reason alone goes to the author only for an alert that named no findings, which is all there is to say about it. When the agent named findings and none of them is open by the time the review goes out — the checking pass dismissed them — nothing is posted: the pull request would otherwise carry an alert whose whole basis has just been rejected. The row still shows the alert, so the reviewer decides what to do with it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- packages/cli/src/inbox/prepare.ts | 13 ++++++-- packages/cli/tests/inbox-prepare.test.ts | 38 ++++++++++++++++++++---- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/inbox/prepare.ts b/packages/cli/src/inbox/prepare.ts index b1274359..ed3586c4 100644 --- a/packages/cli/src/inbox/prepare.ts +++ b/packages/cli/src/inbox/prepare.ts @@ -247,9 +247,12 @@ export async function preparePr(snapshot: PrSnapshot, config: InboxConfig, deps: /** * Puts the findings the agent named behind its alert on the pull request, as one `COMMENT` review * in the reviewer's name, every comment opening with the configured prefix so nobody reads it as a - * verdict a human has stood behind. Null when nothing was posted: the setting is off, the agent - * raised no alert of its own, this head has been posted to already, or the post did not go through - * — the review is prepared either way, and the reviewer still has the alert and the findings. + * verdict a human has stood behind. An alert that named no findings is posted as its reason alone, + * which is all there is to say; one whose named findings are all settled by the time the review + * goes out is not posted at all, because the checking pass has just rejected everything the alert + * rests on. Null when nothing was posted: that case, the setting being off, no alert of the + * agent's own, a head that has been posted to already, or a post that did not go through — the + * review is prepared either way, and the reviewer still has the alert and the findings. */ async function postAlertFindings( snapshot: PrSnapshot, @@ -263,6 +266,10 @@ async function postAlertFindings( const id = prId(snapshot); try { const comments = alertComments(await deps.listThreads(ctx.worktree), ctx.alertFindings, config.postPrefix); + if (ctx.alertFindings.length > 0 && comments.length === 0) { + deps.log(`${id}: the alert's findings did not survive the check — nothing posted`); + return null; + } const result = await deps.postReview({ owner: snapshot.owner, repo: snapshot.repo, diff --git a/packages/cli/tests/inbox-prepare.test.ts b/packages/cli/tests/inbox-prepare.test.ts index 70682076..30b78446 100644 --- a/packages/cli/tests/inbox-prepare.test.ts +++ b/packages/cli/tests/inbox-prepare.test.ts @@ -503,7 +503,19 @@ describe('posting the findings behind an alert', () => { expect(submissions).toHaveLength(1); }); - it('leaves out a finding that is settled, the general summary, and one nobody drafted', async () => { + it('posts the reason alone when the agent named no findings for it', async () => { + const result = await preparePr(snapshot(), posting(), deps({ + ...alerting(), + listThreads: () => Promise.resolve([named()]), + })); + + expect(submissions[0].submission.comments).toEqual([]); + expect(submissions[0].submission.body).toBe('[not yet checked by human] touches auth'); + expect(result.kind === 'prepared' && result.posted?.commentIds).toBe(0); + expect(daemonLog).toContain('posted 0 alert finding(s) to o/demo#4 — https://github.com/o/demo/pull/4#pullrequestreview-9'); + }); + + it('posts nothing when every finding the alert named has been settled', async () => { const result = await preparePr(snapshot(), posting(), deps({ ...alerting('cf15e689', 'aaaabbbb', '11112222', '99999999'), listThreads: () => Promise.resolve([ @@ -513,10 +525,26 @@ describe('posting the findings behind an alert', () => { ]), })); - expect(submissions[0].submission.comments).toEqual([]); - expect(submissions[0].submission.body).toBe('[not yet checked by human] touches auth'); - expect(result.kind === 'prepared' && result.posted?.commentIds).toBe(0); - expect(daemonLog).toContain('posted 0 alert finding(s) to o/demo#4 — https://github.com/o/demo/pull/4#pullrequestreview-9'); + expect(submissions).toEqual([]); + expect(daemonLog).toContain('o/demo#4: the alert\'s findings did not survive the check — nothing posted'); + // The reviewer still gets the alert; it is the author who is not told about a rejected finding. + expect(result.kind === 'prepared' && [result.alert, result.posted]).toEqual(['touches auth', null]); + }); + + it('posts the findings that survived, leaving out the settled one beside them', async () => { + const survivor = 'aaaabbbb-1111-2222-3333-444455556666'; + await preparePr(snapshot(), posting(), deps({ + ...alerting('cf15e689', 'aaaabbbb'), + listThreads: () => Promise.resolve([ + named({ status: 'dismissed' }), + named({ threadId: survivor, startLine: 2, endLine: 2, comments: [{ id: 'c2', body: 'P1: and this one holds' }] }), + ]), + })); + + expect(submissions[0].submission.comments).toEqual([{ + threadId: survivor, filePath: 'a.ts', side: 'RIGHT', startLine: null, endLine: 2, + body: '[not yet checked by human]\n\nP1: and this one holds', + }]); }); it('names a finding by its printed prefix or in full, and posts it once either way', async () => {