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
22 changes: 12 additions & 10 deletions packages/cli/src/agent.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -16,6 +16,7 @@ import {
} from './threads.js';
import { answerLiveRequest, type LiveRequest } from './live.js';
import { clampClientWait } from './live-wait.js';
import { directiveFor } from './live-intent.js';
import { findInstanceForRepo, type RegistryEntry } from './registry.js';
import { createHash } from 'node:crypto';
import { createTour, addTourStep, updateTourStatus, deleteTour, deleteToursForSession } from './tours.js';
Expand DownExpand Up@@ -313,9 +314,15 @@ Examples:
const startedAt = Date.now();
let payload: { request: LiveRequest | null };
try {
const res = await fetch(`http://127.0.0.1:${instance.port}/api/live/claim?wait=${wait}`, {
method: 'POST',
});
// Park on the session the server is serving, not on whatever the shared current-session
// file last named — those differ whenever another worktree has opened a review since.
const info = await fetch(`http://127.0.0.1:${instance.port}/api/info`);
const sessionId = info.ok
? ((await info.json()) as { sessionId?: string }).sessionId
: undefined;
const claimUrl = `http://127.0.0.1:${instance.port}/api/live/claim?wait=${wait}`
+ (sessionId ? `&session=${encodeURIComponent(sessionId)}` : '');
const res = await fetch(claimUrl, { method: 'POST' });
if (!res.ok) {
console.error(pc.red(`Could not wait for a request: ${res.status} ${await res.text()}`));
process.exitCode = 1;
Expand DownExpand Up@@ -347,15 +354,10 @@ Examples:

// stdout is the request, so a script can parse it. The directive goes to stderr, because the
// turn this wakes up may be a long way from whatever armed the loop.
const mayChange = payload.request.mayChangeCode !== false;
console.error(
pc.cyan(
'A request came back from the review page. '
+ (mayChange
? 'Answer it in the thread, amend the finding it is about, or make the change'
: 'Answer it in the thread or amend the finding it is about — this pull request is '
+ 'somebody else\'s, so do not edit its code')
+ ' — then re-arm with `agent await`. The diffity-live skill has the detail.',
`${directiveFor(payload.request.intent, payload.request.mayChangeCode !== false)}\n`
+ 'Then re-arm with `agent await`. The diffity-live skill has the detail.',
),
);
console.log(JSON.stringify(payload.request, null, 2));
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/db.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -141,6 +141,7 @@ function migrateDb(db: DatabaseSync): void {
// because "asked but not yet picked up" and "picked up but not answered" are both waiting, and
// the page says different things about them.
addColumn(db, 'comments', 'live_requested_at', 'TEXT');
addColumn(db, 'comments', 'live_intent', 'TEXT');
addColumn(db, 'comments', 'live_claimed_at', 'TEXT');
addColumn(db, 'comments', 'live_answered_at', 'TEXT');
addColumn(db, 'comment_threads', 'submitted_at', 'TEXT');
Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -72,6 +72,8 @@ program
.option('--dark', 'Open in dark mode (default: light)')
.option('--unified', 'Open in unified view (default: split)')
.option('--new', 'Stop existing instance and start fresh')
.option('--work', 'You are working on this branch, so the agent may change code')
.option('--review', 'You are reviewing it, so the agent may not — even if you wrote it')
.addHelpText('after', `
Common usage:
$ diffity See all uncommitted changes
Expand DownExpand Up@@ -331,6 +333,7 @@ range syntax (main..feature, main...feature) also work.`)
prNumber: parsedPrNumber,
version: pkg.version,
registryInfo: { repoRoot, repoHash, repoName },
purpose: opts.work ? 'work' : opts.review ? 'review' : undefined,
});
const urlParams = new URLSearchParams({ ref: effectiveRef });
if (opts.dark) {
Expand Down
23 changes: 23 additions & 0 deletions packages/cli/src/live-intent.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
/** What the reader pressed: a question, or a request for a change. */
export type LiveIntent = 'ask' | 'act';

/** Anything not plainly `act` is a question: least privilege, and it covers old and malformed alike. */
export function normaliseIntent(value: unknown): LiveIntent {
return value === 'act' ? 'act' : 'ask';
}

/** Said in plain language, because that is what the agent acts on. */
export function directiveFor(intent: LiveIntent, mayChangeCode: boolean): string {
if (intent === 'ask') {
return 'The reader asked a question. Answer it in the thread, or amend the finding it is about. '
+ 'Do not change code — they pressed Ask, not Act.';
}

if (!mayChangeCode) {
return 'The reader asked for a change, but this pull request is somebody else\'s. '
+ 'Do not change code. Answer in the thread, or amend the finding, and say that is why.';
}

return 'The reader asked for a change. Read it, make the change, and reply in the thread with what '
+ 'you did. Do not commit, push or merge.';
}
25 changes: 25 additions & 0 deletions packages/cli/src/live-permissions.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -12,3 +12,28 @@ export function mayChangeCode(pullRequest: { viewerDidAuthor?: boolean } | null)
}
return pullRequest.viewerDidAuthor === true;
}

export type SessionPurpose = 'work' | 'review';

export function normalisePurpose(value: unknown): SessionPurpose | undefined {
return value === 'work' || value === 'review' ? value : undefined;
}

/**
* Authorship is a proxy for whose work this is, and it stops being one the moment work is handed
* over: take over a colleague's branch and the pull request still says they opened it.
*
* The agent launching diffity knows which it is doing, so it can say — once, at launch, for that
* server only. Not a setting: there is nothing to leave switched on, because it dies with the
* process. Unsaid still means derived from authorship.
*/
export function resolveMayChangeCode(
purpose: SessionPurpose | undefined,
pullRequest: { viewerDidAuthor?: boolean } | null,
): boolean {
const said = normalisePurpose(purpose);
if (said) {
return said === 'work';
}
return mayChangeCode(pullRequest);
}
37 changes: 28 additions & 9 deletions packages/cli/src/live.ts
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
import { getDb, queryOne } from './db.js';
import { normaliseIntent, type LiveIntent } from './live-intent.js';

/**
* A question or an instruction the reader left for the agent, taken from the comment it was written
Expand All@@ -25,6 +26,8 @@ export interface LiveRequest {
* filled in by the route that hands the request over.
*/
mayChangeCode?: boolean;
/** What the reader pressed. A question must not turn into an edit. */
intent: LiveIntent;
}

export interface LiveRequestStamp {
Expand All@@ -34,10 +37,10 @@ export interface LiveRequestStamp {
requestedAt: string | null;
}

export function requestLive(commentId: string): LiveRequestStamp {
export function requestLive(commentId: string, intent: LiveIntent = 'ask'): LiveRequestStamp {
getDb()
.prepare("UPDATE comments SET live_requested_at = datetime('now') WHERE id = ?")
.run(commentId);
.prepare("UPDATE comments SET live_requested_at = datetime('now'), live_intent = ? WHERE id = ?")
.run(intent, commentId);

const row = queryOne<{ session_id: string; live_requested_at: string | null }>(
`SELECT t.session_id, c.live_requested_at FROM comments c JOIN comment_threads t ON t.id = c.thread_id
Expand DownExpand Up@@ -73,19 +76,20 @@ export function claimNextLiveRequest(sessionId: string): LiveRequest | null {
return null;
}

return (
queryOne<LiveRequest>(
`SELECT c.id AS commentId, c.thread_id AS threadId, c.body AS body,
c.author_name AS authorName, t.file_path AS filePath, t.side AS side,
const request = queryOne<LiveRequest>(
`SELECT c.id AS commentId, c.thread_id AS threadId, c.body AS body,
c.author_name AS authorName, c.live_intent AS intent,
t.file_path AS filePath, t.side AS side,
t.start_line AS startLine, t.end_line AS endLine,
(SELECT f.body FROM comments f WHERE f.thread_id = t.id
AND COALESCE(f.kind, 'review') = 'review'
ORDER BY f.created_at ASC, f.rowid ASC LIMIT 1) AS findingBody
FROM comments c JOIN comment_threads t ON t.id = c.thread_id
WHERE c.id = ?`,
claimed.id,
) ?? null
claimed.id,
);

return request ? { ...request, intent: normaliseIntent(request.intent) } : null;
}

/**
Expand All@@ -99,6 +103,21 @@ export function answerLiveRequest(commentIdOrPrefix: string): boolean {
return Number(result.changes ?? 0) > 0;
}

/**
* Requests an agent has taken and not yet answered. Between the two it is not parked on the claim
* route, so presence alone would report nobody there while somebody is working on your question.
*/
export function liveWorkingCount(sessionId: string): number {
const row = queryOne<{ n: number }>(
`SELECT COUNT(*) AS n FROM comments c JOIN comment_threads t ON t.id = c.thread_id
WHERE t.session_id = ?
AND c.live_claimed_at IS NOT NULL
AND c.live_answered_at IS NULL`,
sessionId,
);
return row?.n ?? 0;
}

/** How many requests are waiting for somebody to pick them up. */
export function pendingLiveCount(sessionId: string): number {
const row = queryOne<{ n: number }>(
Expand Down
9 changes: 5 additions & 4 deletions packages/cli/src/review-routes.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@ import {
type CommentKind,
} from './threads.js';
import { requestLive, notifyLiveListeners } from './live.js';
import { normaliseIntent } from './live-intent.js';
import { getCurrentSession, resolveSessionId } from './session.js';
import { sendJson, sendError, withJsonBody } from './http-utils.js';

Expand DownExpand Up@@ -50,7 +51,7 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat

if (pathname === '/api/threads' && req.method === 'POST') {
withJsonBody(res, req, 'Failed to create thread', (body) => {
const { sessionId: sid, filePath, side, startLine, endLine, body: commentBody, author, anchorContent, kind, live } = body;
const { sessionId: sid, filePath, side, startLine, endLine, body: commentBody, author, anchorContent, kind, live, intent } = body;
if (!sid || !filePath || !side || typeof startLine !== 'number' || typeof endLine !== 'number' || !commentBody || !author) {
sendError(res, 400, 'Missing required fields');
return;
Expand All@@ -63,7 +64,7 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat
threadKind,
);
if (live === true && threadKind === 'aside') {
const stamp = requestLive(thread.comments[0].id);
const stamp = requestLive(thread.comments[0].id, normaliseIntent(intent));
thread.comments[0].liveRequestedAt = stamp.requestedAt;
notifyLiveListeners(stamp.sessionId);
}
Expand All@@ -75,7 +76,7 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat
const threadReplyMatch = pathname.match(/^\/api\/threads\/([^/]+)\/reply$/);
if (threadReplyMatch && req.method === 'POST') {
withJsonBody(res, req, 'Failed to add reply', (body) => {
const { body: commentBody, author, kind, live } = body;
const { body: commentBody, author, kind, live, intent } = body;
if (!commentBody || !author) {
sendError(res, 400, 'Missing body or author');
return;
Expand All@@ -91,7 +92,7 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat
// request's author, and it is going to the forge rather than to a listener here.
let requestedAt: string | null = null;
if (live === true && commentKind === 'aside') {
const stamp = requestLive(comment.id);
const stamp = requestLive(comment.id, normaliseIntent(intent));
requestedAt = stamp.requestedAt;
notifyLiveListeners(stamp.sessionId);
}
Expand Down
30 changes: 24 additions & 6 deletions packages/cli/src/server.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -47,9 +47,10 @@ import {
type ReviewEvent,
} from '@diffity/github';
import { findOrCreateSession, resolveSessionId } from './session.js';
import { mayChangeCode } from './live-permissions.js';
import { resolveMayChangeCode, type SessionPurpose } from './live-permissions.js';
import {
liveListenerCount,
liveWorkingCount,
pendingLiveCount,
reclaimStaleLiveRequests,
waitForLiveRequest,
Expand DownExpand Up@@ -190,6 +191,11 @@ function isSameOriginRequest(req: IncomingMessage): boolean {
}

interface ServerOptions {
/**
* What the agent launching this said it was here for. Unsaid means derived from who wrote the
* pull request, which is wrong exactly when work has been handed over.
*/
purpose?: SessionPurpose;
port: number;
portIsExplicit?: boolean;
diffArgs: string[];
Expand DownExpand Up@@ -247,6 +253,7 @@ interface ServerResult {

export function startServer(options: ServerOptions): Promise<ServerResult> {
const {
purpose,
port,
portIsExplicit,
diffArgs,
Expand DownExpand Up@@ -365,14 +372,25 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
// as one arms and answers, and react-query keeps an unchanged object's identity — so
// carrying this on the info payload made every consumer of it re-render each time a
// listener came or went, which reads as the page reloading under you.
// The page asks about the session it is showing, which is the one for its ref — not
// whichever session the ambient current-session file last named, which is shared by every
// worktree using this data directory.
const liveSessionId = (): string => {
const asked = url.searchParams.get('session');
if (asked) {
return resolveSessionId(asked);
}
return findOrCreateSession(url.searchParams.get('ref') || effectiveRef).id;
};

if (pathname === '/api/live/status') {
const sid = resolveSessionId(url.searchParams.get('session'));
const sid = liveSessionId();
sendJson(res, {
// A comment box that drives an agent is only as safe as the loopback bind, so live
// mode is not offered at all when the server is reachable from elsewhere.
enabled: isLoopbackBind(getBindHost()),
listening: sid ? liveListenerCount(sid) > 0 : false,
working: sid ? liveWorkingCount(sid) > 0 : false,
waiting: sid ? pendingLiveCount(sid) : 0,
mayChangeCode: resolveMayChangeCode(purpose, authorship()),
});
return;
}
Expand All@@ -382,7 +400,7 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
sendError(res, 403, 'Live mode is only available on a loopback bind');
return;
}
const sid = resolveSessionId(url.searchParams.get('session'));
const sid = liveSessionId();
if (!sid) {
sendError(res, 400, 'No review session');
return;
Expand DownExpand Up@@ -411,7 +429,7 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
}
// Carried on the request rather than left for the agent to look up: a rule nobody
// has to remember is a rule that holds.
sendJson(res, { request: { ...request, mayChangeCode: mayChangeCode(authorship()) } });
sendJson(res, { request: { ...request, mayChangeCode: resolveMayChangeCode(purpose, authorship()) } });
},
err => {
if (!res.writableEnded && !listenerGone.signal.aborted) {
Expand Down
10 changes: 9 additions & 1 deletion packages/cli/src/threads.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -16,6 +16,7 @@ export interface ThreadComment {
kind: CommentKind;
createdAt: string;
liveRequestedAt: string | null;
liveIntent: string | null;
liveClaimedAt: string | null;
liveAnsweredAt: string | null;
}
Expand DownExpand Up@@ -65,6 +66,7 @@ interface CommentRow {
kind?: string | null;
created_at: string;
live_requested_at?: string | null;
live_intent?: string | null;
live_claimed_at?: string | null;
live_answered_at?: string | null;
}
Expand DownExpand Up@@ -96,6 +98,7 @@ function rowToComment(row: CommentRow): ThreadComment {
kind: (row.kind as CommentKind | null) ?? 'review',
createdAt: row.created_at,
liveRequestedAt: row.live_requested_at ?? null,
liveIntent: row.live_intent ?? null,
liveClaimedAt: row.live_claimed_at ?? null,
liveAnsweredAt: row.live_answered_at ?? null,
};
Expand DownExpand Up@@ -202,6 +205,7 @@ export function createThread(
kind,
createdAt: now,
liveRequestedAt: null,
liveIntent: null,
liveClaimedAt: null,
liveAnsweredAt: null,
}],
Expand All@@ -216,6 +220,7 @@ interface JoinedRow extends ThreadRow {
c_kind: string | null;
c_created_at: string | null;
c_live_requested_at: string | null;
c_live_intent: string | null;
c_live_claimed_at: string | null;
c_live_answered_at: string | null;
}
Expand All@@ -230,7 +235,8 @@ export function getThreadsForSession(sessionId: string, status?: ThreadStatus):
SELECT t.*,
c.id AS c_id, c.author_name AS c_author_name, c.author_type AS c_author_type,
c.body AS c_body, c.kind AS c_kind, c.created_at AS c_created_at,
c.live_requested_at AS c_live_requested_at, c.live_claimed_at AS c_live_claimed_at,
c.live_requested_at AS c_live_requested_at, c.live_intent AS c_live_intent,
c.live_claimed_at AS c_live_claimed_at,
c.live_answered_at AS c_live_answered_at
FROM comment_threads t
LEFT JOIN comments c ON c.thread_id = t.id
Expand All@@ -253,6 +259,7 @@ export function getThreadsForSession(sessionId: string, status?: ThreadStatus):
kind: (row.c_kind as CommentKind | null) ?? 'review',
createdAt: row.c_created_at!,
liveRequestedAt: row.c_live_requested_at ?? null,
liveIntent: row.c_live_intent ?? null,
liveClaimedAt: row.c_live_claimed_at ?? null,
liveAnsweredAt: row.c_live_answered_at ?? null,
});
Expand DownExpand Up@@ -307,6 +314,7 @@ export function addReply(
kind,
createdAt: now,
liveRequestedAt: null,
liveIntent: null,
liveClaimedAt: null,
liveAnsweredAt: null,
};
Expand Down
Loading