feat(review): review targets, merge-base resolution and a rubric - #529
Conversation
/review sent one sentence — 'Review the current git diff. Look for bugs ...' — into the running conversation. It had no target, no rubric, and no way to say what it had reviewed. Targets: uncommitted (the default, since that is what 'review this' usually means), base <branch>, commit <sha>, or free text. An unrecognised argument becomes instructions rather than an error, so '/review the auth changes' works. Prompts tell the reviewer how to FIND the change rather than embedding a diff: the resolved merge-base SHA to diff against, the commit to show. That is what lets the reviewer open the code around a hunk and check the callers, which is the difference between a real finding and a plausible one. Merge-base resolution prefers the base branch's upstream. On a fork 'main' is often stale while 'origin/main' is what the change will merge into, and diffing the stale one presents unrelated commits as part of the change. When git cannot answer, the prompt tells the reviewer to work the merge base out itself — a review with a vaguer instruction beats no review. The rubric is mostly exclusions, because what stops a reviewer being useful is volume rather than missing checks: only defects this change introduced, nothing speculative, no rigour the surrounding code does not already have, and 'if nothing meets the bar, report no findings'. It defers to AGENTS.md and scoped instruction files over generic advice. First slice of #524. The constrained subagent and structured findings are separate pieces.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d6f5345c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let target = review::parse_target(args); | ||
| let cwd = std::path::PathBuf::from(&engine.state().cwd); | ||
| let resolved = review::resolve(target, &cwd); | ||
| println!("Reviewing {}…", resolved.hint); |
There was a problem hiding this comment.
In the default modern TUI, slash-command stdout is captured, but the CommandResult::Prompt branch in ui/modern/run.rs enqueues only the prompt and discards the captured text. Consequently, Reviewing changes vs main… (and the equivalent commit/custom hint) is never shown to interactive users, defeating the new target feedback. Pass the hint through the prompt-result path or add it directly to the transcript rather than printing it here.
Useful? React with 👍 / 👎.
The modern TUI captured slash-command stdout but only replayed it for CommandResult::Handled, so `/review`'s "Reviewing changes vs main…" line — the only signal of which target was resolved — never reached interactive users. Emit the captured text for every outcome, before the turn is enqueued so the note precedes the turn it explains.
|
@codex review — P2 fixed in 76b341c: the modern TUI's slash bridge captured stdout but only replayed it on the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76b341c815
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ReviewTarget::BaseBranch { base } => match merge_base(cwd, base) { | ||
| Some(sha) => format!( | ||
| "Review the changes this branch adds on top of '{base}'. The merge base is \ | ||
| {sha}. Run `git diff {sha}` to see exactly what would merge into {base}, then \ |
There was a problem hiding this comment.
When /review base <branch> runs with staged or unstaged changes, git diff {sha} compares the merge base to the working tree, so the review includes local changes that would not merge with the branch and can produce findings outside the requested scope. This follows the one-commit form shown by git diff -h (git diff [<commit>]); use the two-commit form such as git diff {sha} HEAD to inspect only what the branch adds.
Useful? React with 👍 / 👎.
`git diff <merge-base>` is the one-commit form: it compares the base to the working tree, so a `/review base <branch>` run with staged or unstaged edits reviewed those too and reported findings outside the requested scope. Use `git diff <merge-base> HEAD` in both the resolved and the degraded prompt.
|
@codex review — P1 fixed in bcf5165: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcf5165e06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "commit" if !rest.is_empty() => ReviewTarget::Commit { | ||
| sha: rest.to_string(), | ||
| title: None, |
There was a problem hiding this comment.
When the argument begins with -, this accepts it as the SHA and emits an option-only command; for example, /review commit --no-patch produces git show --no-patch, which succeeds by showing HEAD without its diff, so the reviewer can inspect the wrong commit and return a false clean result. git show -h documents the command as git show [<options>] <object>..., confirming that the leading value is parsed as an option rather than an object. Resolve and validate the supplied value as exactly one commit before constructing the prompt, and report an invalid target instead of allowing Git options or additional arguments.
Useful? React with 👍 / 👎.
`git show [<options>] <object>...` parses a leading dash as an option,
so `/review commit --no-patch` became `git show --no-patch` — which
succeeds, showing HEAD with no diff. The reviewer then inspected a
commit nobody asked for and could return a confident clean result.
Reject anything that is not a single revision word, and resolve a
commit target through `git rev-parse --verify <rev>^{commit}` so an
unknown ref is an error rather than whatever git picks instead. The
merge base keeps its degraded path; a commit does not, since it is the
thing under review. `/review` reports the bad target instead of
spending a turn on it.
|
@codex review — P2 fixed in d43c21d: commit targets are now validated before anything is interpolated into a command. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d43c21d079
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| untracked. Use `git status` and `git diff` (including `--staged`) to see them, then read the \ | ||
| surrounding code as needed. Report prioritized, actionable findings."; |
There was a problem hiding this comment.
When the default review contains a newly created, untracked file, these instructions do not expose its contents: git status only lists untracked paths, while neither git diff nor git diff --staged includes files outside the index. This is consistent with git status -h describing -u as “show untracked files” and git diff -h offering only commit/blob/path comparisons. The reviewer can therefore report no findings without reading code that ReviewTarget::Uncommitted explicitly includes; instruct it to enumerate and open every untracked file (or diff each against /dev/null) as well.
Useful? React with 👍 / 👎.
The default target promises staged, unstaged *and* untracked changes, but no diff shows an untracked file: it is outside the index, so `git diff` and `git diff --staged` both skip it and `git status` prints only its path. A whole new file could come back reviewed and clean without the reviewer opening it. List them with `git ls-files --others --exclude-standard` and read each one in full.
|
@codex review — P2 fixed in 55aa688: the uncommitted prompt claimed untracked coverage that no command in it delivered — untracked files are outside the index, so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55aa688b0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| files. Untracked files appear in neither diff, so list them with `git ls-files --others \ | ||
| --exclude-standard` and read each one in full. Then read the surrounding code as needed. \ |
There was a problem hiding this comment.
When the agent's working directory is a repository subdirectory, git ls-files --others --exclude-standard is scoped to that prefix, even though git status reports untracked files elsewhere as paths such as ../other/new.rs. Following this prompt therefore omits those files and can return a clean review without reading them. Resolve the repository top level first and enumerate/read untracked paths relative to it.
Useful? React with 👍 / 👎.
| _ => String::new(), | ||
| }; | ||
| format!( | ||
| "Review the changes introduced by commit {sha}{named}. Run `git show {sha}` to \ |
There was a problem hiding this comment.
When /review commit targets a two-parent merge commit, plain git show <sha> normally prints only the commit metadata and no patch, so the reviewer is given no changes to inspect. This is reproducible with Git 2.43.0: git show <merge> shows only the merge header, while git show --first-parent <merge> shows the net diff introduced relative to the branch receiving the merge. Use the latter form, or explicitly diff <sha>^1 against <sha>.
Useful? React with 👍 / 👎.
Two ways a review could come back clean without the reviewer seeing the code: `git show <merge>` prints a combined diff, which for a routine merge is empty — a header and nothing to read. `--first-parent` gives the net diff and is a no-op on ordinary and root commits. `git ls-files` is scoped to the current prefix, so run from a subdirectory it omitted untracked files elsewhere in the repository. The `:/` pathspec covers the whole tree.
|
@codex review — both P2s fixed in e7ad124, each verified against git 2.43.0 before changing anything. Merge commits: on a real merge in this repo, Untracked files: reproduced the prefix scoping — from Regression tests: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7ad124a61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| None => (args, ""), | ||
| }; | ||
| match head { | ||
| "uncommitted" | "working" => ReviewTarget::Uncommitted, |
There was a problem hiding this comment.
When free-form instructions begin with working or uncommitted—for example, /review working on authentication—this arm discards every word after the alias and silently turns the request into an unrestricted working-tree review. This conflicts with the command's free-text fallback and can omit the user's requested focus; recognize these aliases only when rest is empty, or reject trailing arguments instead of dropping them.
Useful? React with 👍 / 👎.
`/review working on authentication` matched the bare `working` alias and discarded the rest, silently reviewing the whole tree instead of the part asked about. Match the working-tree aliases only when nothing follows; anything more falls through to the free-text path that already exists for exactly this.
|
@codex review — P2 fixed in be52032: the working-tree aliases now match only when nothing follows ( |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
First slice of #524.
Summary
/reviewsent one sentence into the running conversation:No target, no rubric, no way to say what it had reviewed.
Targets
uncommitted(the default — "review this" almost always means the working tree),base <branch>,commit <sha>, or free text. An unrecognised argument becomes instructions rather than an error, so/review the auth changesworks instead of printing usage.Prompts say how to find the change
The prompt does not embed a diff. It gives the reviewer the resolved merge-base SHA to diff against, or the commit to show. That is the whole point: the reviewer has tools, so it can open the code around a hunk, read the callers, and check whether a test exists — the difference between a real finding and a plausible one.
Merge-base resolution prefers the upstream
git merge-base HEAD main@{upstream}beforegit merge-base HEAD main. On a fork,mainis often stale whileorigin/mainis what the change will actually merge into — diffing against the stale one presents unrelated commits as if they were part of the change under review.When git cannot answer at all, the prompt tells the reviewer to work the merge base out itself rather than failing. A review that runs with a vaguer instruction beats no review.
The rubric is mostly exclusions
What stops a reviewer being useful is volume, not missing checks — speculative findings, style nits and pre-existing issues drown the two that matter. So the rubric spends its length on what not to report:
It defers to
AGENTS.mdand scoped instruction files over generic advice, and asks for[P0]–[P3]priorities plus an overall verdict.Overridable per project via
.agent/review-rubric.md(wiring for the override is in a later slice; the constant is already the single source).Verification
9 tests: target parsing including the free-text fallback and keyword-without-argument case; every prompt naming a runnable command and keeping the findings instruction; commit sha/title propagation; an unresolvable base still producing a usable prompt; a real base resolving to a concrete SHA; hint text; and
the_rubric_states_its_exclusions, which fails if the rubric loses the guidance that makes it a rubric rather than a request.cargo test --workspace --all-targetsgreen apart from the 3bwrap_*tests, which fail on this host withsetting up uid map: Permission deniedand pass in CI.clippy --all-targets -- -D warningsandfmt --checkclean.Still to come in #524
The isolated subagent with fresh history (the property that makes a review trustworthy), structured findings + JSON parsing,
POST /reviewonagent serve, and the project rubric override.