Skip to content

feat: add /review slash command and nanocoder review CLI command - #1099

Open
soumojit-D48 wants to merge 6 commits into
Nano-Collective:mainfrom
soumojit-D48:feat/review-command
Open

feat: add /review slash command and nanocoder review CLI command#1099
soumojit-D48 wants to merge 6 commits into
Nano-Collective:mainfrom
soumojit-D48:feat/review-command

Conversation

@soumojit-D48

@soumojit-D48 soumojit-D48 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Description

Add a dedicated /review slash command and nanocoder review <branch|pr-number> CLI command for AI-powered code review. This implements the feature requested in issue #1002.

Code review is a massive use case for AI. Previously, users had to manually prompt "review the git diff" which produced inconsistent results without a strong system prompt. This PR adds a first-class review command that provides architect-level analysis of branch and PR diffs.

Usage

Slash command (interactive TUI):

/review main
/review feature/auth
/review 42

CLI command (non-interactive):

nanocoder review main
nanocoder review feature/auth
nanocoder review 42

How it works

  1. Resolves the target (branch name or PR number)
  2. Fetches the diff against the default branch using existing git tools
  3. For PR numbers, uses gh pr diff when the GitHub CLI is available
  4. Feeds the diff to a dedicated review system prompt
  5. Returns an architect-level review identifying bugs, security issues, and style violations

What the review identifies

  • Correctness bugs and logic errors
  • Edge cases and missing null checks
  • Security vulnerabilities and auth issues
  • Error handling problems
  • Performance issues and resource leaks
  • Type safety concerns
  • API compatibility problems
  • Maintainability issues

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Docs-only or internal chores need no changeset (or run pnpm changeset --empty to note that intentionally).

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

9 test cases covering:

  • Missing arguments (shows usage message)
  • Missing LLM client (shows error)
  • Successful review generation
  • Empty diff handling (shows warning)
  • Empty LLM response (shows warning)
  • LLM failure (shows error)
  • Git failure (shows error)
  • Review system prompt validation

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)
image

Architecture Decisions

  • Dependency injection: The command uses ReviewDependencies interface for testability, following the same pattern as /commit
  • Lazy loading: Registered in lazy-registry.ts to avoid loading at startup
  • PR support: Uses gh pr diff when available, falls back to branch diff when gh CLI is missing
  • Diff truncation: Limits diff to 1000 lines to stay within LLM context windows
  • Error handling: Graceful fallback for git errors, LLM failures, and empty diffs

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

@soumojit-D48

Copy link
Copy Markdown
Contributor Author

@will-lamerton @akramcodez @Avtrkrb, Hi Guys Kindly Review this PR and let me know, Thanku..

@will-lamerton will-lamerton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice shape overall: it follows the /commit dependency-injection pattern, registers lazily with progressLabel on both the command and the registry entry, reuses the existing git utils, and updates README/docs/help together. I checked the branch out locally: tsc --noEmit passes, biome is clean, all 13 tests pass.

Three things need fixing before this can land.

1. nanocoder review is a no-op whenever stdout is not a TTY (pipes, redirects, CI).

cli.tsx sets nonInteractivePrompt = '/review main', but plainAuto enables plain mode whenever !process.stdout.isTTY || ciDetected, and runPlainShell puts the prompt straight into the conversation as {role: 'user', content: prompt} (source/plain/shell.ts:180). There is no slash-command dispatch anywhere in source/plain/. So nanocoder review 42 > out.md or any CI run just sends the literal text /review 42 to the model as chat. The Ink path works only because handleMessageSubmit dispatches commands. Options: teach the plain shell to dispatch built-in commands, call the review logic directly from cli.tsx, or hard-error when review lands in plain mode.

2. The review system prompt never loads in an installed build, so it silently degrades to the one-line fallback.

loadReviewPrompt() resolves join(__dirname, '../app/prompts/sections/review.md'). After tsc, __dirname is dist/commands, so it looks for dist/app/prompts/sections/review.md. tsc does not emit .md and the build script copies only contributors.json, so that path never exists in a built tree. The npm files list ships source/app/prompts/sections, which is exactly why prompt-builder.ts:15 uses ../../source/app/prompts/sections. Net effect: every real install hits the catch {} and gets the one-sentence fallback prompt, losing the point of the PR, with nothing logged. The test passes only because AVA runs from source/. Please match the prompt-builder.ts path (or export a loadPromptSection(name) helper there and reuse it, including its basename traversal guard), log in the catch, and add a test asserting the file resolved rather than the fallback.

3. Target semantics are inverted relative to the documented examples.

getBranchDiff always runs git diff <defaultBranch>...<target>, so the headline example /review main computes git diff main...main and always reports "No changes found". The docs say "fetches the diff against the default branch", which is what a user reviewing their own branch expects. Suggest defaulting the target to the current branch and diffing <target>...HEAD, or at minimum special-casing target === defaultBranch. The empty-diff message is also wrong: it reports between "${currentBranch}" and "${targetDescription}" when currentBranch never appears in the diff command.

Medium

  • args.findIndex(arg => arg === 'review') matches anywhere in argv and unconditionally overwrites nonInteractivePrompt, so nanocoder run please review this file (unquoted prompts are supported) silently becomes /review this file. Anchor on args[0] like copilot login, and error when run and review are both present.
  • The copied flag filter handles --mode=x but not the two-token --mode plan that the run loop skips, so nanocoder review --mode plan main sets the target to --mode. Good case for extracting the run filter into one shared helper instead of duplicating it.
  • When gh is missing or fails, a numeric target falls back to being treated as a branch, git rev-parse --verify 42 fails, and the user sees a raw git error. The bare catch {} also discards why gh failed (not authenticated, PR not found, wrong repo). An explicit "PR review requires the gh CLI" plus the surfaced gh error would be much clearer.

Minor

  • truncateDiff(diff, 1000) keeps the first and last 500 lines and drops the middle, which is the worst part to lose for a review. truncated.truncated is computed but never used: surface "reviewed first/last N of M lines" so the user knows the review is partial.
  • target is not validated before going into git argv. No shell is involved so there is no shell injection, but a leading - is argument injection (/review --ext-diff). Reject targets starting with -, or pass -- before the ref.
  • On the PR path the user message says Reviewing changes from PR #42 into "feature", which is inaccurate; the PR diff has no relation to the current branch.
  • ReviewDependencies declares isGhAvailable/execGh as required, but 8 of the 13 tests construct it without them. That only compiles because tsconfig excludes *.spec.* from tsc --noEmit and biome excludes specs from lint. Make them optional, or accept Partial<ReviewDependencies> merged over defaultDependencies.
  • No tests for the new cli.tsx parsing, even though source/cli.spec.ts already has an established pattern for it. All three CLI-level bugs above would be caught there.
  • No changeset. Please add one naming @nanocollective/nanocoder (a bare nanocoder passes PR checks and then breaks release-prepare on main).
  • Docs say PR review "requires gh CLI", but the code silently falls back instead.

Design note

The command is a single one-shot client.chat over a diff, with no file reads or tool loop. That matches /commit, but it caps review quality: the model cannot open surrounding code to check whether a finding is real, which is what the prompt's "no hallucinated issues" instruction actually needs. Worth confirming with #1002 whether diff-only review is the intended scope or a first step.

@github-actions github-actions Bot added area:tui Terminal UI area:docs Documentation area:vscode VS Code extension and host integration labels Aug 31, 2026
@soumojit-D48

Copy link
Copy Markdown
Contributor Author

@will-lamerton hi pls review the PR again. thanku..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:docs Documentation area:tui Terminal UI area:vscode VS Code extension and host integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants