diff --git a/.changeset/review-command.md b/.changeset/review-command.md new file mode 100644 index 000000000..1cfbce76c --- /dev/null +++ b/.changeset/review-command.md @@ -0,0 +1,5 @@ +--- +'@nanocollective/nanocoder': minor +--- + +Add /review slash command and `nanocoder review` CLI subcommand for AI-powered code review of branch diffs and PRs diff --git a/README.md b/README.md index 1a4760e1a..c89ce13ff 100644 --- a/README.md +++ b/README.md @@ -50,8 +50,14 @@ nanocoder --mode plan run "audit the auth module" # Fullscreen mode with in-app scrolling instead of the inline default nanocoder --alt-screen + +# Review a branch or PR for bugs, security issues, and style violations +nanocoder review main +nanocoder review 42 ``` +> **Note:** `nanocoder review` requires an interactive terminal (TTY). It cannot be used with pipes or redirection. + ### Screen Modes Nanocoder supports two rendering modes, mirroring what Claude Code and Codex ship: diff --git a/docs/features/commands.md b/docs/features/commands.md index aebd22e06..aa8cc592e 100644 --- a/docs/features/commands.md +++ b/docs/features/commands.md @@ -32,6 +32,7 @@ Type `/` in the chat input to see available commands. All commands start with `/ | `/export` | Export current session to markdown file | | `/copy` | Copy the last assistant response to the system clipboard | | `/commit` | Generate a Conventional Commit message from staged Git changes. Add `--copy` (or `-c`) to also copy the message to the system clipboard. A spinner shows while the model is working | +| `/review` | Review a branch or PR diff for bugs, security issues, and style violations. Usage: `/review ` (e.g. `/review main`, `/review 42`) | | `/doctor` | Show environment health report for bug reports | | `/update` | Update Nanocoder to the latest version | | `/usage` | Get current model context usage visually | diff --git a/docs/getting-started/index.md b/docs/getting-started/index.md index b848a61fa..f509319a3 100644 --- a/docs/getting-started/index.md +++ b/docs/getting-started/index.md @@ -62,6 +62,7 @@ nanocoder -h | `--resume [id]` | `-r` | Resume a [saved session](../features/session-management.md) by session ID, 1-based list index, or `last`. With no ID, opens the session picker at startup. Errors if the session is not found. Interactive only — errors with `run`. | | `init [--preset ]` | | Initialize the current project. Bundled presets: `react`, `nextjs`, and `rust` | | `run` | | Run in non-interactive mode | +| `review` | | Review a branch or PR diff for bugs, security, and style violations | **Provider/Model Flags:** @@ -178,6 +179,23 @@ nanocoder --provider ollama --model llama3.1 --context-max 128k run "analyze src nanocoder run --provider openrouter --model anthropic/claude-sonnet-4-20250514 "refactor database module" ``` +## Code Review + +Nanocoder provides AI-powered code review for branches and pull requests: + +```bash +# Review a branch +nanocoder review main +nanocoder review feature/auth + +# Review a PR (requires gh CLI) +nanocoder review 42 +``` + +This fetches the diff against the default branch and runs an architect-level review identifying bugs, security issues, and style violations. You can also use `/review ` inside the interactive TUI. + +**Note:** `nanocoder review` requires an interactive terminal (TTY). It cannot be used with pipes or redirection (e.g. `nanocoder review main > review.md` will error). Use the `/review` slash command inside the interactive TUI for scripting workflows. + **Non-interactive mode behavior:** - Automatically executes the given prompt diff --git a/source/app/prompts/sections/review.md b/source/app/prompts/sections/review.md new file mode 100644 index 000000000..8c284bebb --- /dev/null +++ b/source/app/prompts/sections/review.md @@ -0,0 +1,35 @@ +You are a senior software engineer performing an architect-level code review. + +Review the provided diff and identify actionable issues. Prioritize findings by severity. + +## What to Look For + +- **Correctness bugs**: logic errors, wrong assumptions, incorrect behavior +- **Edge cases**: missing null checks, boundary conditions, race conditions +- **Security vulnerabilities**: injection, auth issues, data exposure, validation gaps +- **Error handling**: swallowed errors, missing try/catch, poor recovery +- **Performance**: unnecessary allocations, N+1 patterns, blocking operations +- **Type safety**: any casts, missing type guards, unsafe assertions +- **API compatibility**: breaking changes, signature mismatches, deprecations +- **Resource leaks**: unclosed handles, missing cleanup, event listener leaks +- **Maintainability**: excessive complexity, poor naming, duplicated logic + +## What to Avoid + +- Trivial formatting or style preferences +- Personal taste disagreements +- Hypothetical problems not supported by the code +- Hallucinated issues that don't appear in the diff +- Unrelated suggestions + +## Output Format + +For each finding, provide: + +1. **Severity**: Critical / Major / Minor +2. **File and line**: Where the issue occurs +3. **Issue**: What is wrong +4. **Impact**: Why it matters +5. **Fix**: How to resolve it + +If the diff is clean with no significant issues, say so explicitly. Do not invent problems. diff --git a/source/cli.spec.ts b/source/cli.spec.ts index dc7240f86..316c0f6aa 100644 --- a/source/cli.spec.ts +++ b/source/cli.spec.ts @@ -1,4 +1,5 @@ import test from 'ava'; +import {filterCliFlags} from '@/utils/cli-flags'; // Test CLI argument parsing for non-interactive mode // These tests verify that the CLI correctly parses the 'run' command @@ -6,35 +7,15 @@ import test from 'ava'; // Helper function to parse prompt from args (mimics the logic in cli.tsx) function parsePrompt(args: string[]): string | undefined { const runCommandIndex = args.findIndex(arg => arg === 'run'); - if (runCommandIndex !== -1 && args[runCommandIndex + 1]) { - // Filter out known flags after 'run' when constructing the prompt - const promptArgs: string[] = []; - const afterRunArgs = args.slice(runCommandIndex + 1); - for (let i = 0; i < afterRunArgs.length; i++) { - const arg = afterRunArgs[i]; - if (arg === '--vscode') { - continue; // skip this flag - } else if (arg === '--vscode-port') { - i++; // skip this flag and its value - continue; - } else if (arg === '--provider') { - i++; // skip this flag and its value - continue; - } else if (arg === '--model') { - i++; // skip this flag and its value - continue; - } else if (arg === '--context-max') { - i++; // skip this flag and its value - continue; - } else if (arg === '--plain' || arg === '--no-plain') { - continue; // skip this flag - } else { - promptArgs.push(arg); - } - } - return promptArgs.join(' '); + if (runCommandIndex === -1) { + return undefined; + } + const afterRunArgs = args.slice(runCommandIndex + 1); + if (afterRunArgs.length === 0) { + return undefined; } - return undefined; + const positionals = filterCliFlags(afterRunArgs); + return positionals.length > 0 ? positionals.join(' ') : undefined; } test('CLI parsing: detects run command with single word prompt', t => { @@ -236,7 +217,7 @@ function resolvePlainMode(opts: { env: NodeJS.ProcessEnv; }): {plainMode: boolean; vscodeMode: boolean} { const {args, stdoutIsTTY, env} = opts; - const nonInteractiveMode = args.includes('run'); + const nonInteractiveMode = args.findIndex(arg => arg === 'run') !== -1; const vscodeMode = args.includes('--vscode'); const plainRequested = args.includes('--plain'); const noPlainRequested = args.includes('--no-plain'); @@ -435,8 +416,7 @@ function resolveResumeFlags(args: string[]): { mutuallyExclusiveError: boolean; nonInteractiveError: boolean; } { - const runCommandIndex = args.findIndex(arg => arg === 'run'); - const nonInteractiveMode = runCommandIndex !== -1; + const nonInteractiveMode = args.findIndex(arg => arg === 'run') !== -1; const continueRequested = args.includes('--continue') || args.includes('-c'); @@ -556,3 +536,139 @@ test('resume flags: --continue without `run` is not a non-interactive error', t const {nonInteractiveError} = resolveResumeFlags(['--continue']); t.false(nonInteractiveError); }); + +// Run command with flags before 'run' (the blocker fix) +test('CLI parsing: handles flags before run command', t => { + const args = ['--plain', 'run', 'say', 'hi']; + const prompt = parsePrompt(args); + t.is(prompt, 'say hi'); +}); + +test('CLI parsing: handles --provider before run command', t => { + const args = ['--provider', 'ollama', 'run', 'analyze', 'code']; + const prompt = parsePrompt(args); + t.is(prompt, 'analyze code'); +}); + +test('CLI parsing: handles --mode before run command', t => { + const args = ['--mode', 'plan', 'run', 'audit', 'module']; + const prompt = parsePrompt(args); + t.is(prompt, 'audit module'); +}); + +// Review guard tests — mirrors the guards in cli.tsx +function resolveReviewGuards(opts: { + args: string[]; + stdoutIsTTY: boolean; + outputFormat: string; +}): {ttyError: boolean; jsonError: boolean; collisionError: boolean} { + const {args, stdoutIsTTY, outputFormat} = opts; + const isRunCommand = args.findIndex(arg => arg === 'run') !== -1; + const isReviewCommand = args[0] === 'review'; + const ttyError = isReviewCommand && !stdoutIsTTY; + const jsonError = isReviewCommand && outputFormat === 'json'; + const collisionError = isRunCommand && isReviewCommand; + return {ttyError, jsonError, collisionError}; +} + +test('review guard: errors when stdout is not a TTY', t => { + const {ttyError} = resolveReviewGuards({ + args: ['review', 'main'], + stdoutIsTTY: false, + outputFormat: 'text', + }); + t.true(ttyError); +}); + +test('review guard: passes on a TTY', t => { + const {ttyError} = resolveReviewGuards({ + args: ['review', 'main'], + stdoutIsTTY: true, + outputFormat: 'text', + }); + t.false(ttyError); +}); + +test('review guard: --json is rejected with review', t => { + const {jsonError} = resolveReviewGuards({ + args: ['review', 'main'], + stdoutIsTTY: true, + outputFormat: 'json', + }); + t.true(jsonError); +}); + +test('review guard: --json is not rejected with run', t => { + const {jsonError} = resolveReviewGuards({ + args: ['run', 'hello'], + stdoutIsTTY: true, + outputFormat: 'json', + }); + t.false(jsonError); +}); + +test('review guard: run and review collision is detected', t => { + const {collisionError} = resolveReviewGuards({ + args: ['review', 'run'], + stdoutIsTTY: true, + outputFormat: 'text', + }); + t.true(collisionError); +}); + +test('review guard: review alone has no collision', t => { + const {collisionError} = resolveReviewGuards({ + args: ['review', 'main'], + stdoutIsTTY: true, + outputFormat: 'text', + }); + t.false(collisionError); +}); + +test('review guard: run alone has no collision', t => { + const {collisionError} = resolveReviewGuards({ + args: ['run', 'hello'], + stdoutIsTTY: true, + outputFormat: 'text', + }); + t.false(collisionError); +}); + +// filterCliFlags: shared flag filter produces single source of truth +test('filterCliFlags: filters all known flags', t => { + const result = filterCliFlags([ + '--vscode', + '--json', + '--trust-directory', + '--plain', + '--no-plain', + '--no-alt-screen', + '--alt-screen', + '--vscode-port', + '3000', + '--provider', + 'ollama', + '--model', + 'llama3', + '--context-max', + '128k', + '--output-format', + 'json', + '--output-format=json', + '--mode', + 'plan', + '--mode=plan', + 'my-prompt', + ]); + t.deepEqual(result, ['my-prompt']); +}); + +test('filterCliFlags: returns all args when no flags present', t => { + const result = filterCliFlags(['hello', 'world']); + t.deepEqual(result, ['hello', 'world']); +}); + +test('filterCliFlags: returns empty array for empty input', t => { + const result = filterCliFlags([]); + t.deepEqual(result, []); +}); diff --git a/source/cli.tsx b/source/cli.tsx index 05b3e10aa..2c90ebd84 100644 --- a/source/cli.tsx +++ b/source/cli.tsx @@ -147,6 +147,7 @@ Commands: init [options] Analyze the project and create AGENTS.md. Use --preset for bundled defaults. copilot login [provider-name] Log in to GitHub Copilot (device flow). Saves credentials for the "GitHub Copilot" provider. + review Review a branch or PR diff for bugs, security issues, and style violations. daemon Manage the per-project skill daemon. Subcommands: start, stop, status, logs, install, uninstall. config Inspect the resolved configuration and where each value came from. @@ -193,6 +194,9 @@ Examples: nanocoder --trust-directory run "analyze src/app.ts" nanocoder --plain run "summarize README.md" nanocoder --plain --json run "summarize README.md" | jq .finalText + nanocoder review main + nanocoder review feature/auth + nanocoder review 42 nanocoder --continue nanocoder --resume last nanocoder --resume @@ -330,53 +334,41 @@ async function main(): Promise { // Check for non-interactive mode (run command) let nonInteractivePrompt: string | undefined; const runCommandIndex = args.findIndex(arg => arg === 'run'); - const afterRunArgs = - runCommandIndex !== -1 ? args.slice(runCommandIndex + 1) : []; - if (runCommandIndex !== -1 && args[runCommandIndex + 1]) { - // Filter out known flags after 'run' when constructing the prompt - const promptArgs: string[] = []; - for (let i = 0; i < afterRunArgs.length; i++) { - const arg = afterRunArgs[i]; - if (arg === '--vscode') { - continue; // skip this flag - } else if (arg === '--vscode-port') { - i++; // skip this flag and its value - continue; - } else if (arg === '--provider') { - i++; // skip this flag and its value - continue; - } else if (arg === '--model') { - i++; // skip this flag and its value - continue; - } else if (arg === '--context-max') { - i++; // skip this flag and its value - continue; - } else if (arg === '--mode') { - i++; // skip this flag and its value - continue; - } else if (arg.startsWith('--mode=')) { - continue; // skip fused form - } else if (arg === '--json') { - continue; // skip this flag - } else if (arg === '--output-format') { - i++; // skip this flag and its value - continue; - } else if (arg.startsWith('--output-format=')) { - continue; // skip fused form - } else if (arg === '--trust-directory') { - continue; // skip this flag - } else if (arg === '--plain' || arg === '--no-plain') { - continue; // skip this flag - } else if (arg === '--no-alt-screen' || arg === '--alt-screen') { - continue; // skip this flag - } else { - promptArgs.push(arg); - } + const isRunCommand = runCommandIndex !== -1; + const afterRunArgs = isRunCommand ? args.slice(runCommandIndex + 1) : []; + if (isRunCommand && afterRunArgs.length > 0) { + const {filterCliFlags} = await import('@/utils/cli-flags'); + nonInteractivePrompt = filterCliFlags(afterRunArgs).join(' '); + } + + let nonInteractiveMode = isRunCommand; + + // Check for `nanocoder review ` — syntactic sugar for + // `nanocoder run /review `. The target is the branch or PR number + // to review. Flags between `review` and the target are filtered the same + // way as `run`. Lazy-loaded to keep it off the lightweight path. + let isReviewCommand = false; + let reviewPrompt: string | undefined; + if (args[0] === 'review') { + const {parseReviewCliArgs} = await import('./commands/review-cli'); + const result = parseReviewCliArgs(args); + isReviewCommand = result.isReviewCommand; + reviewPrompt = result.prompt; + if (result.error) { + console.error(`Error: ${result.error}`); + process.exit(1); } - nonInteractivePrompt = promptArgs.join(' '); } - const nonInteractiveMode = runCommandIndex !== -1; + if (isRunCommand && isReviewCommand) { + console.error('Cannot use both `run` and `review` in the same invocation.'); + process.exit(1); + } + + if (isReviewCommand) { + nonInteractivePrompt = reviewPrompt; + nonInteractiveMode = true; + } // --continue/-c and --resume/-r: session resume flags for the interactive // TUI only (mirrors Claude Code's -c/-r). Mutually exclusive. @@ -433,7 +425,7 @@ async function main(): Promise { console.error('Cannot pass both --plain and --no-plain.'); process.exit(1); } - if (plainRequested && !nonInteractiveMode) { + if (plainRequested && !isRunCommand) { console.error( '--plain requires the `run` subcommand in this version. Try: nanocoder --plain run "..."', ); @@ -450,6 +442,13 @@ async function main(): Promise { process.exit(1); } + if (outputFormat === 'json' && isReviewCommand) { + console.error( + 'Error: --json cannot be used with `nanocoder review`. Review output is displayed in the interactive terminal.', + ); + process.exit(1); + } + const ciDetected = process.env.CI === 'true' || Boolean( @@ -460,12 +459,22 @@ async function main(): Promise { process.env.JENKINS_URL, ); const plainAuto = - nonInteractiveMode && + isRunCommand && !noPlainRequested && !vscodeMode && (!process.stdout.isTTY || ciDetected); const plainMode = plainRequested || plainAuto; + // Hard-error when `review` lands in a non-interactive context (piped + // stdout, CI). The plain shell has no slash-command dispatch, so + // `/review ` would be sent verbatim to the model as chat. + if (isReviewCommand && !process.stdout.isTTY) { + console.error( + 'Error: `nanocoder review` requires an interactive terminal (TTY).', + ); + process.exit(1); + } + // --acp: Agent Client Protocol server mode for editor integration const acpMode = args.includes('--acp'); diff --git a/source/commands/lazy-registry.ts b/source/commands/lazy-registry.ts index 9fabd5258..762c6491e 100644 --- a/source/commands/lazy-registry.ts +++ b/source/commands/lazy-registry.ts @@ -74,6 +74,13 @@ export const lazyCommands: LazyCommand[] = [ progressLabel: 'Generating commit message', load: () => import('@/commands/commit').then(m => m.commitCommand), }, + { + name: 'review', + description: + 'Review a branch or PR diff for bugs, security issues, and style violations', + progressLabel: 'Reviewing code', + load: () => import('@/commands/review').then(m => m.reviewCommand), + }, { name: 'doctor', description: 'Show environment health report for bug reports', diff --git a/source/commands/review-cli.spec.ts b/source/commands/review-cli.spec.ts new file mode 100644 index 000000000..9b8836745 --- /dev/null +++ b/source/commands/review-cli.spec.ts @@ -0,0 +1,194 @@ +import test from 'ava'; +import {parseReviewCliArgs} from './review-cli'; + +test('parseReviewCliArgs: returns false for non-review commands', t => { + t.deepEqual(parseReviewCliArgs(['run', 'hello']), { + isReviewCommand: false, + prompt: undefined, + error: undefined, + }); + t.deepEqual(parseReviewCliArgs([]), { + isReviewCommand: false, + prompt: undefined, + error: undefined, + }); +}); + +test('parseReviewCliArgs: anchors on args[0]', t => { + t.deepEqual(parseReviewCliArgs(['--vscode', 'review', 'main']), { + isReviewCommand: false, + prompt: undefined, + error: undefined, + }); +}); + +test('parseReviewCliArgs: no args produces /review', t => { + t.deepEqual(parseReviewCliArgs(['review']), { + isReviewCommand: true, + prompt: '/review', + error: undefined, + }); +}); + +test('parseReviewCliArgs: branch name', t => { + t.deepEqual(parseReviewCliArgs(['review', 'feature/auth']), { + isReviewCommand: true, + prompt: '/review feature/auth', + error: undefined, + }); +}); + +test('parseReviewCliArgs: PR number', t => { + t.deepEqual(parseReviewCliArgs(['review', '42']), { + isReviewCommand: true, + prompt: '/review 42', + error: undefined, + }); +}); + +test('parseReviewCliArgs: errors on extra positional args', t => { + const result = parseReviewCliArgs(['review', 'feature', 'extra', 'args']); + t.is(result.isReviewCommand, true); + t.is(result.prompt, undefined); + t.truthy(result.error); + t.true(result.error!.includes('Extra arguments')); + t.true(result.error!.includes('extra')); +}); + +test('parseReviewCliArgs: errors on exactly two positional args', t => { + const result = parseReviewCliArgs(['review', 'main', 'other']); + t.is(result.isReviewCommand, true); + t.is(result.prompt, undefined); + t.truthy(result.error); + t.true(result.error!.includes('other')); +}); + +test('parseReviewCliArgs: filters --vscode flag', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--vscode']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --provider flag and value', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--provider', 'openrouter']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --model flag and value', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--model', 'gpt-4']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --mode two-token', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--mode', 'plan']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --mode fused form', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--mode=plan']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --json flag', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--json']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --output-format flag and value', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--output-format', 'json']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --output-format fused form', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--output-format=json']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --context-max flag and value', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--context-max', '128k']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --vscode-port flag and value', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--vscode-port', '3000']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --plain and --no-plain', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--plain']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); + t.deepEqual(parseReviewCliArgs(['review', 'main', '--no-plain']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --no-alt-screen and --alt-screen', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--no-alt-screen']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); + t.deepEqual(parseReviewCliArgs(['review', 'main', '--alt-screen']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: filters --trust-directory flag', t => { + t.deepEqual(parseReviewCliArgs(['review', 'main', '--trust-directory']), { + isReviewCommand: true, + prompt: '/review main', + error: undefined, + }); +}); + +test('parseReviewCliArgs: handles multiple mixed flags', t => { + t.deepEqual(parseReviewCliArgs(['review', 'feature', '--provider', 'ollama', '--mode', 'plan', '--json']), { + isReviewCommand: true, + prompt: '/review feature', + error: undefined, + }); +}); + +test('parseReviewCliArgs: flags between target and extra positionals still error', t => { + const result = parseReviewCliArgs(['review', 'main', '--provider', 'ollama', 'extra']); + t.is(result.isReviewCommand, true); + t.is(result.prompt, undefined); + t.truthy(result.error); + t.true(result.error!.includes('extra')); +}); diff --git a/source/commands/review-cli.ts b/source/commands/review-cli.ts new file mode 100644 index 000000000..780ab573c --- /dev/null +++ b/source/commands/review-cli.ts @@ -0,0 +1,43 @@ +/** + * Parse CLI args for the `nanocoder review` subcommand. + * + * Extracted from cli.tsx so the logic is testable without importing the + * full entry-point module (which c8/ava cannot instrument). + */ + +import {filterCliFlags} from '@/utils/cli-flags'; + +export type ReviewCliResult = { + isReviewCommand: boolean; + prompt: string | undefined; + error: string | undefined; +}; + +export function parseReviewCliArgs(args: string[]): ReviewCliResult { + const isReviewCommand = args[0] === 'review'; + if (!isReviewCommand) { + return {isReviewCommand: false, prompt: undefined, error: undefined}; + } + + const afterReviewArgs = args.slice(1); + const positionals = filterCliFlags(afterReviewArgs); + + if (positionals.length === 0) { + return {isReviewCommand: true, prompt: '/review', error: undefined}; + } + + if (positionals.length > 1) { + const extra = positionals.slice(1).join(', '); + return { + isReviewCommand: true, + prompt: undefined, + error: `Review accepts only one target (branch name or PR number). Extra arguments: ${extra}`, + }; + } + + return { + isReviewCommand: true, + prompt: `/review ${positionals[0]}`, + error: undefined, + }; +} diff --git a/source/commands/review.spec.tsx b/source/commands/review.spec.tsx new file mode 100644 index 000000000..c4a354e0c --- /dev/null +++ b/source/commands/review.spec.tsx @@ -0,0 +1,536 @@ +import test from 'ava'; +import React from 'react'; +import {renderWithTheme} from '@/test-utils/render-with-theme'; +import type {Message} from '@/types/core'; +import {createReviewCommand} from './review'; + +const baseMessages: Message[] = [ + {role: 'user', content: '/review feature'}, +]; + +const testMetadata = { + provider: 'test-provider', + model: 'test-model', + tokens: 0, + getMessageTokens: (m: Message) => m.content.length, +}; + +function createClient(response: string) { + return { + chat: async () => ({ + choices: [ + { + message: { + content: response, + }, + }, + ], + }), + }; +} + +test('reviewCommand has correct name and description', t => { + const command = createReviewCommand({ + execGit: async () => '', + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + t.is(command.name, 'review'); + t.regex( + command.description, + /Review a branch or PR diff for bugs, security issues, and style violations/, + ); +}); + +test('review with no args reviews current branch (no usage error)', async t => { + let diffArgs: string[] = []; + + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + diffArgs = args; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const result = await command.handler([], baseMessages, { + ...testMetadata, + client: createClient('Looks good.'), + }); + + t.truthy(React.isValidElement(result)); + t.deepEqual(diffArgs, [ + 'diff', + '--no-ext-diff', + '--no-color', + 'main...feature', + ]); +}); + +test('review returns an error when no client is available', async t => { + const command = createReviewCommand({ + execGit: async () => '', + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const result = await command.handler(['feature'], baseMessages, { + ...testMetadata, + client: undefined, + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('No active LLM client available')); +}); + +test('review generates a review from the branch diff', async t => { + let receivedMessages: Message[] = []; + + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const client = { + chat: async (messages: Message[]) => { + receivedMessages = messages; + return { + choices: [ + { + message: { + content: + '## Review\n\n**Critical**: Potential null reference at line 5.', + }, + }, + ], + }; + }, + }; + + const result = await command.handler(['feature'], baseMessages, { + ...testMetadata, + client, + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('Potential null reference')); + t.is(receivedMessages[0]?.role, 'system'); + t.is(receivedMessages[1]?.role, 'user'); + t.true( + (receivedMessages[1]?.content as string).includes( + 'branch "feature" against "main"', + ), + ); +}); + +test('review warns when diff is empty', async t => { + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + return ''; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const result = await command.handler(['feature'], baseMessages, { + ...testMetadata, + client: createClient('should not be called'), + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('No changes found')); +}); + +test('review warns when the model returns an empty response', async t => { + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const result = await command.handler(['feature'], baseMessages, { + ...testMetadata, + client: createClient(''), + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('Model returned an empty review')); +}); + +test('review returns an error when the LLM request fails', async t => { + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const client = { + chat: async () => { + throw new Error('LLM request failed'); + }, + }; + + const result = await command.handler(['feature'], baseMessages, { + ...testMetadata, + client, + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('LLM request failed')); +}); + +test('review returns an error when git fails', async t => { + const command = createReviewCommand({ + execGit: async () => { + throw new Error('not a git repository'); + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const result = await command.handler(['feature'], baseMessages, { + ...testMetadata, + client: createClient('should not be called'), + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('not a git repository')); +}); + +test('review uses the review system prompt', async t => { + let systemPrompt = ''; + + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const client = { + chat: async (messages: Message[]) => { + systemPrompt = (messages[0]?.content as string) || ''; + return { + choices: [ + { + message: { + content: 'No issues found.', + }, + }, + ], + }; + }, + }; + + await command.handler(['feature'], baseMessages, { + ...testMetadata, + client, + }); + + t.true(systemPrompt.includes('architect-level code review')); + t.true(systemPrompt.includes('Correctness bugs')); + t.true(systemPrompt.includes('Security vulnerabilities')); +}); + +test('review handles PR number target with gh available', async t => { + let executedGhArgs: string[][] = []; + + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + if (args[0] === 'remote') return 'git@github.com:user/repo.git'; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + isGhAvailable: () => true, + execGh: async args => { + executedGhArgs.push(args); + return 'diff --git a/pr-file.ts b/pr-file.ts\n+const y = 2;'; + }, + }); + + const result = await command.handler(['42'], baseMessages, { + ...testMetadata, + client: createClient('PR review looks good.'), + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('PR review looks good.')); + t.deepEqual(executedGhArgs, [['pr', 'diff', '42', '--repo', 'user/repo']]); +}); + +test('review returns error for PR number when gh is unavailable', async t => { + const command = createReviewCommand({ + execGit: async () => '', + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + isGhAvailable: () => false, + }); + + const result = await command.handler(['42'], baseMessages, { + ...testMetadata, + client: createClient('should not be called'), + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('PR review requires the gh CLI')); +}); + +test('review returns error for PR number when gh fails', async t => { + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'remote') return 'git@github.com:user/repo.git'; + return ''; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + isGhAvailable: () => true, + execGh: async () => { + throw new Error('not authenticated'); + }, + }); + + const result = await command.handler(['42'], baseMessages, { + ...testMetadata, + client: createClient('should not be called'), + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('Failed to fetch PR #42 diff')); + t.true(output.includes('not authenticated')); +}); + +test('review returns error for PR number with non-GitHub remote', async t => { + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'remote') return 'git@gitlab.com:user/repo.git'; + return ''; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + isGhAvailable: () => true, + execGh: async () => 'diff from gh', + }); + + const result = await command.handler(['42'], baseMessages, { + ...testMetadata, + client: createClient('should not be called'), + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('Cannot determine GitHub repository slug')); +}); + +test('review rejects target starting with dash', async t => { + const command = createReviewCommand({ + execGit: async () => '', + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const result = await command.handler(['--ext-diff'], baseMessages, { + ...testMetadata, + client: createClient('should not be called'), + }); + + t.truthy(React.isValidElement(result)); + + const {lastFrame} = renderWithTheme(result as React.ReactElement); + const output = lastFrame() || ''; + + t.true(output.includes('must not start with "-"')); +}); + +test('review with no args reviews current branch against default', async t => { + let diffArgs: string[] = []; + + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + diffArgs = args; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const result = await command.handler([], baseMessages, { + ...testMetadata, + client: createClient('Looks good.'), + }); + + t.truthy(React.isValidElement(result)); + t.deepEqual(diffArgs, [ + 'diff', + '--no-ext-diff', + '--no-color', + 'main...feature', + ]); +}); + +test('review with default branch as target reviews current branch against it', async t => { + let diffArgs: string[] = []; + + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + diffArgs = args; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const result = await command.handler(['main'], baseMessages, { + ...testMetadata, + client: createClient('Looks good.'), + }); + + t.truthy(React.isValidElement(result)); + t.deepEqual(diffArgs, [ + 'diff', + '--no-ext-diff', + '--no-color', + 'main...feature', + ]); +}); + +test('review surfaces truncation info when diff exceeds limit', async t => { + const bigDiff = Array.from({length: 1100}, (_, i) => `+line ${i}`).join( + '\n', + ); + + let userMessage = ''; + + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + return bigDiff; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + }); + + const client = { + chat: async (messages: Message[]) => { + userMessage = (messages[1]?.content as string) || ''; + return { + choices: [ + { + message: { + content: 'Looks good.', + }, + }, + ], + }; + }, + }; + + const result = await command.handler(['feature'], baseMessages, { + ...testMetadata, + client, + }); + + t.truthy(React.isValidElement(result)); + t.true(userMessage.includes('diff truncated')); + t.true(userMessage.includes('first and last 500 of 1100 lines')); +}); + +test('review uses fallback prompt when loadPrompt returns fallback', async t => { + const fallbackPrompt = + 'You are a senior software engineer performing a code review. Review the diff for bugs, security issues, and style violations. Be concise and actionable.'; + + let systemPrompt = ''; + + const client = { + chat: async (messages: Message[]) => { + systemPrompt = (messages[0]?.content as string) || ''; + return { + choices: [ + { + message: { + content: 'Looks good.', + }, + }, + ], + }; + }, + }; + + const command = createReviewCommand({ + execGit: async args => { + if (args[0] === 'rev-parse') return ''; + return 'diff --git a/file.ts b/file.ts\n+const x = 1;'; + }, + getCurrentBranch: async () => 'feature', + getDefaultBranch: async () => 'main', + isGhAvailable: () => false, + execGh: undefined, + loadPrompt: () => fallbackPrompt, + }); + + const result = await command.handler(['feature'], baseMessages, { + ...testMetadata, + client, + }); + + t.truthy(React.isValidElement(result)); + t.is(systemPrompt, fallbackPrompt); +}); diff --git a/source/commands/review.ts b/source/commands/review.ts new file mode 100644 index 000000000..eedd4acae --- /dev/null +++ b/source/commands/review.ts @@ -0,0 +1,213 @@ +import {dirname, join} from 'node:path'; +import {fileURLToPath} from 'node:url'; +import { + execGh, + execGit, + getCurrentBranch, + getDefaultBranch, + isGhAvailable, + truncateDiff, +} from '@/tools/git/utils'; +import type {Command} from '@/types/commands'; +import type {Message} from '@/types/core'; +import {formatError} from '@/utils/error-formatter'; +import {getLogger} from '@/utils/logging'; +import {errorMsg, successMsg, warningMsg} from '@/utils/message-factory'; +import {loadSection} from '@/utils/prompt-builder'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); + +// Maximum number of diff lines to send to the model. truncateDiff keeps +// the first and last half of this budget; keep in sync with the test +// assertion that checks the truncation note. +const REVIEW_MAX_DIFF_LINES = 1000; + +export type ReviewDependencies = { + execGit: (args: string[]) => Promise; + getCurrentBranch: () => Promise; + getDefaultBranch: () => Promise; + isGhAvailable?: () => boolean; + execGh?: (args: string[]) => Promise; + loadPrompt?: () => string; +}; + +const defaultDependencies: ReviewDependencies = { + execGit, + getCurrentBranch, + getDefaultBranch, + isGhAvailable, + execGh, +}; + +function loadReviewPrompt(): string { + const content = loadSection('review'); + if (content) return content; + + const logger = getLogger(); + const promptPath = join( + __dirname, + '../../source/app/prompts/sections/review.md', + ); + logger.warn( + 'Review prompt not found at %s — falling back to built-in default', + promptPath, + ); + return 'You are a senior software engineer performing a code review. Review the diff for bugs, security issues, and style violations. Be concise and actionable.'; +} + +function validateTarget(target: string): string | null { + if (target.startsWith('-')) { + return 'Target must not start with "-". Pass a branch name or PR number.'; + } + return null; +} + +export function createReviewCommand( + dependencies: ReviewDependencies = defaultDependencies, +): Command { + return { + name: 'review', + description: + 'Review a branch or PR diff for bugs, security issues, and style violations', + progressLabel: 'Reviewing code', + handler: async (args, _messages, metadata) => { + const client = metadata.client; + if (!client) { + return errorMsg('No active LLM client available.', 'review'); + } + + try { + const defaultBranch = await dependencies.getDefaultBranch(); + const currentBranch = await dependencies.getCurrentBranch(); + + let diff: string; + let targetDescription: string; + + if (args.length === 0) { + // No target: review current branch against default branch + diff = await getBranchDiff( + dependencies, + currentBranch, + defaultBranch, + ); + targetDescription = `current branch "${currentBranch}" against "${defaultBranch}"`; + } else { + const target = args[0] as string; + + const validationError = validateTarget(target); + if (validationError) { + return errorMsg(validationError, 'review'); + } + + const isPRNumber = /^\d+$/.test(target); + if (isPRNumber) { + const ghAvailable = dependencies.isGhAvailable?.() ?? false; + if (ghAvailable && dependencies.execGh) { + try { + const remote = await dependencies.execGit([ + 'remote', + 'get-url', + 'origin', + ]); + const match = remote.match(/github\.com[:/](.+?)(?:\.git)?$/); + if (!match?.[1]) { + throw new Error( + 'Cannot determine GitHub repository slug from remote URL.', + ); + } + diff = await dependencies.execGh([ + 'pr', + 'diff', + target, + '--repo', + match[1], + ]); + targetDescription = `PR #${target}`; + } catch (error) { + const message = + error instanceof Error ? error.message : String(error); + return errorMsg( + `Failed to fetch PR #${target} diff: ${message}`, + 'review', + ); + } + } else { + return errorMsg( + 'PR review requires the gh CLI. Install it from https://cli.github.com or use a branch name instead.', + 'review', + ); + } + } else { + // If the user passes the default branch name, they want + // to review the current branch against it (not an empty + // diff of main...main). + const branch = target === defaultBranch ? currentBranch : target; + diff = await getBranchDiff(dependencies, branch, defaultBranch); + targetDescription = + target === defaultBranch + ? `current branch "${currentBranch}" against "${defaultBranch}"` + : `branch "${target}" against "${defaultBranch}"`; + } + } + + const truncated = truncateDiff(diff, REVIEW_MAX_DIFF_LINES); + + if (!truncated.content.trim()) { + return warningMsg( + `No changes found in ${targetDescription}.`, + 'review', + ); + } + + const reviewPrompt = dependencies.loadPrompt?.() ?? loadReviewPrompt(); + + const parts: string[] = [ + `Reviewing changes from ${targetDescription}:\n`, + ]; + if (truncated.truncated) { + const halfLines = Math.ceil(REVIEW_MAX_DIFF_LINES / 2); + parts.push( + `[Note: diff truncated — reviewed first and last ${halfLines} of ${truncated.totalLines} lines]\n`, + ); + } + parts.push(truncated.content); + + const messages: Message[] = [ + {role: 'system', content: reviewPrompt}, + {role: 'user', content: parts.join('\n')}, + ]; + + const response = await client.chat(messages, {}, {}); + const review = response?.choices?.[0]?.message?.content?.trim(); + + if (!review) { + return warningMsg('Model returned an empty review.', 'review'); + } + + return successMsg(review, 'review'); + } catch (error) { + return errorMsg(formatError(error), 'review'); + } + }, + }; +} + +async function getBranchDiff( + dependencies: ReviewDependencies, + branch: string, + defaultBranch: string, +): Promise { + await dependencies.execGit(['rev-parse', '--verify', branch]); + + // defaultBranch...branch shows changes on `branch` since it diverged + // from defaultBranch — exactly what a reviewer wants to see. + return dependencies.execGit([ + 'diff', + '--no-ext-diff', + '--no-color', + `${defaultBranch}...${branch}`, + ]); +} + +export const reviewCommand = createReviewCommand(); diff --git a/source/utils/cli-flags.ts b/source/utils/cli-flags.ts new file mode 100644 index 000000000..84d7520e8 --- /dev/null +++ b/source/utils/cli-flags.ts @@ -0,0 +1,42 @@ +/** + * Filter known CLI flags from an argument list, returning only positional + * arguments. Shared by the `run` prompt extraction in cli.tsx and the + * `review` CLI arg parsing in review-cli.ts to keep the flag set in one + * place and prevent regressions like the one caught in review round 2. + */ +export function filterCliFlags(args: string[]): string[] { + const positionals: string[] = []; + for (let i = 0; i < args.length; i++) { + const arg = args[i]; + if ( + arg === '--vscode' || + arg === '--json' || + arg === '--trust-directory' || + arg === '--plain' || + arg === '--no-plain' || + arg === '--no-alt-screen' || + arg === '--alt-screen' + ) { + continue; + } else if ( + arg === '--vscode-port' || + arg === '--provider' || + arg === '--model' || + arg === '--context-max' || + arg === '--output-format' + ) { + i++; // skip this flag and its value + continue; + } else if (arg === '--mode') { + i++; // skip this flag and its value + continue; + } else if (arg.startsWith('--mode=')) { + continue; // skip fused form + } else if (arg.startsWith('--output-format=')) { + continue; // skip fused form + } else { + positionals.push(arg); + } + } + return positionals; +} diff --git a/source/utils/prompt-builder.ts b/source/utils/prompt-builder.ts index 99e7c441e..ee2223ef6 100644 --- a/source/utils/prompt-builder.ts +++ b/source/utils/prompt-builder.ts @@ -23,7 +23,7 @@ function getSectionFilePath(name: string): string { return join(sectionsDir, `${safeName}.md`); } -function loadSection(name: string): string { +export function loadSection(name: string): string { const cached = sectionCache.get(name); if (cached !== undefined) return cached;