feat(hooks): review per-commit + pre-PR gate, replacing pre-push review#3056
Open
mmagician wants to merge 6 commits into
Open
feat(hooks): review per-commit + pre-PR gate, replacing pre-push review#3056mmagician wants to merge 6 commits into
mmagician wants to merge 6 commits into
Conversation
Reviewers now run as a PostToolUse hook on `git commit` over the commit's
own delta (HEAD~1..HEAD) and a PreToolUse gate on `gh pr create` over the
whole PR. Reviewing per commit makes the diff scope trivially the commit
itself, eliminating the base-selection logic (merge-base / @{upstream}
tracking, "don't flag pre-existing code" prompt rules) that pre-push
review needed to review each change once.
- _review.py: shared orchestrator (run both reviewers in parallel over a
given diff range, parse severity, return blocked + rendered output).
- post_commit_review.py: PostToolUse on git commit; covers normal/-a/
--amend/rebase uniformly via HEAD~1..HEAD; exit 2 halts the agent which
fixes and amends to re-review.
- pre_pr_review.py: PreToolUse on gh pr create; whole-PR review against
origin/HEAD merge-base; denies PR creation on findings.
- Delete pre_push_review.py; push keeps only its test gate.
- Simplify reviewer prompts to diff the range named in the prompt.
- Update settings.json and hook routing tests; add parser + base tests.
Supersedes #3048.
mmagician
added
the
no changelog
Reviewing in context shouldn't mandate reading the whole file every time - what matters is the relevant and related code (callers, callees, the rest of the file where relevant), which may be the whole file or just a few chunks.
The reviewers run as fresh subprocesses with no view of the conversation, so they sometimes contradict choices the user explicitly asked for. Extract the human prompts from the hook payload's transcript_path and pass them to both reviewers, with a rule to respect deliberate, requested choices (real correctness/security bugs still block). - _hookutils.recent_user_prompts: parse the JSONL transcript, keep human turns (drop tool results, tool calls, and <system-reminder> noise), most-recent-first and length-capped. - _review.run_review gains an `intent` arg folded into the reviewer prompt. - post_commit_review / pre_pr_review pass the extracted intent. - code-reviewer / security-reviewer prompts gain a respect-intent rule.
- Drop the recent_user_prompts unit tests (and unused import). - Align the agent prompts with the hook's severity policy: Critical/ Important (code) and Critical/Warning (security) block; Nits/Notes are surfaced but never block. Removes the contradiction where the prompts claimed "all findings block" while the hook only blocks the top tiers, and fixes the BLOCK/APPROVE/CLEAN verdicts to match. - Stop mixing "Nit/Note": each agent uses its own lowest tier (Nit for code-reviewer, Note for security-reviewer); the shared run_review prompt is now convention-neutral and defers severity to each agent's rules. - Stricter intent handling: a genuinely exploitable/real defect a user requested stays blocking; only downgrade to a Nit/Note when the objection is defensive-programming hardening or a non-critical weakness.
partylikeits1983
approved these changes
Jun 10, 2026
partylikeits1983
left a comment
partylikeits1983
left a comment
Contributor
There was a problem hiding this comment.
Looks good, but was wondering if we could make _review.py and _hookutils.py not have a prepended _.
Comment on lines
-125
to
+123
| **All findings (Critical, Important, and Nit) block the merge.** Every issue must be addressed before pushing. | ||
| **Critical and Important findings block the merge; Nits are surfaced but do not block.** Address the blocking findings before pushing. |
Comment on lines
+1
to
+2
| """Shared reviewer orchestration for the review hooks. | ||
|
|
Contributor
There was a problem hiding this comment.
why does this file name have a prepended _ and the others don't?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Spawn review agents after each commit + before creating a PR draft, instead of pre-push.
This has two effects:
HEAD~1..HEAD), but we still run the pre-draft hook on the full PR as some changes might only be relevant within the broader PR contextThe pre-draft hook will only be relevant when the engineer uses a
/workcommand (see #3057) which goes full cycle towards opening a PR.Other commands might need to be added alongside
/workfor local-first workflows