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

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.

Thanks for the updates. Most of the previous points are genuinely fixed, but one of the fixes introduced a regression, and the tests that would have caught it were rewritten to match it. Still needs another pass.

Blocker (new): run no longer works with flags before it

My medium point asked to anchor review on args[0]. That was also applied to run (source/cli.tsx:321), and args is raw process.argv.slice(2) with no normalisation. Every documented invocation with a flag before run now breaks:

• nanocoder --plain run "summarize README.md" (README and cli.tsx's own Examples block)
• nanocoder --plain --json run "..." | jq .finalText (README)
• nanocoder --mode plan run "audit the auth module" (README)
• nanocoder --trust-directory run "analyze src/app.ts" (cli.tsx help)
• nanocoder --provider ollama --model llama3.1 --context-max 128k run "analyze src" (docs/getting-started)

Verified on this branch:

$ npx tsx source/cli.tsx --plain run "say hi"
--plain requires the `run` subcommand in this version. Try: nanocoder --plain run "..."

The error tells you to run the command that produced it. On 4143e0ef the same invocation works. --json and --trust-directory before run hit their own guards at cli.tsx:456 and :464 the same way.

Two existing tests that covered this were edited rather than kept, which is why all 88 tests pass:

• CLI parsing: handles mixed arguments with run command: ['--vscode', 'run', ...] changed to ['run', ...]
• plain mode: explicit --plain enables it on a TTY without CI: ['--plain', 'run', 'hi'] changed to ['run', 'hi', '--plain'], title changed to "when run is args[0]"

Please leave run on findIndex, anchor only review on args[0], and restore both tests. Related: the isRunCommand && isReviewCommand guard at cli.tsx:420 is unreachable, since args[0] cannot be both.

Previous blocking points

1. Plain mode - addressed via the hard-error route. One problem: the message suggests nanocoder review <target> 2>&1 | tee out.md, but | tee makes stdout a pipe and trips the same guard. There is currently no way to get review output into a file, so the CLI half of the feature is interactive-only. Worth confirming that is the intended end state.
2. Prompt path - addressed. review.ts:24 matches prompt-builder.ts:15, resolves correctly from both source/commands and dist/commands, the catch logs, and the test asserts real prompt content rather than the fallback. Nits: the shared loadPromptSection helper was not extracted, so the path derivation now exists twice; and basename(normalize('review')...) sanitises a hardcoded literal, so that guard protects against nothing.
3. Target semantics - addressed. No-arg reviews current vs default, target === defaultBranch is special-cased, and the empty-diff message uses targetDescription.

Medium

• Two-token --mode plan is handled, but by copying the filter. That flag list now exists four times: run and review in cli.tsx, parsePrompt and parseReviewArgs in the spec. Extracting it would also have prevented the regression above.
• gh handling is properly fixed, with an explicit message, gh's error surfaced, non-GitHub remotes caught, and tests for all three.

Minor

• Truncation note arithmetic is wrong. review.ts uses Math.ceil(truncated.totalLines / 2), but truncateDiff keeps Math.ceil(maxLines / 2) = 500 head and 500 tail. For an 1100-line diff the note reads "reviewed first and last 550 of 1100 lines", claiming full coverage while 100 lines were dropped, and it contradicts the accurate marker truncateDiff already embeds. The test only asserts of 1100 lines, so it passes. Fix to 500, or drop the note since the marker is already in the diff.
• Leading-- rejection, --no-ext-diff, the PR-path message, optional isGhAvailable/execGh, the changeset naming, and the docs/gh consistency are all done.
• The new cli.tsx tests follow the existing duplicate-the-helper convention, which is fine, but nothing covers the two new CLI behaviours: the non-TTY review guard and the run/review conflict.
• nanocoder review with no target exits 1 with usage, while /review with no args reviews the current branch. Same command, two contracts.
• reviewArgs.join(' ') lets nanocoder review a b become /review a b; the handler reads args[0] and drops b silently.

Design note

Still unanswered from last time. This remains a single one-shot client.chat over a diff with no tool loop, so the prompt's "no hallucinated issues" instruction has nothing to verify findings against. Fine as a first step, but worth confirming against #1002.

Fourth pass. The blocker from last time is genuinely fixed and I verified it. What is left is one medium that was moved rather than fixed, the two test gaps I asked for, three carried-over nits, and two design questions that have now gone unanswered across three reviews. Requesting changes mainly for the last two: I do not want to merge a feature whose scope is still undecided.

Checked out 3deffe9f in an isolated worktree: tsc --noEmit clean, biome clean, 95 tests pass across cli.spec.ts, review.spec.tsx, review-cli.spec.ts.

Blocker: fixed, verified

run is back on findIndex (cli.tsx:323) and only review anchors on args[0], via the extracted parseReviewCliArgs in source/commands/review-cli.ts. plainRequested, plainAuto, and the --json / --trust-directory guards all read isRunCommand rather than nonInteractiveMode, so every documented flag-before-run invocation works again:

$ npx tsx source/cli.tsx --plain run "say hi"
Directory ... is not trusted. Pass --trust-directory ...

Both tests are restored byte-for-byte to their main versions, titles included, and three new cases cover flags before run. The unreachable isRunCommand && isReviewCommand guard is gone. Thanks for taking that one seriously.

Needs another change

1. nanocoder review a b still drops b silently. This was not fixed, it was relocated. parseReviewCliArgs returns reviewArgs[0] and discards the rest, and parseReviewCliArgs: only uses first positional arg now locks that behaviour in as intended. Silently ignoring an argument the user typed is the thing I was asking you to remove. Either error on more than one positional, or accept the extra args and reject them in the handler with a message.

2. No tests for either new CLI behaviour. review-cli.spec.ts covers only parseReviewCliArgs, which is good as far as it goes. Nothing covers the non-TTY review guard or a run / review collision, which are the two behaviours that are new in cli.tsx. cli.spec.ts already has resolvePlainMode as a pattern to copy. I verified the guard by hand instead:

$ npx tsx source/cli.tsx review main < /dev/null | cat
Error: `nanocoder review` requires an interactive terminal (TTY).

3. Docs do not mention that nanocoder review is TTY-only. docs/getting-started/index.md and the README both present it in a bash block next to run, which does support piping and redirection. Anyone who writes nanocoder review main > review.md gets an error the docs gave them no reason to expect. One sentence in the Code Review section covers it.

4. The prompt-fallback test renames a tracked source file. review.spec.tsx:507 does renameSync on source/app/prompts/sections/review.md and restores in a finally. A killed or crashed run leaves review.md.bak in the working tree and the real prompt missing, which then silently degrades every subsequent local review to the fallback. Mock readFileSync, or point the loader at a temp dir.

Carried over, still open

• loadPromptSection was not extracted. The path is now derived in two places, source/utils/prompt-builder.ts:15 and source/commands/review.ts:24, and they have to stay in sync by hand.
• basename(normalize('review').replace(...)) still sanitises a hardcoded string literal. It protects against nothing. Either take a real parameter or drop it.
• The flag list is still triplicated: the run loop in cli.tsx:331, parseReviewCliArgs, and parsePrompt in cli.spec.ts. The three agree today. They also agreed before the last regression.

Smaller things from this pass

• await import('./commands/review-cli') sits at cli.tsx:202, directly beneath the comment saying --acp / --plain / auth must stay on the lightweight path. The module has no imports so the cost is near zero, but it runs on every single invocation for a subcommand that is almost never the one being used. Move it inside an args[0] === 'review' check.
• nanocoder review --json main on a TTY passes the --json guard, because review sets nonInteractiveMode = true, and then --json is silently ignored on the Ink path.
• Truncation arithmetic is correct now, but the 500 in review.ts is hardcoded and only right because the call site passes 1000. Change that number and the note lies again with no test failure. Deriving it from the same expression truncateDiff uses would close it.

Design, third ask

Both of these are still unanswered, and they are the reason this is request-changes rather than approve.

Is diff-only review the intended scope? This is still a single one-shot client.chat over a diff with no tool loop. The system prompt tells the model not to hallucinate issues, but it has no way to open the surrounding code and check whether a finding is real, which is precisely what that instruction requires. That is a defensible v1, but I want it stated as a deliberate first step against #1002 rather than left implicit.

Is TTY-only the intended end state for the CLI half? The hard-error route is a reasonable fix for the plain-shell gap, and the message no longer suggests the self-defeating | tee. But the result is that nanocoder review cannot write its output anywhere, which is most of the reason to have a CLI form at all. If that is the plan for now, say so in the PR and in the docs and I will take it. If the intent is to teach the plain shell to dispatch built-in commands later, an issue linking back here would be enough.

Answer those two and fix 1 through 4 and I think this lands.