diff --git a/README.md b/README.md index 34c0e05..05fdaaf 100644 --- a/README.md +++ b/README.md @@ -276,6 +276,24 @@ Point it **outside** the working tree, or add it to `.gitignore`. Otherwise the untracked files in the very diff you are reviewing; diffity warns on startup when that happens. The database quotes the code under review, so it is created readable only by you. +## Review standards + +An agent reviewing a diff can be told what this project reviews against, so the standards live with +the code rather than in one person's agent configuration: + +```json +{ + "review": { + "severities": ["P1", "P2", "P3"], + "standards": ".claude/skills/code-review/SKILL.md" + } +} +``` + +`severities` are the labels findings are prefixed with, most severe first, defaulting to +`P1`/`P2`/`P3`. `standards` is a repository-relative path to a document the agent reads before +reviewing. `diffity agent standards` prints both, and the review skill reads it first. + ## License [PolyForm Shield 1.0.0](./LICENSE) © [Kamran Ahmed](https://x.com/kamrify) diff --git a/packages/cli/src/agent.ts b/packages/cli/src/agent.ts index c07cfd1..fe984b1 100644 --- a/packages/cli/src/agent.ts +++ b/packages/cli/src/agent.ts @@ -14,6 +14,9 @@ import { type Thread, } from './threads.js'; import { createTour, addTourStep, updateTourStatus } from './tours.js'; +import { readAnchor } from './anchor.js'; +import { readRepoConfig, DEFAULT_SEVERITIES, resolveInRepo, REPO_CONFIG_FILE } from '@diffity/git'; +import { readFileSync } from 'node:fs'; function requireSession() { if (!isGitRepo()) { @@ -164,6 +167,8 @@ Examples: endLine, opts.body, { name: 'Agent', type: 'agent' }, + // Recorded so the finding can follow its code when a later commit moves it. + opts.side === 'new' ? readAnchor(opts.file, opts.line, endLine) : undefined, ); console.log(pc.green(`Created thread ${thread.id.slice(0, 8)}`)); }); @@ -238,6 +243,46 @@ Examples: process.stdout.write(raw); }); + agent + .command('standards') + .description("Print the project's review standards and severity labels") + .option('--json', 'Output as JSON') + .action((opts) => { + if (!isGitRepo()) { + console.error(pc.red('Error: Not a git repository')); + process.exit(1); + } + + const { review } = readRepoConfig(getRepoRoot()); + const severities = review?.severities ?? DEFAULT_SEVERITIES; + let standards: { path: string; content: string } | null = null; + + if (review?.standards) { + try { + standards = { + path: review.standards, + content: readFileSync(resolveInRepo(review.standards), 'utf-8'), + }; + } catch { + console.error(pc.yellow(`Warning: cannot read ${review.standards} from ${REPO_CONFIG_FILE}`)); + } + } + + if (opts.json) { + console.log(JSON.stringify({ severities, standards }, null, 2)); + return; + } + + console.log(`Severities: ${severities.join(', ')}`); + if (standards) { + console.log(`Standards: ${standards.path}`); + console.log(''); + console.log(standards.content); + } else { + console.log(pc.dim(`No review standards configured in ${REPO_CONFIG_FILE}.`)); + } + }); + agent .command('tour-start') .description('Start a new guided tour of the codebase') diff --git a/packages/cli/src/anchor.ts b/packages/cli/src/anchor.ts new file mode 100644 index 0000000..1c0850f --- /dev/null +++ b/packages/cli/src/anchor.ts @@ -0,0 +1,67 @@ +import { getWorkingTreeFileContent } from '@diffity/git'; + +export interface AnchorRange { + startLine: number; + endLine: number; +} + +/** + * The source lines a comment is attached to, in the same shape the browser stores: the lines + * themselves, joined, with no line numbers. + */ +export function readAnchor(filePath: string, startLine: number, endLine: number): string | undefined { + try { + const lines = getWorkingTreeFileContent(filePath).split('\n'); + const anchor = lines.slice(startLine - 1, endLine).join('\n'); + return anchor || undefined; + } catch { + return undefined; + } +} + +/** + * Finds where a comment's lines went after the file changed under it. + * + * The match is exact: a line that was edited is a different line, and guessing at similarity + * would move a comment onto code it was not written about. When the same lines appear more than + * once, the occurrence nearest to where the comment used to be wins. + */ +export function reanchor( + anchorContent: string, + fileLines: string[], + originalStartLine: number, +): AnchorRange | null { + const anchorLines = anchorContent.split('\n'); + if (anchorContent === '' || anchorLines.length === 0) { + return null; + } + + const matches: number[] = []; + for (let i = 0; i + anchorLines.length <= fileLines.length; i++) { + if (anchorLines.every((line, offset) => fileLines[i + offset] === line)) { + matches.push(i + 1); + } + } + + if (matches.length === 0) { + return null; + } + + const startLine = matches.reduce((best, candidate) => + Math.abs(candidate - originalStartLine) < Math.abs(best - originalStartLine) ? candidate : best, + ); + + return { startLine, endLine: startLine + anchorLines.length - 1 }; +} + +export function reanchorInWorkingTree( + filePath: string, + anchorContent: string, + originalStartLine: number, +): AnchorRange | null { + try { + return reanchor(anchorContent, getWorkingTreeFileContent(filePath).split('\n'), originalStartLine); + } catch { + return null; + } +} diff --git a/packages/cli/src/session.ts b/packages/cli/src/session.ts index fa05100..a0933ed 100644 --- a/packages/cli/src/session.ts +++ b/packages/cli/src/session.ts @@ -2,7 +2,9 @@ import { randomUUID } from 'node:crypto'; import { readFileSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { getHeadHash, getDiffityDir } from '@diffity/git'; -import { getDb, queryOne } from './db.js'; +import { getDb, queryAll, queryOne } from './db.js'; +import { reanchorInWorkingTree } from './anchor.js'; +import { updateThreadLines } from './threads.js'; export interface Session { id: string; @@ -71,6 +73,32 @@ export function carryForward(fromSessionId: string, toSessionId: string): void { toSessionId, fromSessionId, ); + + reanchorThreads(toSessionId); +} + +/** + * A finding that outlives the commit it was written against points at a line that has since + * moved. Only the new side is re-anchored: a comment on a removed line has nothing to follow. + */ +function reanchorThreads(sessionId: string): void { + const threads = queryAll<{ + id: string; + file_path: string; + side: string; + start_line: number; + anchor_content: string | null; + }>( + "SELECT id, file_path, side, start_line, anchor_content FROM comment_threads WHERE session_id = ? AND status = 'open' AND side = 'new' AND anchor_content IS NOT NULL", + sessionId, + ); + + for (const thread of threads) { + const moved = reanchorInWorkingTree(thread.file_path, thread.anchor_content!, thread.start_line); + if (moved && moved.startLine !== thread.start_line) { + updateThreadLines(thread.id, moved.startLine, moved.endLine); + } + } } export function getCurrentSession(): Session | null { diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index d292a1e..94a291f 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -101,6 +101,15 @@ function getCommentsForThread(threadId: string): ThreadComment[] { return map.get(threadId) ?? []; } +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( + startLine, + endLine, + threadId, + ); +} + export function createThread( sessionId: string, filePath: string, diff --git a/packages/cli/tests/anchor.test.ts b/packages/cli/tests/anchor.test.ts new file mode 100644 index 0000000..1e20ab3 --- /dev/null +++ b/packages/cli/tests/anchor.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect } from 'vitest'; +import { reanchor } from '../src/anchor.js'; + +const file = [ + 'import { a } from "a";', + '', + 'function first() {', + ' return 1;', + '}', + '', + 'function second() {', + ' return 2;', + '}', +]; + +describe('reanchor', () => { + it('finds the lines where they now are', () => { + const moved = reanchor('function second() {\n return 2;', ['// new header', ...file], 7); + + expect(moved).toEqual({ startLine: 8, endLine: 9 }); + }); + + it('leaves a line that has not moved alone', () => { + expect(reanchor(' return 1;', file, 4)).toEqual({ startLine: 4, endLine: 4 }); + }); + + it('gives up when the line was edited rather than moved', () => { + expect(reanchor(' return 1; // changed', file, 4)).toBeNull(); + }); + + it('gives up when the code is gone', () => { + expect(reanchor('function third() {', file, 4)).toBeNull(); + }); + + it('takes the occurrence nearest to where the comment was', () => { + const duplicated = [' return 1;', 'x', ' return 1;', 'y', ' return 1;']; + + expect(reanchor(' return 1;', duplicated, 3)?.startLine).toBe(3); + expect(reanchor(' return 1;', duplicated, 5)?.startLine).toBe(5); + expect(reanchor(' return 1;', duplicated, 1)?.startLine).toBe(1); + }); + + it('refuses an empty anchor rather than matching everywhere', () => { + expect(reanchor('', file, 1)).toBeNull(); + }); + + it('handles an anchor longer than the file', () => { + expect(reanchor(file.join('\n') + '\nextra', file, 1)).toBeNull(); + }); +}); diff --git a/packages/cli/tests/session-continuity.test.ts b/packages/cli/tests/session-continuity.test.ts index 30f09c3..6ad6dba 100644 --- a/packages/cli/tests/session-continuity.test.ts +++ b/packages/cli/tests/session-continuity.test.ts @@ -71,6 +71,69 @@ describe('a session when HEAD moves', () => { expect(left.map(thread => thread.id)).toEqual([dealtWith.id]); }); + it('follows its code when a later commit moves it', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, getThreadsForSession } = await import('../src/threads.js'); + + writeFileSync(join(repoDir, 'moving.ts'), 'one\ntwo\nthree\n'); + git(['add', '.']); + git(['commit', '-m', 'moving.ts']); + + const before = findOrCreateSession('work'); + const finding = createThread( + before.id, + 'moving.ts', + 'new', + 2, + 2, + 'P2: about the second line', + { name: 'Agent', type: 'agent' }, + 'two', + ); + + // Two lines land above it, so the code it points at is now on line 4. + writeFileSync(join(repoDir, 'moving.ts'), 'inserted\nalso inserted\none\ntwo\nthree\n'); + git(['add', '.']); + git(['commit', '-m', 'insert above']); + + const after = findOrCreateSession('work'); + const carried = getThreadsForSession(after.id).find(thread => thread.id === finding.id); + + expect(carried?.startLine).toBe(4); + expect(carried?.endLine).toBe(4); + }); + + it('leaves a finding where it is when its code was edited rather than moved', async () => { + const { findOrCreateSession } = await import('../src/session.js'); + const { createThread, getThreadsForSession } = await import('../src/threads.js'); + + writeFileSync(join(repoDir, 'edited.ts'), 'keep\ntarget\nkeep\n'); + git(['add', '.']); + git(['commit', '-m', 'edited.ts']); + + const before = findOrCreateSession('work'); + const finding = createThread( + before.id, + 'edited.ts', + 'new', + 2, + 2, + 'P3: about the target', + { name: 'Agent', type: 'agent' }, + 'target', + ); + + writeFileSync(join(repoDir, 'edited.ts'), 'keep\ntarget changed\nkeep\n'); + git(['add', '.']); + git(['commit', '-m', 'edit the target']); + + const after = findOrCreateSession('work'); + const carried = getThreadsForSession(after.id).find(thread => thread.id === finding.id); + + expect(carried).toBeDefined(); + expect(carried?.startLine).toBe(2); + }); + it('returns the same session while HEAD stays put', async () => { const { findOrCreateSession } = await import('../src/session.js'); diff --git a/packages/git/src/config.ts b/packages/git/src/config.ts index 9c1ecb5..66852f3 100644 --- a/packages/git/src/config.ts +++ b/packages/git/src/config.ts @@ -4,14 +4,28 @@ import { isAbsolute, join, resolve } from 'node:path'; export const REPO_CONFIG_FILE = '.diffity.json'; +export interface ReviewConfig { + /** Severity labels a reviewer should use, most severe first. */ + severities?: string[]; + /** + * Repository-relative path to the project's own review standards, for an agent to read + * before reviewing. Keeping them in the repository means they are versioned with the code + * and shared by everyone, rather than living in one person's agent configuration. + */ + standards?: string; +} + export interface RepoConfig { /** * Where review threads, walkthroughs and sessions are kept. Relative paths resolve against * the repository root, so a project can keep its review notes with itself. */ dataDir?: string; + review?: ReviewConfig; } +export const DEFAULT_SEVERITIES = ['P1', 'P2', 'P3']; + export function readRepoConfig(repoRoot: string): RepoConfig { const path = join(repoRoot, REPO_CONFIG_FILE); if (!existsSync(path)) { @@ -20,13 +34,42 @@ export function readRepoConfig(repoRoot: string): RepoConfig { try { const parsed = JSON.parse(readFileSync(path, 'utf-8')) as RepoConfig; - return typeof parsed?.dataDir === 'string' ? { dataDir: parsed.dataDir } : {}; + const config: RepoConfig = {}; + + if (typeof parsed?.dataDir === 'string') { + config.dataDir = parsed.dataDir; + } + + const review = readReviewConfig(parsed?.review); + if (review) { + config.review = review; + } + + return config; } catch { // A malformed config must not stop a review; the default is always usable. return {}; } } +function readReviewConfig(raw: unknown): ReviewConfig | undefined { + if (!raw || typeof raw !== 'object') { + return undefined; + } + + const { severities, standards } = raw as ReviewConfig; + const review: ReviewConfig = {}; + + if (Array.isArray(severities) && severities.every(s => typeof s === 'string') && severities.length > 0) { + review.severities = severities; + } + if (typeof standards === 'string' && standards) { + review.standards = standards; + } + + return Object.keys(review).length > 0 ? review : undefined; +} + /** * The hashed subdirectory only exists to keep repositories apart inside the shared default * location. A data directory chosen for one project needs no such disambiguation, so it is diff --git a/packages/git/src/index.ts b/packages/git/src/index.ts index a876586..52ba783 100644 --- a/packages/git/src/index.ts +++ b/packages/git/src/index.ts @@ -5,7 +5,7 @@ export { getDiff, getDiffFiles, getDiffStat, getDiffStatForRef, getUntrackedFile export type { RefDiffArgs } from './diff.js'; export { getStagedFiles, getUnstagedFiles, isDirty } from './status.js'; export { getRecentCommits } from './commits.js'; -export { readRepoConfig, resolveDataDir, REPO_CONFIG_FILE } from './config.js'; -export type { RepoConfig } from './config.js'; +export { readRepoConfig, resolveDataDir, REPO_CONFIG_FILE, DEFAULT_SEVERITIES } from './config.js'; +export type { RepoConfig, ReviewConfig } from './config.js'; export { getTree, getTreeEntries, getTreeFingerprint, getWorkingTreeFileContent, getWorkingTreeRawFile, resolveInRepo } from './tree.js'; export type { TreeEntry } from './tree.js'; diff --git a/packages/git/tests/data-dir.test.ts b/packages/git/tests/data-dir.test.ts index efe18d6..4bde6ba 100644 --- a/packages/git/tests/data-dir.test.ts +++ b/packages/git/tests/data-dir.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { readRepoConfig, resolveDataDir } from '../src/config.js'; +import { readRepoConfig, resolveDataDir, DEFAULT_SEVERITIES } from '../src/config.js'; let repoRoot: string; const homeDir = '/home/someone'; @@ -82,3 +82,44 @@ describe('readRepoConfig', () => { expect(readRepoConfig(repoRoot)).toEqual({}); }); }); + +describe('review config', () => { + it('reads severities and a standards path', () => { + writeFileSync( + join(repoRoot, '.diffity.json'), + JSON.stringify({ review: { severities: ['blocker', 'nit'], standards: 'docs/review.md' } }), + ); + + expect(readRepoConfig(repoRoot).review).toEqual({ + severities: ['blocker', 'nit'], + standards: 'docs/review.md', + }); + }); + + it('leaves review absent when the section is missing', () => { + writeFileSync(join(repoRoot, '.diffity.json'), JSON.stringify({ dataDir: '.notes' })); + + expect(readRepoConfig(repoRoot)).toEqual({ dataDir: '.notes' }); + }); + + it('ignores an empty or wrongly typed severity list', () => { + writeFileSync(join(repoRoot, '.diffity.json'), JSON.stringify({ review: { severities: [] } })); + expect(readRepoConfig(repoRoot).review).toBeUndefined(); + + writeFileSync(join(repoRoot, '.diffity.json'), JSON.stringify({ review: { severities: [1, 2] } })); + expect(readRepoConfig(repoRoot).review).toBeUndefined(); + }); + + it('keeps a valid half when the other is malformed', () => { + writeFileSync( + join(repoRoot, '.diffity.json'), + JSON.stringify({ review: { severities: ['P1'], standards: 42 } }), + ); + + expect(readRepoConfig(repoRoot).review).toEqual({ severities: ['P1'] }); + }); + + it('offers a default vocabulary', () => { + expect(DEFAULT_SEVERITIES).toEqual(['P1', 'P2', 'P3']); + }); +}); diff --git a/packages/skills/diffity-review/SKILL.md b/packages/skills/diffity-review/SKILL.md index 07c9f9f..786a7ca 100644 --- a/packages/skills/diffity-review/SKILL.md +++ b/packages/skills/diffity-review/SKILL.md @@ -16,6 +16,7 @@ You are reviewing a diff and leaving inline comments using the `{{binary}} agent ## CLI Reference ``` +{{binary}} agent standards [--json] {{binary}} agent diff {{binary}} agent list [--status open|resolved|dismissed] [--json] {{binary}} agent comment --file --line [--end-line ] [--side new|old] --body "" @@ -23,6 +24,9 @@ You are reviewing a diff and leaving inline comments using the `{{binary}} agent {{binary}} agent resolve [--summary ""] {{binary}} agent dismiss [--reason ""] {{binary}} agent reply --body "" +{{binary}} agent tour-start --topic "" [--body ""] --json +{{binary}} agent tour-step --tour --file --line [--end-line ] --body "" [--annotation ""] +{{binary}} agent tour-done --tour ``` - `--file`, `--line`, `--body` are required for `comment` @@ -59,11 +63,16 @@ The review needs a running session whose ref matches the requested ref. A ref mi {{binary}} agent diff ``` This outputs the full unified diff for the current session. Line numbers are in the `@@` hunk headers. -2. Find and read all relevant CLAUDE.md files — the root CLAUDE.md and any CLAUDE.md files in directories containing modified files. These define project-specific rules that the diff must follow. +2. **Read the project's review standards.** Run `{{binary}} agent standards`. A project can point + `review.standards` in `.diffity.json` at its own standards document, and set `review.severities` + to the labels its reviewers use. Whatever it prints outranks the generic guidance in this skill: + it is what this team has agreed to review against. If nothing is configured, carry on with the + defaults below. +3. Find and read all relevant CLAUDE.md files — the root CLAUDE.md and any CLAUDE.md files in directories containing modified files. These define project-specific rules that the diff must follow. #### Assess the change size and adapt your strategy -3. **Gauge the diff size** and plan your approach. Every file gets a thorough review regardless of diff size — the difference is how you organize the work: +4. **Gauge the diff size** and plan your approach. Every file gets a thorough review regardless of diff size — the difference is how you organize the work: - **Small** (under ~100 changed lines, 1-3 files): Straightforward — review each file in order. - **Medium** (100-500 changed lines, 3-10 files): Group files by area (e.g. backend, frontend, tests, config). Review core logic files first so you understand intent before reviewing the ripple effects. - **Large** (500+ changed lines or 10+ files): Group files by area. Start with core logic, then review every remaining file. For mechanically repetitive changes (e.g. the same rename applied to 20 files), verify the pattern is correct on the first few instances, then check every remaining instance for deviations from the pattern — don't skip any, but you can check them faster once the pattern is established. @@ -72,7 +81,7 @@ The review needs a running session whose ref matches the requested ref. A ref mi #### Understand the change before reviewing it -4. **Summarize the change first.** Before looking for problems, build a mental model of the diff: +5. **Summarize the change first.** Before looking for problems, build a mental model of the diff: - What is this change trying to accomplish? (new feature, bug fix, refactor, config change) - Which files are structural changes vs. the core logic change? - What is the author's intent? Read commit messages (`git log --oneline `) and any linked issues or PR descriptions for context. @@ -80,12 +89,12 @@ The review needs a running session whose ref matches the requested ref. A ref mi Understanding intent helps you distinguish intentional behavior from real bugs. -5. For each changed file (adjusted by size strategy above), read the **entire file** (not just the diff hunks) to understand the full context. -6. **Cross-reference callers and dependents.** For any changed function signature, renamed export, modified return type, or altered behavior: grep for usages across the codebase. A function that looks correct in isolation can break every caller. Check: +6. For each changed file (adjusted by size strategy above), read the **entire file** (not just the diff hunks) to understand the full context. +7. **Cross-reference callers and dependents.** For any changed function signature, renamed export, modified return type, or altered behavior: grep for usages across the codebase. A function that looks correct in isolation can break every caller. Check: - Who calls this function? Will they handle the new return value / error / null case? - Who imports this module? Will the changed export name resolve? - Does this type change propagate correctly to consumers? -7. Analyze the code changes using the techniques below. If a `focus` argument was provided, concentrate on that area. Otherwise, apply all analysis passes and the signal threshold. +8. Analyze the code changes using the techniques below. If a `focus` argument was provided, concentrate on that area. Otherwise, apply all analysis passes and the signal threshold. #### How to analyze @@ -170,13 +179,20 @@ If a repeated pattern appears across files, comment on the first occurrence and ### Step 3: Leave comments -1. **Order comments by severity.** Post all `[must-fix]` comments first, then `[suggestion]`, then `[question]`. Within each severity, follow file order. This ensures the most important issues are seen first if the author skims. +1. **Order comments by severity**, most severe first, and within a severity follow file order. The + most important issues are then seen first by someone who skims. -2. Categorize each finding with a severity prefix in the comment body: +2. Prefix each finding with its severity. Use the labels `{{binary}} agent standards` printed — they + are what this project's reviewers read, and matching them is what makes a review usable rather + than merely correct. `P1: …`, `P2: …`, `P3: …` are the default. Only when a project configures + nothing, fall back to: - `[must-fix]` — Bugs, security issues, data loss risks. Code that will break or produce wrong results. - `[suggestion]` — Concrete improvements with a clear reason. Not style preferences — real improvements. This includes missing tests, incomplete changes, and better approaches. - `[question]` — Something unclear that needs clarification from the author. + Whichever vocabulary applies, the most severe label means *this must not merge*. Do not inflate: + a review where everything is severe tells the reader nothing. + 3. For each finding, leave an inline comment using: ``` {{binary}} agent comment --file --line [--end-line ] [--side new] --body "" @@ -199,7 +215,25 @@ If a repeated pattern appears across files, comment on the first occurrence and {{binary}} agent general-comment --body "" ``` -### Step 4: Open the browser +### Step 4: Set the reading order + +A diff is served alphabetically, which is rarely the order it should be read in. Give the reader +one, unless the change is a single file: + +1. Decide the order someone should read the change in — the piece that explains the rest first, the + call sites and their ripple effects after, mechanical or signature-only files last. +2. Record it: + ``` + {{binary}} agent tour-start --topic "Reading order" --body "" --json + {{binary}} agent tour-step --tour --file --line [--end-line ] \ + --body "" --annotation "<3-6 words on why it is read here>" + {{binary}} agent tour-done --tour + ``` +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. + +### Step 5: Open the browser 1. Open the browser now that comments are ready: ``` @@ -210,6 +244,8 @@ If a repeated pattern appears across files, comment on the first occurrence and > Review complete — check your browser. > - > Found: 2 must-fix, 1 suggestion + > Found: 1 P1, 2 P2. The file list is in reading order; the P1 is on the last stop. > > When you're ready, run **{{slash}}resolve** to fix them. + + Report the counts using the same labels you used in the comments. diff --git a/skills/diffity-review/SKILL.md b/skills/diffity-review/SKILL.md index e0a9bde..d1eb2e7 100644 --- a/skills/diffity-review/SKILL.md +++ b/skills/diffity-review/SKILL.md @@ -16,6 +16,7 @@ You are reviewing a diff and leaving inline comments using the `diffity agent` C ## CLI Reference ``` +diffity agent standards [--json] diffity agent diff diffity agent list [--status open|resolved|dismissed] [--json] diffity agent comment --file --line [--end-line ] [--side new|old] --body "" @@ -23,6 +24,9 @@ diffity agent general-comment --body "" diffity agent resolve [--summary ""] diffity agent dismiss [--reason ""] 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 ``` - `--file`, `--line`, `--body` are required for `comment` @@ -59,11 +63,16 @@ The review needs a running session whose ref matches the requested ref. A ref mi diffity agent diff ``` This outputs the full unified diff for the current session. Line numbers are in the `@@` hunk headers. -2. Find and read all relevant CLAUDE.md files — the root CLAUDE.md and any CLAUDE.md files in directories containing modified files. These define project-specific rules that the diff must follow. +2. **Read the project's review standards.** Run `diffity agent standards`. A project can point + `review.standards` in `.diffity.json` at its own standards document, and set `review.severities` + to the labels its reviewers use. Whatever it prints outranks the generic guidance in this skill: + it is what this team has agreed to review against. If nothing is configured, carry on with the + defaults below. +3. Find and read all relevant CLAUDE.md files — the root CLAUDE.md and any CLAUDE.md files in directories containing modified files. These define project-specific rules that the diff must follow. #### Assess the change size and adapt your strategy -3. **Gauge the diff size** and plan your approach. Every file gets a thorough review regardless of diff size — the difference is how you organize the work: +4. **Gauge the diff size** and plan your approach. Every file gets a thorough review regardless of diff size — the difference is how you organize the work: - **Small** (under ~100 changed lines, 1-3 files): Straightforward — review each file in order. - **Medium** (100-500 changed lines, 3-10 files): Group files by area (e.g. backend, frontend, tests, config). Review core logic files first so you understand intent before reviewing the ripple effects. - **Large** (500+ changed lines or 10+ files): Group files by area. Start with core logic, then review every remaining file. For mechanically repetitive changes (e.g. the same rename applied to 20 files), verify the pattern is correct on the first few instances, then check every remaining instance for deviations from the pattern — don't skip any, but you can check them faster once the pattern is established. @@ -72,7 +81,7 @@ The review needs a running session whose ref matches the requested ref. A ref mi #### Understand the change before reviewing it -4. **Summarize the change first.** Before looking for problems, build a mental model of the diff: +5. **Summarize the change first.** Before looking for problems, build a mental model of the diff: - What is this change trying to accomplish? (new feature, bug fix, refactor, config change) - Which files are structural changes vs. the core logic change? - What is the author's intent? Read commit messages (`git log --oneline `) and any linked issues or PR descriptions for context. @@ -80,12 +89,12 @@ The review needs a running session whose ref matches the requested ref. A ref mi Understanding intent helps you distinguish intentional behavior from real bugs. -5. For each changed file (adjusted by size strategy above), read the **entire file** (not just the diff hunks) to understand the full context. -6. **Cross-reference callers and dependents.** For any changed function signature, renamed export, modified return type, or altered behavior: grep for usages across the codebase. A function that looks correct in isolation can break every caller. Check: +6. For each changed file (adjusted by size strategy above), read the **entire file** (not just the diff hunks) to understand the full context. +7. **Cross-reference callers and dependents.** For any changed function signature, renamed export, modified return type, or altered behavior: grep for usages across the codebase. A function that looks correct in isolation can break every caller. Check: - Who calls this function? Will they handle the new return value / error / null case? - Who imports this module? Will the changed export name resolve? - Does this type change propagate correctly to consumers? -7. Analyze the code changes using the techniques below. If a `focus` argument was provided, concentrate on that area. Otherwise, apply all analysis passes and the signal threshold. +8. Analyze the code changes using the techniques below. If a `focus` argument was provided, concentrate on that area. Otherwise, apply all analysis passes and the signal threshold. #### How to analyze @@ -170,13 +179,20 @@ If a repeated pattern appears across files, comment on the first occurrence and ### Step 3: Leave comments -1. **Order comments by severity.** Post all `[must-fix]` comments first, then `[suggestion]`, then `[question]`. Within each severity, follow file order. This ensures the most important issues are seen first if the author skims. +1. **Order comments by severity**, most severe first, and within a severity follow file order. The + most important issues are then seen first by someone who skims. -2. Categorize each finding with a severity prefix in the comment body: +2. Prefix each finding with its severity. Use the labels `diffity agent standards` printed — they + are what this project's reviewers read, and matching them is what makes a review usable rather + than merely correct. `P1: …`, `P2: …`, `P3: …` are the default. Only when a project configures + nothing, fall back to: - `[must-fix]` — Bugs, security issues, data loss risks. Code that will break or produce wrong results. - `[suggestion]` — Concrete improvements with a clear reason. Not style preferences — real improvements. This includes missing tests, incomplete changes, and better approaches. - `[question]` — Something unclear that needs clarification from the author. + Whichever vocabulary applies, the most severe label means *this must not merge*. Do not inflate: + a review where everything is severe tells the reader nothing. + 3. For each finding, leave an inline comment using: ``` diffity agent comment --file --line [--end-line ] [--side new] --body "" @@ -199,7 +215,25 @@ If a repeated pattern appears across files, comment on the first occurrence and diffity agent general-comment --body "" ``` -### Step 4: Open the browser +### Step 4: Set the reading order + +A diff is served alphabetically, which is rarely the order it should be read in. Give the reader +one, unless the change is a single file: + +1. Decide the order someone should read the change in — the piece that explains the rest first, the + call sites and their ripple effects after, mechanical or signature-only files last. +2. Record it: + ``` + diffity agent tour-start --topic "Reading order" --body "" --json + diffity agent tour-step --tour --file --line [--end-line ] \ + --body "" --annotation "<3-6 words on why it is read here>" + diffity agent tour-done --tour + ``` +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. + +### Step 5: Open the browser 1. Open the browser now that comments are ready: ``` @@ -210,6 +244,8 @@ If a repeated pattern appears across files, comment on the first occurrence and > Review complete — check your browser. > - > Found: 2 must-fix, 1 suggestion + > Found: 1 P1, 2 P2. The file list is in reading order; the P1 is on the last stop. > > When you're ready, run **/diffity-resolve** to fix them. + + Report the counts using the same labels you used in the comments.