Repository navigation
PROF-16456: Enforce the pre-PR agentic review with a skill, marker script and hooks - #935
Conversation
utils/review-marker.py records that a review covered a branch and checks it later. The marker certifies commits: write refuses uncommitted tracked changes and stores the verbatim patch-id of the diff from the merge base to HEAD. An advancing base or a clean rebase keeps it fresh unless the base changed lines next to the branch's hunks; context lines anchor each hunk's position, so that case errs toward stale. Any change to the branch's diff, whitespace or position included, makes it stale. Markers live per branch, named by hash, in the git common dir, so every worktree sees them and they are never committed. git 2.39+ is required and checked. install-hook writes a pre-push shim into the common hooks dir instead of setting core.hooksPath: a repo-local hooksPath would replace global hooks, such as Datadog's secret scanner, which chain to .git/hooks. PROF-16456 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The skill lives in .agents/skills/ for harnesses that read that path; .claude/skills is a symlink to it. It installs the pre-push shim, runs the available reviewer, adds the comment-budget and plan/spec-reference checks, and writes the marker. .githooks/pre-push refuses to update a branch with an open PR unless the pushed commits match its marker. It reads the real refs from git, so it covers every harness, human pushes and HEAD or sha refspecs. It runs the local check first and asks gh about open PRs only when that fails, and fails open when gh cannot answer. The Claude Code PreToolUse hook covers what pre-push cannot see: gh pr create/new/ready. It matches the raw command text, with backslash continuations joined, instead of parsing shell syntax, so subshells, bash -c and multi-line commands cannot hide the call; the accepted cost is a false block when the text is only quoted while the marker is stale. Where the text cannot pin the target down it blocks: --head, conflicting bases, anything that may switch checkouts (cd, pushd, env -C/--chdir, GIT_DIR), and git steps that move HEAD in the same command, since the hook runs before the command. gh pr ready is checked against the PR's head commit on GitHub. The settings entry exits 0 when the hook file is missing, so an older checkout cannot block every call. PROF-16456 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ate" The comment budget needs judgment, so no tool enforces it; the agentic review checks it instead. The review section keeps the obligation, now also covering PRs marked ready and fixes pushed to an open PR, and points to the pre-pr-review skill, review-marker.py and the hooks. The inline-vs-doc rule now sends procedures to skills and enforcement to scripts, hooks and CI. PROF-16456 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Scan-Build Report
Bug Summary
Reports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
| when the work is stacked. Commit the change set first — the marker certifies | ||
| commits, so uncommitted work is not covered. | ||
|
|
||
| 2. **Review.** Use the strongest reviewer the harness provides: `/sphinx review`, |
There was a problem hiding this comment.
Since the step 4 will do the triage and fixes, we should use /sphinx review --report-json (or /sphinx review --report-json <path> to store the findings in a file) and skip the built-in auto-fix loop.
There was a problem hiding this comment.
Agreed, done in 79b1c19. Step 2 now runs report-only, with no reviewer fix flow. Steps 3 and 4 do the triage and fixes, so there is one fix loop. The review of this change also showed that the range has to be named explicitly: without one, sphinx reviews only unpushed changes, and the marker would certify code nobody reviewed. So the step now reads /sphinx review range $(git merge-base <base> HEAD)..$(git rev-parse HEAD) --report-json <path>, with /code-review given the same range as fallback.
There was a problem hiding this comment.
Hm, I will check it. It should look at 'branch changes' as well, but there might be some issue there - I had some before, thanks to the worktrees etc.
The pre-push shim ran the checkout's .githooks/pre-push, which loaded the checkout's review-marker.py, and the Claude Code hook preferred the checkout's script too. Checking out an untrusted branch, such as a fork PR, and then pushing or running gh pr create would execute that branch's code with the user's credentials; git itself never runs versioned hooks for this reason. The shim, pre-push and the Claude Code hook now take their code from refs/remotes/origin/main via git show and run it with python3 -I -c. The ref is fully qualified so a tag named origin/main cannot substitute it, and -I keeps the worktree root off sys.path so stdlib-named files planted there are not imported. The skill runs install-hook and write the same way, so markers are written by the version that checks them. When the ref lacks the scripts, the hooks allow everything. Claude Code's own loading of .claude/ hooks from the session's checkout is out of scope. PROF-16456 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Name the reviewed range explicitly, merge-base to HEAD: without a range, sphinx reviews only unpushed changes and /code-review only recent ones, so the marker would certify code nobody reviewed. Run /sphinx review range with --report-json and no reviewer fix flow; steps 3 and 4 already triage and fix. PROF-16456 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
.pi/extensions/require-fresh-review.ts runs the same hook from Pi's tool_call event: it passes the bash command as the hook's JSON payload and blocks on exit 2. Pi already discovers the skill in .agents/skills. Its tests drive the extension through a stub Pi API under node. New test cases pin the git 2.39 version boundary, tag pushes never consulting gh, an explicit --base winning over the default branch, the -H short form, and -R/--repo being forwarded to gh pr view. PROF-16456 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI Test ResultsRun: #41869045727 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 31 | Failed: 0 Updated: 2026-11-04 17:30:14 UTC |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: bc7d270eeb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two of the most critical issues are: the patch-id digest does not encode hunk location, so moving a reviewed edit between identically-contexted blocks leaves the marker falsely fresh; and the hook ignores the branch-level gh-merge-base config, validating against the wrong base for stacked or release-targeted branches. Additional distinct issues affect repository-selector handling in gh pr ready, stale base resolution after PR retargeting, remote-head-to-local-branch mapping, unqualified merge-base ref resolution, and ambiguous branch-name shortening when a tag shadows a branch.
🤖 Bits Code Review · Commit bc7d270
- gh pr create without --base targets branch.<name>.gh-merge-base before the repo default; the hook now checks that base, without a network call. - The repo selector of gh pr ready is read from the whole command, so `gh -R o/r pr ready N` and `--repo=o/r` keep their repository. - The digest's merge base uses refs/remotes/origin/<branch>, and so does the skill's reviewed range, so a tag named origin/<branch> cannot shift either of them. - Branch names come from the full symbolic ref: next to a same-named tag, --short yields "heads/<branch>" and the marker landed under that name. utils/README.md lists the accepted limits: edits moved between identical contexts keep the digest, pre-push does not see a retargeted PR base, and gh pr ready does not map a renamed push back to its local branch. PROF-16456 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/150158477 |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
6aa5253
into
main
What does this PR do?:
Turns the agentic pre-PR review from prose in AGENTS.md into a procedure that tools carry out and enforce.
.agents/skills/pre-pr-review/SKILL.md(.claude/skillssymlinks to.agents/skills)utils/review-marker.pyinstall-hookinstalls the push gate..githooks/pre-push.claude/hooks/require-fresh-review.pyPreToolUsehook for what pre-push cannot see:gh pr create/new/ready..pi/extensions/require-fresh-review.tstool_callevent and blocks on its exit 2. Pi already finds the skill in.agents/skills/.AGENTS.mdMotivation:
Additional Notes:
Design decisions, flagged for reviewer acknowledgment:
writerefuses uncommitted tracked changes and stores thegit patch-id --verbatimofmerge-base..HEAD. Context lines anchor hunk positions, so an upstream change right next to the branch's hunks errs toward stale. That forces a re-review rather than letting unreviewed code through. Requires git 2.39+.<git-common-dir>/hooks/, never throughcore.hooksPath. A repo-localcore.hooksPathwould replace a global one. On Datadog machines that disables thedd-git-hookssecret scanner. The global wrapper chains to.git/hooks, which I verified for both the main checkout and worktrees.bash -c, multi-line commands and continuations. The accepted cost is a false block when the text is only quoted while the marker is stale. Where the target is ambiguous, it blocks:--head, conflicting bases,cd/pushd/env -C/GIT_DIR, and git steps that move HEAD in the same command.pre-pushand the Claude Code hook load their code fromrefs/remotes/origin/mainand run it withpython3 -I. Checking out an untrusted branch, such as a fork PR, and then pushing therefore runs only reviewed code. The ref is fully qualified, so a tag namedorigin/maincannot substitute it.-Ikeeps planted stdlib-named files at the worktree root from being imported. The skill runsinstall-hookandwritethe same way, so markers are written by the version that checks them.main. Both hooks allow everything whenrefs/remotes/origin/mainlacks the scripts. That also covers clones without anoriginremote. Claude Code's own loading of.claude/hooks from the session's checkout falls under its workspace-trust model and is out of scope.install-hook), the PR-command hook covers only Claude Code and Pi (Pi loads project extensions only after project trust), and CI cannot see the local marker. The ticket's production check covers the first real use.Agentic review: seven
/code-review(high) passes, then two sphinx range reviews: one over the full change set (1671c13..79b1c19) and one over the Pi/test commit (79b1c19..bc7d270). Neither found Critical/High; the test gaps the first one flagged are covered. The bot findings raised on ready-for-review (gh-merge-base, qualified digest base, tag-safe branch names, repo selectors) are fixed in09055deand were reviewed with/code-review, whose one High (the skill's unqualified merge-base) is fixed in the same commit. Every Critical/High finding is fixed. These Medium/Low findings are open for reviewer judgment:gh pr createchecks the local tip, while the PR opens from the remote branch, which can differ.writedoes not normalize--base, so--base mainnever matches the hook'sorigin/main.git show(for example, a clone whose main remote isupstream), and it is silent.origin/mainrestore it on the next line instead of in a trap.--report-jsonpath is not pinned to a gitignored location such as.sphinx/.--sessionhas no defined source in report-only mode.test_review_marker.shruns only by hand, like the otherutils/testsscripts.--experimental-strip-types); on older Node they fail instead of skipping.-Ris read from the whole matched text, which can start at an earlierghcommand on the same line (gh pr view -R x 34 && gh pr ready 12).gh pr create -R other/repowithout--baseis checked against the checkout repo's default branch.origin/bases are qualified; a base on another remote (upstream/main) can still be shadowed by a same-named tag.-Rtakes the first value (gh takes the last);-Belsewhere on the line overridesgh-merge-base; agh-merge-basegiven as a full ref is not normalized; no tests for-Rxor the detached-HEAD fallback.has_open_prignores which remote is pushed to.gh pr readydoesn't parse the--repo=form.How to test the change?:
bash utils/tests/test_review_marker.sh(136 hermetic cases, including the Pi adapter driven through a stub Pi API undernode; each in a throwaway repo with a bare origin and aghstub; global git config is ignored).origin/mainsourcing,-I, the qualified ref) fails at least one test. The trust-model tests plant payloads in the checkout (.githooks/pre-push,utils/review-marker.py,argparse.py,subprocess.py) and a tag namedorigin/main, then assert that no payload ran.utils/check-tracked-artifacts.py,utils/check-doc-index.py.pre-pr-reviewshould appear in the skill list (through the.claude/skillssymlink).For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.🤖 Generated with Claude Code