Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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. |
Expand Down
12 changes: 6 additions & 6 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/api/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@diffity/api",
"version": "0.10.30",
"version": "0.10.31",
"private": true,
"type": "module",
"main": "./dist/index.js",
Expand Down
28 changes: 26 additions & 2 deletions packages/api/src/bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -121,17 +131,31 @@ 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),
// Line 0 is real: a general comment is about the whole diff and sits on no line.
...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 {
Expand Down
36 changes: 36 additions & 0 deletions packages/api/tests/bundle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>[])[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<string, unknown>[])[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<string, unknown>[])[0].posted = { githubCommentId: 0 };
expect(errorOf(wrong)).toBe('threads[0].posted.githubCommentId must be an integer >= 1');

const notAnObject = validBundle();
(notAnObject.threads as Record<string, unknown>[])[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');
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/package.json
Original file line number Diff line number Diff line change
@@ -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": {
Expand Down
33 changes: 33 additions & 0 deletions packages/cli/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
addReply,
updateThreadStatus,
editComment,
markThreadsSubmitted,
type Thread,
} from './threads.js';
import {
Expand Down Expand Up @@ -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('<threads...>', 'Thread ids, each optionally <thread-id>=<github-comment-id>')
.option('--review-url <url>', 'The review they went out in')
.option('--head-sha <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)')
Expand Down Expand Up @@ -772,6 +791,20 @@ Examples:
});
}

/** `<thread-id>` or `<thread-id>=<github-comment-id>`, 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) {
Expand Down
17 changes: 16 additions & 1 deletion packages/cli/src/bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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++;
}

Expand Down
30 changes: 29 additions & 1 deletion packages/cli/src/inbox/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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<InboxConfig, 'filter' | 'skipTitles' | 'alertWhen' | 'alertPaths' | 'maxPrepared' | 'pollMinutes' | 'live' | 'liveTimeoutMinutes' | 'prepareTimeoutMinutes' | 'waitForCi' | 'agent' | 'validate'>;
export type InboxSettings = Pick<InboxConfig, 'filter' | 'skipTitles' | 'alertWhen' | 'alertPaths' | 'postAlerts' | 'postPrefix' | 'maxPrepared' | 'pollMinutes' | 'live' | 'liveTimeoutMinutes' | 'prepareTimeoutMinutes' | 'waitForCi' | 'agent' | 'validate'>;

/**
* Writes the page-editable settings into the config file, leaving every other key as the reviewer
Expand Down
5 changes: 3 additions & 2 deletions packages/cli/src/inbox/daemon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PrepareResult>;
prepare?: (snapshot: PrSnapshot, opts: { bumped: boolean; alreadyPostedHead: string | null }) => Promise<PrepareResult>;
/** 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. */
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(),
Expand Down
Loading
Loading