feat: add /review slash command and nanocoder review CLI command - #1099
feat: add /review slash command and nanocoder review CLI command#1099soumojit-D48 wants to merge 6 commits into
Conversation
|
@will-lamerton @akramcodez @Avtrkrb, Hi Guys Kindly Review this PR and let me know, Thanku.. |
will-lamerton
left a comment
There was a problem hiding this comment.
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 overwritesnonInteractivePrompt, sonanocoder run please review this file(unquoted prompts are supported) silently becomes/review this file. Anchor onargs[0]likecopilot login, and error whenrunandrevieware both present.- The copied flag filter handles
--mode=xbut not the two-token--mode planthat therunloop skips, sonanocoder review --mode plan mainsets the target to--mode. Good case for extracting therunfilter into one shared helper instead of duplicating it. - When
ghis missing or fails, a numeric target falls back to being treated as a branch,git rev-parse --verify 42fails, and the user sees a raw git error. The barecatch {}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.truncatedis computed but never used: surface "reviewed first/last N of M lines" so the user knows the review is partial.targetis 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. ReviewDependenciesdeclaresisGhAvailable/execGhas required, but 8 of the 13 tests construct it without them. That only compiles because tsconfig excludes*.spec.*fromtsc --noEmitand biome excludes specs from lint. Make them optional, or acceptPartial<ReviewDependencies>merged overdefaultDependencies.- No tests for the new
cli.tsxparsing, even thoughsource/cli.spec.tsalready 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 barenanocoderpasses 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.
51222fa to
412306a
Compare
80e7c29 to
6a95365
Compare
6a95365 to
f08dc1e
Compare
|
@will-lamerton hi pls review the PR again. thanku.. |
Description
Add a dedicated
/reviewslash command andnanocoder 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):
CLI command (non-interactive):
How it works
gh pr diffwhen the GitHub CLI is availableWhat the review identifies
Type of Change
Changeset
pnpm changeset) describing this change for the changelogDocs-only or internal chores need no changeset (or run
pnpm changeset --emptyto note that intentionally).Testing
Automated Tests
.spec.ts/tsxfilespnpm test:allcompletes successfully)9 test cases covering:
Manual Testing
Architecture Decisions
ReviewDependenciesinterface for testability, following the same pattern as/commitlazy-registry.tsto avoid loading at startupgh pr diffwhen available, falls back to branch diff when gh CLI is missingChecklist