diff --git a/README.md b/README.md index 040f2a6..1b7d432 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,10 @@ It works with Claude Code, Cursor, Codex, and any AI coding agent. | [Browse project files](#browse-project-files) | Explore your repo and comment on any file for AI to resolve | | [Guided code tours](#guided-code-tours) | Walk through your codebase step by step with highlighted code | | [Learn any topic](#learn-any-topic) | Project-driven learning for programming languages, tools, and frameworks | -| [GitHub PRs](#github-prs) | Pull down a PR, review it locally, push comments back to GitHub | +| [GitHub PRs](#github-prs) | Pull down a PR, review it locally, submit one review back | +| [Reading order](#reading-a-diff-in-the-order-it-makes-sense) | Read a diff in the order it makes sense, not alphabetically | +| [Attention](#what-gets-your-attention) | Highlight what matters, dim what a rule can prove is mechanical | +| [Review state](#while-a-review-is-running) | See when a review is still running, and what has already been sent | | [Multiple projects](#multiple-projects) | Run it in multiple repos at once, each gets its own port | ## See your diffs @@ -83,10 +86,10 @@ Leave comments on any line — when you're done, run `/diffity-resolve` to have ### `/diffity-review` -Your agent reviews the diff and leaves inline comments in the viewer. Uses severity tags (`[must-fix]`, `[suggestion]`, `[nit]`, `[question]`) so you can triage by importance. Supports refs, focus areas, and natural language: +Your agent reviews the diff and leaves inline comments in the viewer, prefixed with severities so you can triage by importance — `P1`/`P2`/`P3` by default, or whatever the project configures (see [Review standards](#review-standards)). It reads the project's own standards first, records a reading order for the diff, and announces itself as running so you do not approve while findings are still arriving. Supports refs, focus areas, and natural language: ``` -/diffity-review # review working tree changes +/diffity-review # the branch's pull request, or the working tree /diffity-review main # review what you're merging into main /diffity-review main..feature # review what you're merging into main /diffity-review identify security issues # focus on security issues @@ -191,9 +194,23 @@ Pass a GitHub PR URL to view and review pull requests locally: diffity https://github.com/owner/repo/pull/123 ``` -This checks out the PR, opens the diff against its base branch, and lets you leave comments in the viewer. Requires the [`gh` CLI](https://cli.github.com/) installed and authenticated (`gh auth login`), and the current repo must match the PR's repository. +This checks out the PR and opens the diff against **the commit the pull request is based on**, so the file and line counts match what the forge shows rather than drifting with your local base branch. A merged pull request works too: its branch is usually deleted, so the head is fetched from `refs/pull//head`. Requires the [`gh` CLI](https://cli.github.com/) installed and authenticated (`gh auth login`), and the current repo must match the PR's repository. -You can push your comments (including AI review comments) back to GitHub as PR review comments, and pull existing GitHub comments into the viewer. Both are available from the viewer UI. +Above the diff you get the pull request's description and every review already on it, so you are not re-deriving intent from the code or repeating a point someone else has made. + +### Submitting a review + +The forge dialog is a composer, not a push button: + +- a checkbox per finding, so you send the ones you agree with. Everything open starts selected; deselecting is remembered, so a finding your agent writes while the dialog is open cannot slip into a set you have already curated +- replies on a finding are folded into the one comment the forge will hold +- general comments seed the summary, which you can edit +- **Comment**, **Approve** or **Request changes** — and an approval needs nothing attached, since a verdict stands on its own. Approve and Request changes are disabled on your own pull request, which the forge refuses anyway +- everything goes as **one review**: one notification for the author, a summary that has somewhere to live, and no half-posted review if something fails + +A comment on a line the pull request does not touch is caught before anything is sent, because the whole review is a single request and one unpostable line would reject all of it. Findings already sent are marked *already on the pull request* and left unselected. + +Existing inline comments can be pulled into the viewer from the same dialog. The skills work with PR URLs too: @@ -205,6 +222,80 @@ The skills work with PR URLs too: Passing a PR URL to `/diffity-tour` locks it to review mode — the agent reads the PR's description, commits, and diff to build a guided walkthrough that you can use before approving or merging. +## Reading a diff in the order it makes sense + +A diff arrives alphabetically, which is rarely the order it should be read in. When a walkthrough +exists for the change, the file list is **reordered** to follow it: the file that explains the rest +first, the mechanical ones last, everything the walkthrough does not mention below a divider. Each +file carries the walkthrough's one-line note on why it is read at that point, a stepper walks the +stops, and `A-Z` in the sidebar header returns to the alphabetical tree. + +A walkthrough is recorded by an agent (`/diffity-review` does it as its last step, and +`/diffity-tour` builds one on request), so this costs you nothing to use. + +## What gets your attention + +Two mechanisms, with the decider deliberately different for each. + +**Highlighted** — the lines a walkthrough points at are tinted. An agent can only ever *add* +attention this way, never take it away. + +**Dimmed** — decided by rules, never by a model, because dimming asserts that something needs +*less* attention. A hunk recedes when every line it touches is an import, when its added and +removed lines are the same lines with different whitespace, or when the file is generated. It comes +back on hover, stays selectable and commentable, and carries the reason, so you can always find out +why rather than having to trust it. + +Files where indentation is syntax — `.py`, `.yml`, `.yaml`, `.md`, `Makefile` and friends — are +never whitespace-dimmed, because a reindent there can change behaviour. + +Whitespace hiding is **on by default** and remembered: it is the formatter's business, not yours. +Because a filtered diff shows fewer lines than the forge does, the header says so and names the +amount — `whitespace hidden (2 files, 18 lines suppressed)`. + +## Commenting + +Click a line to comment on it. **Shift-click** a second line to extend the comment across the span, +the way the forge does it — within one file and one side, since a range spanning both sides of a +diff is not something that can be commented on. Dragging down the gutter also selects a range. + +## While a review is running + +An agent announces a review before it starts writing and again when it finishes. While one is open +the page carries a banner with the count of findings so far, and **submitting is blocked** — the +difference between "nothing found" and "not finished looking" is the difference between approving a +change and approving it too early. + +A finding survives the commits you make in response to it: when HEAD moves, open findings and the +walkthrough follow into the new session, and a finding whose code merely *moved* is re-anchored to +it. A finding whose code was **edited** keeps its old position rather than being guessed onto +something it was not written about. + +## The agent CLI + +Skills drive these; they are listed because they are the whole interface an agent needs. + +``` +diffity agent standards [--json] # the project's severities and standards document +diffity agent diff # the unified diff for this session +diffity agent list [--status open|resolved|dismissed] [--json] +diffity agent review-start [--note ] +diffity agent review-done +diffity agent comment --file --line [--end-line ] [--side new|old] --body +diffity agent general-comment --body +diffity agent reply --body +diffity agent resolve [--summary ] +diffity agent dismiss [--reason ] +diffity agent tour-start --topic [--body ] [--json] +diffity agent tour-step --tour --file --line [--end-line ] --body [--annotation ] +diffity agent tour-done --tour +diffity agent tour-delete # correct a walkthrough instead of adding another +diffity agent tour-delete --all # or clear the session's finished ones +``` + +A comment's line range is trimmed to the file's length, and you are told when that happens: a range +running past the end would otherwise be counted and highlighted with nothing to show. + ## Multiple projects Diffity supports running multiple projects simultaneously. Each gets its own port automatically: @@ -235,8 +326,12 @@ diffity list --json # machine-readable output --unified Unified view (default: split) --quiet Minimal terminal output --new Stop existing instance and start fresh +--repo Repository to work on, when the current directory is not one ``` +`--repo` is for the common case where a project directory holds several worktrees as +subdirectories rather than being a repository itself. It must come before a positional argument. + ## Environment variables | Variable | Description | diff --git a/UPSTREAM.md b/UPSTREAM.md index 1ce8a2c..de31c94 100644 --- a/UPSTREAM.md +++ b/UPSTREAM.md @@ -7,11 +7,19 @@ of it can be extracted as a pointed upstream pull request later. - **`main`** is a pristine mirror of `upstream/main`. Upstream updates land here first (`git merge upstream/main`), then flow to `develop`. Nothing is committed to it directly. -- **`develop`** is the working version — what the `diffity` on PATH is built from. The worktree at - `~/nc/diffity-fork/diffity` stays parked on it; feature work happens in a second worktree so - switching branches cannot rebuild the tool while someone is using it. -- **Branches are kept after merging**, because each one is the extraction unit. To offer a change - upstream: `git diff ..`. +- **`develop`** is the default branch and the working version — what the `diffity` on PATH is built + from. The worktree at `~/nc/diffity-fork/diffity` stays parked on it; feature work happens in a + second worktree so switching branches cannot rebuild the tool while someone is using it. +- **Squash merge only**, through a pull request, matching the other NaturalCycles repositories. + Nothing is merged into `develop` locally. After a squash lands, a local `develop` is refreshed with + `git fetch && git reset --hard origin/develop` rather than merged — the branch commits are not its + ancestors. +- **The squashed commit is the extraction unit.** One commit per concern, so offering a change + upstream is `git show ` rather than a branch diff. Head branches are deleted on merge, which + is fine for that reason; the table below records the commit each change landed as. + +Everything up to and including PR #24 was merged with merge commits, before this arrangement — that +part of the history is mixed, and those branches still exist. Nothing here is NaturalCycles-specific — all of it is upstreamable as-is. When that changes, fork-local work goes in its own section below so it never lands in an upstream patch by accident. diff --git a/packages/cli/src/agent.ts b/packages/cli/src/agent.ts index 87d3b03..31af1ad 100644 --- a/packages/cli/src/agent.ts +++ b/packages/cli/src/agent.ts @@ -13,7 +13,7 @@ import { type ThreadStatus, type Thread, } from './threads.js'; -import { createTour, addTourStep, updateTourStatus } from './tours.js'; +import { createTour, addTourStep, updateTourStatus, deleteTour, deleteToursForSession } from './tours.js'; import { readAnchor, clampToFile, countWorkingTreeLines } from './anchor.js'; import { startReviewRun, finishReviewRun } from './review-run.js'; import { readRepoConfig, DEFAULT_SEVERITIES, resolveInRepo, REPO_CONFIG_FILE } from '@diffity/git'; @@ -351,6 +351,30 @@ Examples: console.log(pc.green(`Added step ${step.sortOrder} to tour`)); }); + agent + .command('tour-delete') + .description('Remove a walkthrough') + .argument('[tour-id]', 'Walkthrough to remove') + .option('--all', 'Remove every finished walkthrough in this session instead') + .option('--include-building', 'With --all, also remove one another agent may still be writing') + .action((tourId: string | undefined, opts: { all?: boolean; includeBuilding?: boolean }) => { + const session = requireSession(); + if (tourId) { + deleteTour(tourId); + console.log(pc.green(`Removed walkthrough ${tourId.slice(0, 8)}`)); + return; + } + // Deleting every walkthrough has to be asked for. Reaching it by leaving the id off meant + // an agent told to "fix the walkthrough" could wipe one a human recorded. + if (!opts.all) { + console.error(pc.red('Give a walkthrough id, or --all to remove every one in this session')); + process.exitCode = 1; + return; + } + deleteToursForSession(session.id, { keepBuilding: !opts.includeBuilding }); + console.log(pc.green('Removed every walkthrough in this session')); + }); + agent .command('tour-done') .description('Mark a tour as ready for viewing') diff --git a/packages/cli/src/db.ts b/packages/cli/src/db.ts index 13fbe3f..76c27f8 100644 --- a/packages/cli/src/db.ts +++ b/packages/cli/src/db.ts @@ -128,6 +128,13 @@ function migrateDb(db: DatabaseSync): void { // identified by ref and commit alone, which two repositories sharing a data directory can // collide on -- `work` and `master` are not unique names. addColumn(db, 'review_sessions', 'repo_root', 'TEXT'); + // Sending a finding to the forge is not the same as resolving it, so this is its own column + // rather than a status: a submitted thread is still open until someone deals with it. + addColumn(db, 'comment_threads', 'submitted_at', 'TEXT'); + // Sending is not resolving, and a timestamp alone cannot answer "did this go out against the + // code that is there now?" — so the review and the commit it went out against are kept too. + addColumn(db, 'comment_threads', 'submitted_review_url', 'TEXT'); + addColumn(db, 'comment_threads', 'submitted_head_sha', 'TEXT'); } function addColumn(db: DatabaseSync, table: string, column: string, type: string): void { diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 870f7fc..36434ea 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -169,7 +169,7 @@ range syntax (main..feature, main...feature) also work.`) if (!opts.quiet) { console.log(pc.dim(` Checking out PR #${parsed.number}...`)); } - checkoutPr(parsed.number); + checkoutPr(parsed.number, parsed.owner, parsed.repo); } catch (err) { console.error(pc.red(`Error: Failed to checkout PR #${parsed.number}.`)); console.log(` ${err}`); @@ -177,7 +177,7 @@ range syntax (main..feature, main...feature) also work.`) } try { - prBase = getPrBase(parsed.number); + prBase = getPrBase(parsed.number, parsed.owner, parsed.repo); } catch { console.error(pc.red(`Error: Could not determine base branch for PR #${parsed.number}.`)); process.exit(1); diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 60e6399..98dafc4 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -50,7 +50,7 @@ import { findOrCreateSession } from './session.js'; import { computeDiffFingerprint } from './fingerprint.js'; import { parseDiffStatSummary } from './diff-stat.js'; import { getReviewRun } from './review-run.js'; -import { createThread, addReply, getThreadsForSession } from './threads.js'; +import { createThread, addReply, getThreadsForSession, markThreadsSubmitted } from './threads.js'; import { handleReviewRoute } from './review-routes.js'; import { handleTourRoute } from './tour-routes.js'; import { sendJson, sendError, readBody } from './http-utils.js'; @@ -550,6 +550,10 @@ export function startServer(options: ServerOptions): Promise { details.headSha, { event, body: summary, comments }, ); + markThreadsSubmitted(result.submittedThreadIds, { + reviewUrl: result.reviewUrl, + headSha: details.headSha, + }); sendJson(res, result); return; } diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index 94a291f..757eb6d 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -27,10 +27,17 @@ export interface Thread { anchorContent: string | null; createdAt: string; updatedAt: string; + /** When this finding was last sent to the forge, or null while it has never left the machine. */ + submittedAt: string | null; + submittedReviewUrl: string | null; + submittedHeadSha: string | null; comments: ThreadComment[]; } interface ThreadRow { + submitted_at?: string | null; + submitted_review_url?: string | null; + submitted_head_sha?: string | null; id: string; session_id: string; file_path: string; @@ -64,6 +71,9 @@ function rowToThread(row: ThreadRow, comments: ThreadComment[]): Thread { anchorContent: row.anchor_content, createdAt: row.created_at, updatedAt: row.updated_at, + submittedAt: row.submitted_at ?? null, + submittedReviewUrl: row.submitted_review_url ?? null, + submittedHeadSha: row.submitted_head_sha ?? null, comments, }; } @@ -101,6 +111,27 @@ function getCommentsForThread(threadId: string): ThreadComment[] { return map.get(threadId) ?? []; } +export interface SubmittedIn { + reviewUrl?: string | null; + headSha?: string | null; +} + +export function markThreadsSubmitted(threadIds: string[], submittedIn: SubmittedIn = {}): void { + if (threadIds.length === 0) { + return; + } + + const db = getDb(); + const placeholders = threadIds.map(() => '?').join(', '); + db.prepare( + `UPDATE comment_threads + SET submitted_at = datetime('now'), + submitted_review_url = ?, + submitted_head_sha = ? + WHERE id IN (${placeholders})`, + ).run(submittedIn.reviewUrl ?? null, submittedIn.headSha ?? null, ...threadIds); +} + export function updateThreadLines(threadId: string, startLine: number, endLine: number): void { const db = getDb(); db.prepare('UPDATE comment_threads SET start_line = ?, end_line = ? WHERE id = ?').run( @@ -146,6 +177,9 @@ export function createThread( anchorContent: anchorContent ?? null, createdAt: now, updatedAt: now, + submittedAt: null, + submittedReviewUrl: null, + submittedHeadSha: null, comments: [{ id: commentId, author, diff --git a/packages/cli/src/tour-routes.ts b/packages/cli/src/tour-routes.ts index c36a369..34603ff 100644 --- a/packages/cli/src/tour-routes.ts +++ b/packages/cli/src/tour-routes.ts @@ -1,6 +1,7 @@ import type { IncomingMessage, ServerResponse } from 'node:http'; import { createTour, + deleteTour, getTour, getToursForSession, addTourStep, @@ -57,6 +58,12 @@ export function handleTourRoute(req: IncomingMessage, res: ServerResponse, pathn } const tourMatch = pathname.match(/^\/api\/tours\/([^/]+)$/); + if (tourMatch && req.method === 'DELETE') { + deleteTour(tourMatch[1]); + sendJson(res, { ok: true }); + return true; + } + if (tourMatch && req.method === 'GET') { const tour = getTour(tourMatch[1]); if (!tour) { diff --git a/packages/cli/src/tours.ts b/packages/cli/src/tours.ts index bb948e7..b9365e6 100644 --- a/packages/cli/src/tours.ts +++ b/packages/cli/src/tours.ts @@ -200,3 +200,34 @@ export function updateTourStatus(tourId: string, status: TourStatus): void { const db = getDb(); db.prepare('UPDATE tours SET status = ? WHERE id = ?').run(status, tourId); } + +/** + * A walkthrough can be wrong — a mistaken step, a body mangled on the way in — and until now the + * only remedy was to add another and rely on the newest winning, leaving the bad one behind. + */ +export function deleteTour(tourId: string): void { + const db = getDb(); + db.prepare('DELETE FROM tour_steps WHERE tour_id = ?').run(tourId); + db.prepare('DELETE FROM tours WHERE id = ?').run(tourId); +} + +/** + * Spares a walkthrough that is still being written unless told otherwise: the whole point of the + * building state is that something is mid-flight, and two agents on one session should not erase + * each other. An agent replacing its own half-written walkthrough knows it is doing that, and says + * so with `keepBuilding: false`. + */ +export function deleteToursForSession( + sessionId: string, + options: { keepBuilding?: boolean } = {}, +): void { + const keepBuilding = options.keepBuilding ?? true; + const filter = keepBuilding ? " AND status != 'building'" : ''; + const db = getDb(); + db.prepare( + `DELETE FROM tour_steps WHERE tour_id IN ( + SELECT id FROM tours WHERE session_id = ?${filter} + )`, + ).run(sessionId); + db.prepare(`DELETE FROM tours WHERE session_id = ?${filter}`).run(sessionId); +} diff --git a/packages/cli/tests/submitted-state.test.ts b/packages/cli/tests/submitted-state.test.ts new file mode 100644 index 0000000..821e880 --- /dev/null +++ b/packages/cli/tests/submitted-state.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; + +beforeAll(() => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-submitted-')); + repoDir = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: repoDir, stdio: 'pipe' }); + writeFileSync(join(repoDir, 'a.ts'), 'const a = 1;\n'); + execFileSync('git', ['add', '.'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'init'], { cwd: repoDir, stdio: 'pipe' }); + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +async function newThread(body: string) { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread } = await import('../src/threads.js'); + const session = findOrCreateSession('work'); + return createThread(session.id, 'a.ts', 'new', 1, 1, body, { name: 'Agent', type: 'agent' }); +} + +describe('a thread that has been sent to the forge', () => { + it('starts out unsent', async () => { + const thread = await newThread('P2: not sent yet'); + + expect(thread.submittedAt).toBeNull(); + }); + + it('records when it was sent', async () => { + const { markThreadsSubmitted, getThread } = await import('../src/threads.js'); + const thread = await newThread('P2: about to be sent'); + + markThreadsSubmitted([thread.id]); + + expect(getThread(thread.id)?.submittedAt).toBeTruthy(); + }); + + it('leaves other threads alone', async () => { + const { markThreadsSubmitted, getThread } = await import('../src/threads.js'); + const sent = await newThread('P2: sent'); + const kept = await newThread('P2: kept back'); + + markThreadsSubmitted([sent.id]); + + expect(getThread(sent.id)?.submittedAt).toBeTruthy(); + expect(getThread(kept.id)?.submittedAt).toBeNull(); + }); + + it('is still open, because sending is not resolving', async () => { + const { markThreadsSubmitted, getThread } = await import('../src/threads.js'); + const thread = await newThread('P2: sent but unresolved'); + + markThreadsSubmitted([thread.id]); + + expect(getThread(thread.id)?.status).toBe('open'); + }); + + it('ignores an empty list rather than marking everything', async () => { + const { markThreadsSubmitted, getThread } = await import('../src/threads.js'); + const thread = await newThread('P2: untouched'); + + markThreadsSubmitted([]); + + expect(getThread(thread.id)?.submittedAt).toBeNull(); + }); +}); + +describe('what a sent thread remembers about the review', () => { + it('records the review it went out in and the commit it went out against', async () => { + const { markThreadsSubmitted, getThread } = await import('../src/threads.js'); + const thread = await newThread('P2: sent with provenance'); + + markThreadsSubmitted([thread.id], { + reviewUrl: 'https://github.com/o/r/pull/1#pullrequestreview-9', + headSha: 'abc1234', + }); + + const sent = getThread(thread.id); + expect(sent?.submittedReviewUrl).toBe('https://github.com/o/r/pull/1#pullrequestreview-9'); + expect(sent?.submittedHeadSha).toBe('abc1234'); + }); + + // The question a reviewer actually asks after a push is "did this go out against the code that + // is there now?", which needs the sha, not just a timestamp. + it('still marks the thread when the forge told us neither', async () => { + const { markThreadsSubmitted, getThread } = await import('../src/threads.js'); + const thread = await newThread('P2: sent without provenance'); + + markThreadsSubmitted([thread.id]); + + const sent = getThread(thread.id); + expect(sent?.submittedAt).toBeTruthy(); + expect(sent?.submittedReviewUrl).toBeNull(); + expect(sent?.submittedHeadSha).toBeNull(); + }); +}); diff --git a/packages/cli/tests/tour-delete.test.ts b/packages/cli/tests/tour-delete.test.ts new file mode 100644 index 0000000..1097b08 --- /dev/null +++ b/packages/cli/tests/tour-delete.test.ts @@ -0,0 +1,179 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; + +beforeAll(() => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-tour-delete-')); + repoDir = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: repoDir, stdio: 'pipe' }); + writeFileSync(join(repoDir, 'a.ts'), 'const a = 1;\n'); + execFileSync('git', ['add', '.'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'init'], { cwd: repoDir, stdio: 'pipe' }); + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +async function session() { + const { findOrCreateSession } = await import('../src/session.js'); + return findOrCreateSession('work'); +} + +describe('correcting a walkthrough', () => { + it('can be deleted, taking its steps with it', async () => { + const { createTour, addTourStep, deleteTour, getTour, getToursForSession } = await import( + '../src/tours.js' + ); + const s = await session(); + const tour = createTour(s.id, 'wrong order', ''); + addTourStep(tour.id, 'a.ts', 1, 1, 'body', 'annotation'); + + deleteTour(tour.id); + + expect(getTour(tour.id)).toBeNull(); + expect(getToursForSession(s.id).some(t => t.id === tour.id)).toBe(false); + }); + + it('leaves another walkthrough alone', async () => { + const { createTour, deleteTour, getTour } = await import('../src/tours.js'); + const s = await session(); + const doomed = createTour(s.id, 'doomed', ''); + const kept = createTour(s.id, 'kept', ''); + + deleteTour(doomed.id); + + expect(getTour(doomed.id)).toBeNull(); + expect(getTour(kept.id)).not.toBeNull(); + }); + + it('is silent about an id that is not there', async () => { + const { deleteTour } = await import('../src/tours.js'); + + expect(() => deleteTour('no-such-tour')).not.toThrow(); + }); + + it('can replace its own half-written attempts in a single step', async () => { + const { createTour, getToursForSession, deleteToursForSession } = await import('../src/tours.js'); + const s = await session(); + createTour(s.id, 'first attempt', ''); + createTour(s.id, 'second attempt', ''); + + // A walkthrough starts out building, so clearing your own needs saying so. + deleteToursForSession(s.id, { keepBuilding: false }); + const replacement = createTour(s.id, 'the good one', ''); + + expect(getToursForSession(s.id).map(t => t.id)).toEqual([replacement.id]); + }); +}); + +describe('a walkthrough still being written', () => { + it('survives a bulk delete, because something is mid-flight', async () => { + const { createTour, updateTourStatus, deleteToursForSession, getTour } = await import( + '../src/tours.js' + ); + const s = await session(); + const finished = createTour(s.id, 'finished', ''); + updateTourStatus(finished.id, 'ready'); + const building = createTour(s.id, 'still writing', ''); + + deleteToursForSession(s.id); + + expect(getTour(finished.id)).toBeNull(); + expect(getTour(building.id)).not.toBeNull(); + }); + + it('can still be deleted on purpose, by id', async () => { + const { createTour, deleteTour, getTour } = await import('../src/tours.js'); + const s = await session(); + const building = createTour(s.id, 'abandoned halfway', ''); + + deleteTour(building.id); + + expect(getTour(building.id)).toBeNull(); + }); +}); + +async function runAgent(args: string[]): Promise { + const { Command } = await import('commander'); + const { registerAgentCommands } = await import('../src/agent.js'); + const program = new Command(); + program.exitOverride(); + registerAgentCommands(program); + process.exitCode = 0; + await program.parseAsync(['node', 'diffity', 'agent', ...args]); + const code = process.exitCode ?? 0; + process.exitCode = 0; + return code; +} + +describe('the command that removes walkthroughs', () => { + // "Fix the walkthrough" used to be enough to wipe one a human had recorded, because the bulk + // form was the one you got by leaving the id off. + it('refuses to do anything without an id', async () => { + const { createTour, getToursForSession } = await import('../src/tours.js'); + const s = await session(); + createTour(s.id, 'someone else work', ''); + + const code = await runAgent(['tour-delete']); + + expect(code).toBe(1); + expect(getToursForSession(s.id).length).toBeGreaterThan(0); + }); + + it('sweeps the session when asked to', async () => { + const { createTour, updateTourStatus, getToursForSession } = await import('../src/tours.js'); + const s = await session(); + const finished = createTour(s.id, 'finished', ''); + updateTourStatus(finished.id, 'ready'); + + await runAgent(['tour-delete', '--all']); + + expect(getToursForSession(s.id).some(t => t.id === finished.id)).toBe(false); + }); + + it('leaves an in-flight walkthrough for --all alone', async () => { + const { createTour, getTour } = await import('../src/tours.js'); + const s = await session(); + const building = createTour(s.id, 'mid-flight', ''); + + await runAgent(['tour-delete', '--all']); + + expect(getTour(building.id)).not.toBeNull(); + }); + + it('takes the in-flight one too when told', async () => { + const { createTour, getTour } = await import('../src/tours.js'); + const s = await session(); + const building = createTour(s.id, 'mid-flight', ''); + + await runAgent(['tour-delete', '--all', '--include-building']); + + expect(getTour(building.id)).toBeNull(); + }); + + it('removes just the one it is given', async () => { + const { createTour, getTour } = await import('../src/tours.js'); + const s = await session(); + const doomed = createTour(s.id, 'doomed', ''); + const kept = createTour(s.id, 'kept', ''); + + await runAgent(['tour-delete', doomed.id]); + + expect(getTour(doomed.id)).toBeNull(); + expect(getTour(kept.id)).not.toBeNull(); + }); +}); diff --git a/packages/github/src/pr-url.ts b/packages/github/src/pr-url.ts index 18bf421..3f5deee 100644 --- a/packages/github/src/pr-url.ts +++ b/packages/github/src/pr-url.ts @@ -1,5 +1,5 @@ import { execFileSync } from 'node:child_process'; -import { exec } from './exec.js'; +import { exec, gh } from './exec.js'; import type { PrBase } from './types.js'; const PR_URL_REGEX = /(?:https?:\/\/)?github\.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/; @@ -21,9 +21,11 @@ export function parseGitHubPrUrl(url: string): { owner: string; repo: string; nu }; } -export function checkoutPr(prNumber: number): void { +export function checkoutPr(prNumber: number, owner: string, repo: string): void { try { - exec(`gh pr checkout ${prNumber}`); + // Pinned to the repository asked for: gh resolves a fork's parent, so an unpinned checkout + // fetches whichever pull request happens to carry that number upstream. + gh(['pr', 'checkout', String(prNumber), '--repo', `${owner}/${repo}`]); return; } catch (err) { // A merged pull request usually has its branch deleted, and gh can only check out a branch. @@ -50,6 +52,8 @@ export function parsePrBase(json: string): PrBase { return { name: baseRefName, oid: baseRefOid }; } -export function getPrBase(prNumber: number): PrBase { - return parsePrBase(exec(`gh pr view ${prNumber} --json baseRefName,baseRefOid`)); +export function getPrBase(prNumber: number, owner: string, repo: string): PrBase { + return parsePrBase( + gh(['pr', 'view', String(prNumber), '--repo', `${owner}/${repo}`, '--json', 'baseRefName,baseRefOid']), + ); } diff --git a/packages/github/src/pr.ts b/packages/github/src/pr.ts index b26f656..82474e2 100644 --- a/packages/github/src/pr.ts +++ b/packages/github/src/pr.ts @@ -143,6 +143,7 @@ export function createReview( const errors: string[] = []; const comments: ReviewCommentPayload[] = []; + const sentThreadIds: string[] = []; let skipped = 0; for (const comment of submission.comments) { @@ -164,13 +165,16 @@ export function createReview( continue; } comments.push(toReviewComment(comment)); + if (comment.threadId) { + sentThreadIds.push(comment.threadId); + } } const dropped = errors.length; const body = submission.body.trim(); if (comments.length === 0 && !body && submission.event === 'COMMENT') { - return { submitted: 0, skipped, failed: dropped, errors, reviewUrl: null }; + return { submitted: 0, submittedThreadIds: [], skipped, failed: dropped, errors, reviewUrl: null }; } try { @@ -186,6 +190,7 @@ export function createReview( const review = JSON.parse(raw) as { html_url?: string }; return { submitted: comments.length, + submittedThreadIds: sentThreadIds, skipped, failed: dropped, errors, @@ -196,6 +201,7 @@ export function createReview( const ghLine = msg.split('\n').find(line => line.includes('gh:')); return { submitted: 0, + submittedThreadIds: [], skipped, failed: dropped + comments.length, errors: [...errors, ghLine ? ghLine.trim() : 'GitHub rejected the review'], diff --git a/packages/github/src/types.ts b/packages/github/src/types.ts index df6c721..864f4aa 100644 --- a/packages/github/src/types.ts +++ b/packages/github/src/types.ts @@ -27,6 +27,8 @@ export interface ReviewSubmission { export interface ReviewResult { submitted: number; + /** The findings that actually left the machine, which is not every one that was offered. */ + submittedThreadIds: string[]; skipped: number; failed: number; errors: string[]; @@ -68,6 +70,8 @@ export interface PulledThread { } export interface PrComment { + /** The finding this came from, so a successful review can mark it as sent. */ + threadId?: string; filePath: string; side: 'LEFT' | 'RIGHT'; startLine: number | null; diff --git a/packages/skills/diffity-review/SKILL.md b/packages/skills/diffity-review/SKILL.md index dd723d8..51a303f 100644 --- a/packages/skills/diffity-review/SKILL.md +++ b/packages/skills/diffity-review/SKILL.md @@ -32,6 +32,7 @@ You are reviewing a diff and leaving inline comments using the `{{binary}} agent {{binary}} agent tour-start --topic "" [--body ""] --json {{binary}} agent tour-step --tour --file --line [--end-line ] --body "" [--annotation ""] {{binary}} agent tour-done --tour +{{binary}} agent tour-delete [] ``` - `--file`, `--line`, `--body` are required for `comment` @@ -279,6 +280,9 @@ one, unless the change is a single file: 3. The `--annotation` becomes the file's label in the reordered file list, so make it say *why* this file is read at this point ("the primitive", "first consumer", "where the P1 lives") rather than restating its name. Point a step at the most important lines in the file, not line 1. +4. Check what you recorded — `{{binary}} agent tour-start --json` and the steps you added. If a step + went in wrong, `{{binary}} agent tour-delete` and build it again. Adding a second walkthrough + leaves the wrong one in place, and the reader only ever sees the newest. ### Step 5: Open the browser diff --git a/packages/ui/src/components/comments/types.ts b/packages/ui/src/components/comments/types.ts index ea9077f..2b4e878 100644 --- a/packages/ui/src/components/comments/types.ts +++ b/packages/ui/src/components/comments/types.ts @@ -29,6 +29,8 @@ export interface CommentThread { status: ThreadStatus; anchorContent?: string; updatedAt?: string; + /** Set once this finding has been sent to the forge. Sending is not resolving. */ + submittedAt?: string | null; sessionId?: string; } diff --git a/packages/ui/src/components/layout/github-dialog.tsx b/packages/ui/src/components/layout/github-dialog.tsx index cbe77d1..acddcec 100644 --- a/packages/ui/src/components/layout/github-dialog.tsx +++ b/packages/ui/src/components/layout/github-dialog.tsx @@ -16,6 +16,7 @@ import type { CommentThread } from '../comments/types'; import { canSubmitReview, isSubmittable, + wasSubmitted, summaryFromGeneralThreads, threadToPayload, } from '../../lib/review-submission'; @@ -62,7 +63,7 @@ export function GitHubDialog(props: GitHubDialogProps) { setSelected(prev => { const next = new Set(prev); for (const thread of submittable) { - if (!prev.has(`-${thread.id}`)) { + if (!prev.has(`-${thread.id}`) && !wasSubmitted(thread)) { next.add(thread.id); } } @@ -255,6 +256,11 @@ export function GitHubDialog(props: GitHubDialogProps) { {lineLabel(thread)} + {wasSubmitted(thread) && ( + + already on the pull request + + )} {thread.comments.length > 1 && ( +{thread.comments.length - 1} repl{thread.comments.length === 2 ? 'y' : 'ies'} diff --git a/packages/ui/src/lib/api.ts b/packages/ui/src/lib/api.ts index d610e51..7192284 100644 --- a/packages/ui/src/lib/api.ts +++ b/packages/ui/src/lib/api.ts @@ -232,6 +232,7 @@ export async function fetchFileContent(filePath: string, ref?: string): Promise< } export interface PrCommentPayload { + threadId?: string; filePath: string; side: 'LEFT' | 'RIGHT'; startLine: number | null; @@ -251,6 +252,7 @@ export type ReviewEvent = 'COMMENT' | 'APPROVE' | 'REQUEST_CHANGES'; export interface CreateReviewResult { submitted: number; + submittedThreadIds: string[]; skipped: number; failed: number; errors: string[]; diff --git a/packages/ui/src/lib/review-submission.ts b/packages/ui/src/lib/review-submission.ts index 088253d..7a14f12 100644 --- a/packages/ui/src/lib/review-submission.ts +++ b/packages/ui/src/lib/review-submission.ts @@ -13,6 +13,7 @@ export function threadToPayload(thread: CommentThread): PrCommentPayload { : first.body; return { + threadId: thread.id, filePath: thread.filePath, side: thread.side === 'old' ? 'LEFT' : 'RIGHT', startLine: thread.startLine !== thread.endLine ? thread.startLine : null, @@ -60,3 +61,8 @@ export function canSubmitReview(input: { return input.comments > 0 || input.summary.trim().length > 0; } + +/** Already on the pull request, so resending has to be asked for rather than assumed. */ +export function wasSubmitted(thread: CommentThread): boolean { + return !!thread.submittedAt; +} diff --git a/packages/ui/tests/review-submission.test.ts b/packages/ui/tests/review-submission.test.ts index 23ffa81..677b8dc 100644 --- a/packages/ui/tests/review-submission.test.ts +++ b/packages/ui/tests/review-submission.test.ts @@ -29,6 +29,7 @@ function thread(overrides: Partial = {}): CommentThread { describe('threadToPayload', () => { it('maps a single-line comment on the new side', () => { expect(threadToPayload(thread())).toEqual({ + threadId: 't1', filePath: 'src/a.ts', side: 'RIGHT', startLine: null, diff --git a/packages/ui/tests/submitted-threads.test.tsx b/packages/ui/tests/submitted-threads.test.tsx new file mode 100644 index 0000000..31ca813 --- /dev/null +++ b/packages/ui/tests/submitted-threads.test.tsx @@ -0,0 +1,87 @@ +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, cleanup, screen } from '@testing-library/react'; +import { GitHubDialog } from '../src/components/layout/github-dialog'; +import type { GitHubDetails } from '../src/lib/api'; +import type { CommentThread } from '../src/components/comments/types'; +import { isSubmittable, threadToPayload } from '../src/lib/review-submission'; + +const details: GitHubDetails = { + prNumber: 14378, + prTitle: 'feat: Apple Watch raw data mapping A2', + prUrl: 'https://github.com/o/r/pull/14378', + prCreatedAt: '2026-08-21T10:00:00.000Z', + headSha: 'abc123', + commentCount: 2, + viewerDidAuthor: false, + prBody: '', + reviews: [], +}; + +function thread(id: string, body: string, submittedAt: string | null = null): CommentThread { + return { + id, + filePath: 'src/a.ts', + side: 'new', + startLine: 10, + endLine: 10, + status: 'open', + submittedAt, + comments: [{ id: `c-${id}`, author: { name: 'Agent', type: 'agent' }, body, createdAt: '' }], + }; +} + +function renderDialog(threads: CommentThread[]) { + return render( + , + ); +} + +function submitButton(): HTMLButtonElement { + return screen + .getAllByRole('button') + .find(b => /^submit/i.test((b.textContent ?? '').trim())) as HTMLButtonElement; +} + +afterEach(cleanup); + +describe('a finding already sent to the forge', () => { + it('says so, rather than looking identical to an unsent one', () => { + renderDialog([thread('t1', 'P2: already sent', '2026-08-21 16:01:10')]); + + expect(screen.getByText(/already on the pull request/i)).toBeTruthy(); + }); + + it('is not selected, so submitting again is a deliberate act', () => { + renderDialog([thread('t1', 'P2: already sent', '2026-08-21 16:01:10')]); + + const boxes = screen.getAllByRole('checkbox') as HTMLInputElement[]; + expect(boxes.every(box => !box.checked)).toBe(true); + // Nothing selected and no summary, so a plain comment review has nothing to say. + expect(submitButton().disabled).toBe(true); + }); + + it('leaves an unsent finding selected', () => { + renderDialog([thread('t1', 'P2: not sent', null)]); + + const boxes = screen.getAllByRole('checkbox') as HTMLInputElement[]; + expect(boxes.some(box => box.checked)).toBe(true); + expect(submitButton().disabled).toBe(false); + }); +}); + +describe('the payload', () => { + it('carries the finding id, so the forge reply can mark it', () => { + expect(threadToPayload(thread('t9', 'P1: x')).threadId).toBe('t9'); + }); + + it('still offers an already-sent finding, in case it must go again', () => { + // Excluded from the default selection, not from the list: a reviewer may resend deliberately. + expect(isSubmittable(thread('t1', 'x', '2026-08-21 16:01:10'))).toBe(true); + }); +}); diff --git a/skills/diffity-review/SKILL.md b/skills/diffity-review/SKILL.md index 6f16cd4..2cfb7f2 100644 --- a/skills/diffity-review/SKILL.md +++ b/skills/diffity-review/SKILL.md @@ -32,6 +32,7 @@ diffity agent reply --body "" diffity agent tour-start --topic "" [--body ""] --json diffity agent tour-step --tour --file --line [--end-line ] --body "" [--annotation ""] diffity agent tour-done --tour +diffity agent tour-delete [] ``` - `--file`, `--line`, `--body` are required for `comment` @@ -279,6 +280,9 @@ one, unless the change is a single file: 3. The `--annotation` becomes the file's label in the reordered file list, so make it say *why* this file is read at this point ("the primitive", "first consumer", "where the P1 lives") rather than restating its name. Point a step at the most important lines in the file, not line 1. +4. Check what you recorded — `diffity agent tour-start --json` and the steps you added. If a step + went in wrong, `diffity agent tour-delete` and build it again. Adding a second walkthrough + leaves the wrong one in place, and the reader only ever sees the newest. ### Step 5: Open the browser