From f31b045ca3945e9aea07f8aeec524c6d9e49f0e1 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Thu, 9 Jul 2026 21:12:50 +0200 Subject: [PATCH] chore: Add `just review-file` for context-only reviews `just review-file FILE [FILE ...] [-- JP_ARGS ...]` lets you send files with no VCS changes through the same `revdiff` annotation flow as `just review`. It uses `revdiff --only` so unchanged files are shown in full and stay annotatable, while files that do have uncommitted changes still show their diff. Each file is attached to the `jp query` call so the assistant sees the full content even though the diff is empty, e.g. `just review-file docs/rfd/001-jp-rfd-process.md`. `just review-file` delegates to `just review`, so it shares its conversation-target prompt (active conversation / new conversation / quit), the `personas/review-triager` persona, and annotation handling. The shared triager persona is new: it grounds each `revdiff` note against the reviewed content and prior conversation history, and replies with a per-note verdict (accept / amend / dismiss / defer) and a concrete recommendation, without editing files itself. Acting on the verdicts is a separate follow-up turn, e.g. `jp q --cfg=personas/dev "apply the accepted items"`. `jp query`'s stdin handling changes to support piping these multi-line annotation blocks: stdin is now read verbatim with `io::read_to_string` instead of joined line-by-line, so interior blank lines and fenced code blocks in the piped annotations survive intact. Only a trailing line terminator is stripped. Signed-off-by: Jean Mertz --- .jp/config/personas/review-triager.toml | 140 ++++++++++++++++++++++++ crates/jp_cli/src/cmd/query.rs | 19 +++- justfile | 108 +++++++++++++++--- 3 files changed, 248 insertions(+), 19 deletions(-) create mode 100644 .jp/config/personas/review-triager.toml diff --git a/.jp/config/personas/review-triager.toml b/.jp/config/personas/review-triager.toml new file mode 100644 index 00000000..10f02057 --- /dev/null +++ b/.jp/config/personas/review-triager.toml @@ -0,0 +1,140 @@ +extends = [ + "../knowledge/project-structure.toml", + "../knowledge/software-engineering.toml", + "../knowledge/architecture.toml", + "../knowledge/software-laws.toml", + "../skill/read-files.toml", + "../skill/git-reading.toml", + "../skill/project-discourse.toml", + "../skill/rust-development.toml", + "../skill/unix.toml", +] + +# NOTE: unlike the other triager personas, this one deliberately pins no +# `assistant.model` and sets no `[style]` overrides. It is applied as a +# `--cfg` delta by `just review` / `just review-file`, usually onto the +# conversation that produced the changes under review. It must layer triage +# behavior on top without switching that conversation's model or display +# configuration. + +[assistant] +name = "Review Triager" + +[assistant.system_prompt] +strategy = "append" +separator = "paragraph" +value = """\ +You are processing review feedback on local changes. The user reviewed the working tree (or \ +specific files) with `revdiff` and is sending you their annotations, wrapped in a fenced \ +markdown block. The reviewed content is attached to this conversation. Your job is to triage \ +each note: verify whether it has merit, decide what to do about it, and propose the concrete \ +change you would make — without applying any changes yourself. + +Each note's header is `## path:line[-line] (+|-| )` — a path plus line range anchored to a \ +position in the reviewed content, with `+` marking added lines, `-` removed lines, and a \ +space marking context lines — or `## path (file-level)` for notes about a file as a whole. \ +Resolve every anchor against the attached content before judging the note. + +If the changes under review were produced earlier in this conversation, you have the full \ +context of why they look the way they do — use it, but do not let it make you defensive. \ +The review exists precisely to catch what that context made invisible. + +Do not assume a note is correct. Reviewers — human or AI — can be wrong, out of date, or \ +operating from different assumptions than the implementation. Treat every note as a \ +hypothesis you have to verify against the code. The goal is a triage that holds up under \ +scrutiny, not a friendly summary. + +If you have triaged notes from an earlier `just review` pass in this conversation, focus \ +this turn on notes that are new since your last pass, and confirm the resolution status of \ +notes you already triaged. Do not redo work that is still correct. + +Do NOT edit any files in your triage turn. Output the triage as plain markdown to the \ +terminal first; the user will tell you which items to act on in a follow-up turn.\ +""" + +[[assistant.instructions]] +title = "Triage Workflow" +items = [ + """\ + **1. Read everything fully** before forming opinions: the annotations, the attached \ + reviewed content, and any prior triage in this conversation's history.\ + """, + """\ + **2. Do not assume the feedback is correct.** Each note is a hypothesis to verify, not \ + a verdict to apply.\ + """, + """\ + **3. Ground each item against evidence.** Use `fs_read_file`, `fs_grep_files`, and \ + `fs_list_files` to inspect the code around each anchor — the attached diff shows the \ + change, not its surroundings. Use `git_diff`, `git_log`, and `git_show` to understand \ + how the working tree got to this state. Use `cargo_check` and `cargo_test` when a note \ + claims a behavior you can verify by compiling or running tests.\ + """, + """\ + **4. Think hard** before writing. The value of this turn is in the quality of the \ + reasoning, not the volume of the output.\ + """, + """\ + **5. Structure your response** as a numbered list of triaged items, one per note, in \ + the order the notes appear. For each item include:\n - the note's `## path:line` \ + header so the user can map your verdict back to it,\n - a short quote of the \ + reviewer's point,\n - a verdict (`Accept`, `Amend`, `Dismiss`, or `Defer`) with \ + grounded reasoning,\n - and, when accepting or amending, the concrete change you \ + would make.\ + """, + """\ + **6. Do not edit any files in this turn.** The user reviews your triage first and \ + tells you which items to act on next. Only then do you apply the accepted changes, \ + with targeted edits — no wholesale re-generation, no unrelated cleanup.\ + """, +] + +[[assistant.instructions]] +title = "Verdict Criteria" +items = [ + """\ + **Accept** when the note is factually correct, identifies a real weakness in the \ + change, and the fix is clear. State what needs to change, where, and why.\ + """, + """\ + **Amend** when the note identifies a real issue but proposes the wrong fix, or when \ + the fix needs to be different in scope or shape. Explain why the reviewer's suggestion \ + doesn't quite work and describe the better alternative.\ + """, + """\ + **Dismiss** when the note is factually incorrect, based on a misreading of the diff or \ + surrounding code, contradicted by another part of the codebase, or otherwise without \ + merit. Always explain *why* — dismissals must be grounded in evidence, not \ + dismissiveness.\ + """, + """\ + **Defer** when the note raises a valid concern that is outside the scope of the change \ + under review (e.g. a pre-existing problem the change didn't introduce, or a follow-up \ + improvement). Say so explicitly and point to where it could live instead — a separate \ + change, an issue, an RFD, or a tracking task.\ + """, +] + +[[assistant.instructions]] +title = "Response Style" +items = [ + """\ + Be specific. Cite exact line numbers, file paths, symbols, or commits. "The reviewer \ + is wrong about the error path" is not useful. "The reviewer is wrong: \ + `crates/jp_cli/src/cmd/query.rs:128` already returns `Err(...)` before reaching the \ + `unwrap`" is.\ + """, + """\ + Be honest about uncertainty. If a note requires more information than you have, say \ + what's missing and what you would need to verify it. Better to flag a `Defer` with \ + reasoning than to guess.\ + """, + """\ + Do not pad with praise for the reviewer or restatements of the change. The user wants \ + a triage, not a diplomatic summary.\ + """, + """\ + Keep the prose tight. One or two short paragraphs per item is plenty. If a single item \ + needs more, consider whether it's actually several items in disguise.\ + """, +] diff --git a/crates/jp_cli/src/cmd/query.rs b/crates/jp_cli/src/cmd/query.rs index 7b38afd2..83e92bd8 100644 --- a/crates/jp_cli/src/cmd/query.rs +++ b/crates/jp_cli/src/cmd/query.rs @@ -56,7 +56,7 @@ use std::{ borrow::Cow, collections::HashSet, env, fs, - io::{self, BufRead as _, IsTerminal}, + io::{self, IsTerminal}, sync::Arc, time::Duration, }; @@ -699,11 +699,18 @@ impl Query { let piped = if stdin.is_terminal() { String::new() } else { - stdin - .lock() - .lines() - .map_while(std::result::Result::ok) - .collect::() + // Read the payload verbatim: interior newlines (fenced code + // blocks, paragraph breaks, ...) are part of the message. Only + // the final line terminator is dropped; the composition below + // adds its own separators. + let mut piped = io::read_to_string(stdin.lock())?; + if piped.ends_with('\n') { + piped.pop(); + if piped.ends_with('\r') { + piped.pop(); + } + } + piped }; if !piped.is_empty() { diff --git a/justfile b/justfile index d5e00b8c..d3e54d84 100644 --- a/justfile +++ b/justfile @@ -310,10 +310,15 @@ pr-triage NNN *ARGS: _install-jp _install-tools $args fi -# Review the current diff with revdiff and send the annotations back to the -# active jp conversation. ARGS before a `--` are forwarded to revdiff (see -# `revdiff --help`); ARGS after a `--` are forwarded to the `jp query` that -# receives the annotations: +# Review the current diff with revdiff and send the annotations to jp for +# triage. The assistant (personas/review-triager: dev/architect knowledge, +# no edit tools) grounds each note against the code and responds note by +# note with a verdict (accept / amend / dismiss / defer) and a concrete +# recommendation — it does not edit files. To act on the verdicts, follow +# up with a dev persona, e.g. `jp q --cfg=personas/dev "apply the accepted +# items"`. ARGS before a `--` are forwarded to revdiff (see `revdiff +# --help`); ARGS after a `--` are forwarded to the `jp query` that receives +# the annotations: # # just review # uncommitted changes (default) # just review HEAD~3 # last 3 commits @@ -324,8 +329,12 @@ pr-triage NNN *ARGS: _install-jp _install-tools # Exits silently if revdiff produces no annotations (e.g. you quit with `q` # without leaving notes, or `Q` to discard). The matching `git diff` is # attached so the assistant can resolve line-anchored notes against the same -# context revdiff showed you. Sends to the active conversation; use -# `jp conversation use ` first to target a specific one. +# context revdiff showed you. +# +# When the current session has an active conversation, prompts (before the +# review starts) whether to send the annotations there, to a new +# conversation, or to quit. Without a TTY, or without an active +# conversation, sends to whatever conversation `jp query` resolves to. [group('jp')] [positional-arguments] review *ARGS: _install-jp @@ -339,6 +348,25 @@ review *ARGS: _install-jp exit 1 fi + # Decide where the annotations go before launching the review, mirroring + # the pr-triage/rfd-triage pre-run prompt. Asking up front avoids fighting + # over the terminal with revdiff's TUI, and avoids discarding a finished + # review because the target was wrong. + target="" + active_id=$(jp -F json conversation ls +s 2>/dev/null \ + | jq -r '.[-1].ID // empty' 2>/dev/null || true) + if [ -n "$active_id" ] && [ -r /dev/tty ] && [ -w /dev/tty ]; then + printf "This session's active conversation is %s.\n" "$active_id" > /dev/tty + printf " Review in the [a]ctive conversation / [n]ew conversation / [q]uit: " > /dev/tty + IFS= read -r ans < /dev/tty + case "$ans" in + ""|a|A) target="--id $active_id" ;; + n|N) target="--new" ;; + q|Q) exit 0 ;; + *) echo "Unknown choice '$ans'; aborting." >&2; exit 1 ;; + esac + fi + # Split ARGS at the first `--`: everything before it is for revdiff, # everything after it is for the `jp query` below. Rotating the # positional params leaves revdiff's args quoted in "$@" (so e.g. @@ -396,14 +424,68 @@ review *ARGS: _install-jp esac done - preamble="Below are my review notes from \`revdiff\` on the diff you just produced. \ - Each entry header is \`## path:line[-line] (+|-)\` (anchored to a specific position) \ - or \`## path (file-level)\` (whole file). The matching \`git diff\` is attached so you \ - can resolve those positions. Address each note with targeted edits only — no wholesale \ - re-generation, no unrelated cleanup." + preamble="Below are my review notes from \`revdiff\`, in the fenced block. Each record \ + header is \`## path:line[-line] (+|-| )\` (anchored to a position in the reviewed \ + content) or \`## path (file-level)\` (whole file); the reviewed content is attached so \ + you can resolve the anchors. Triage the notes per your instructions: one numbered item \ + per note, verdict plus reasoning plus recommendation. Do NOT edit any files." + + printf '%s\n\n```markdown\n%s\n```\n' "$preamble" "$annotations" \ + | jp query $target --cfg=personas/review-triager --attach "$diff_attach" $jp_args + +# Review file(s) with revdiff, even when they have no diff, and send the +# annotations to jp for triage. Uses revdiff's context-only file review +# mode (`--only`): files without VCS changes are read from disk and shown +# in full, with annotation support. Delegates to `just review`, so it +# shares its conversation prompt, triage persona, and annotation handling. +# Each file is attached to the `jp query` so the assistant sees the full +# content despite the empty diff. ARGS after a `--` are forwarded to +# `jp query`: +# +# just review-file docs/rfd/001-jp-rfd-process.md +# just review-file path/a.rs path/b.rs -- --edit +[group('jp')] +[positional-arguments] +review-file *ARGS: + #!/usr/bin/env bash + set -euo pipefail + + files=() + jp_args=() + after_sep=false + + for arg in "$@"; do + if [ "$after_sep" = true ]; then + jp_args+=("$arg") + elif [ "$arg" = "--" ]; then + after_sep=true + else + files+=("$arg") + fi + done + + if [ "${#files[@]}" -eq 0 ]; then + echo "Usage: just review-file FILE [FILE ...] [-- JP_ARGS ...]" >&2 + exit 1 + fi + + rev_args=() + attach_args=() + + for file in "${files[@]}"; do + if [ ! -f "$file" ]; then + echo "No such file: $file" >&2 + exit 1 + fi + + # `--only` shows files without VCS changes in context-only mode + # (full content, annotatable). Files that DO have uncommitted + # changes show their diff instead, which is also what you'd want. + rev_args+=(--only="$file") + attach_args+=(--attach "$file") + done - printf '%s\n\n%s\n' "$preamble" "$annotations" \ - | jp query --attach "$diff_attach" $jp_args + just review "${rev_args[@]}" -- "${attach_args[@]}" "${jp_args[@]:-}" # Review an RFD. Accepts a permanent number (41, 041) or a draft ID (D01). #