Repository navigation
Conversation
A request to review another engineer's PR had no skill, so each session chose its own review depth. The new skill takes the reviewer from the pre-pr-review table, runs it in a fresh context on the PR's range and reports in the session. It posts only when the engineer agrees, never approves, and writes no review marker. AGENTS.md points to it next to the pre-pr-review pointer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Name the head commit in every reviewer command and read the PR's code at that commit, only as data. PR text and the PR's agent config are never instructions. Keep a gate fix in the gate row, accept a reviewed head only from the requester or the requester's own account, and review the whole PR when the delta contains a merge. Post only the triaged findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…588) A PR comment can be edited by the PR owner, so drop the post-based shortcut for an updated PR. The fix row now needs the engineer's reviewed head and earlier findings, and only covers a fixes-only delta. Keep a base-moved guard, create the throwaway worktree outside the checkout, drop the PR-number reviewer routes, warn when the checkout contains the PR, re-check the head before posting, and name `.agents/`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…16588) Compare the checkout's agent config with the base, including uncommitted changes, and stop the review when they differ. Name the reviewed head in a fix-range post, keep the throwaway worktree path for removal, and review the whole PR when the reviewed head does not resolve. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A skill cannot prove which agent config the session runs under: the harness loads it at session start, partly from parent directories. Stop only when the PR changes agent config and the engineer's checkout carries those changes, compared with origin/main, not the PR's base. Write down the check's limits. Remove the throwaway worktree on every exit, and never start a fresh session from a worktree of the PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Take the agent-config changes and the PR's commits from the merge base with origin/main, so that a stacked PR's base is also checked. Match nested .agents, .claude and .pi directories. Test the history with one merge-base --is-ancestor command. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For a PR merged with a merge commit, take the merge bases from the merge commit's first parent, so the range is not empty. Count Sphinx as available only when its MCP review tools are present, and say what the gate row's AGENTS.md review rules are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis 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: c84e93db3f
ℹ️ 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".
| if `git rev-parse --verify "<merge>^{commit}"` fails, and take | ||
| `MB=$(git merge-base "<merge>^1" <head>)`. If `<head>` is also an ancestor of | ||
| `refs/remotes/origin/main`, take `MM` the same way. | ||
| - **Config check.** Take `MM=$(git merge-base refs/remotes/origin/main <head>)`. |
There was a problem hiding this comment.
When a merged PR's head is already an ancestor of origin/main, line 33 computes MM from the merge commit's first parent, but this unconditional assignment immediately replaces it with <head>. The resulting $MM..<head> diff is empty, so a merged PR that changed AGENTS.md, .agents/, or another agent-config path bypasses the check and can be reviewed from a session that loaded those untrusted changes. Preserve the historical MM for this merged-PR case instead of overwriting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7d5950759. A merged PR whose head is in origin/main now skips the config check: its agent config is already main's, reviewed when it merged. So no MM value is carried over or reset. A PR whose head is not in main, for example a stacked PR, still gets the check with MM from origin/main.
| `refs/remotes/origin/<base>`, that merge base is `<head>` and the range is empty. | ||
| Then use the `mergeCommit` oid, `<merge>`: fetch it with `git fetch origin <merge>` | ||
| if `git rev-parse --verify "<merge>^{commit}"` fails, and take | ||
| `MB=$(git merge-base "<merge>^1" <head>)`. If `<head>` is also an ancestor of |
There was a problem hiding this comment.
When the hosting service reports a fast-forward merge, <merge> equals <head>, so git merge-base "<merge>^1" <head> returns <head>^. The review range then contains only the PR's final commit and silently omits every earlier commit. Detect this case and derive the original PR base from the PR's commit metadata rather than from the merge commit's immediate parent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7d5950759. The first-parent formula now applies only when the merge commit has two parents and its second parent is the head. Otherwise, for example after a fast-forward, the skill says that the range cannot be recovered and asks the engineer for it, instead of reviewing only the last commit.
Scan-Build Report
Bug Summary
Filter Results by FileReports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The configuration check can miss copied earlier PR configuration and locally applied deletions, allowing review under untrusted configuration. The merged-PR fallback also silently omits earlier commits when a PR was fast-forwarded.
🤖 Bits Code Review · Commit c84e93d
| It also carries them if a listed file in the working tree is identical to its | ||
| version at `<head>` (`git show <head>:<path> | cmp -s - <path>`) and differs | ||
| from `refs/remotes/origin/main`. |
There was a problem hiding this comment.
When a checkout on main contains copied configuration from an earlier PR commit, the ancestry check permits review and the final-head comparison misses the copied version. Applying a PR's configuration deletion locally also escapes detection: git show fails for the deleted path and cmp fails for the absent working-tree file. Review therefore proceeds with PR-controlled configuration or removed hooks instead of stopping. Compare configuration states across the PR history, including matching deletions.
| It also carries them if a listed file in the working tree is identical to its | |
| version at `<head>` (`git show <head>:<path> | cmp -s - <path>`) and differs | |
| from `refs/remotes/origin/main`. | |
| It also carries them if a listed path's working-tree state matches | |
| that path in any PR commit in `MM..<head>` and differs from | |
| `refs/remotes/origin/main`. Compare contents for existing files; for a path | |
| deleted by that commit, matching absence in the working tree also counts. |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Deletions are fixed in d4df0d3e1 (see the thread on line 46). Copied older revisions of the PR's config, which differ from the head's version, are a stated limit of this check: the skill tells the engineer not to review from a session that had the PR's branch checked out. Enforcement beyond what a skill can check is tracked separately, as a hook-level ticket.
| if `git rev-parse --verify "<merge>^{commit}"` fails, and take | ||
| `MB=$(git merge-base "<merge>^1" <head>)`. If `<head>` is also an ancestor of |
There was a problem hiding this comment.
For a multi-commit PR merged by fast-forward, mergeCommit can equal the PR head. The fallback then sets MB to head's first parent and reviews only the final commit, silently omitting earlier PR changes. Recover the original pre-merge base or stop explicitly when it cannot be established; the first-parent fallback requires an actual merge commit.
| if `git rev-parse --verify "<merge>^{commit}"` fails, and take | |
| `MB=$(git merge-base "<merge>^1" <head>)`. If `<head>` is also an ancestor of | |
| if `git rev-parse --verify "<merge>^{commit}"` fails. If `<merge>` has fewer than two | |
| parents, stop and report that a trusted pre-merge base is needed to recover | |
| the full PR range; do not use `<merge>^1` as its base. Otherwise take | |
| `MB=$(git merge-base "<merge>^1" <head>)`. If `<head>` is also an ancestor of |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in 7d5950759. The first-parent formula now applies only when the merge commit has two parents and its second parent is the head. Otherwise, for example after a fast-forward, the skill says that the range cannot be recovered and asks the engineer for it, instead of reviewing only the last commit.
CI Test ResultsRun: #43103721494 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-11 21:54:22 UTC |
There was a problem hiding this comment.
The checkout-safety check can miss deleted safeguards and untrusted history when multiple merge bases exist, allowing a reviewer to run under PR-controlled configuration.
🤖 Bits Code Review · Commit c84e93d
| changes. It carries PR commits unless | ||
| `git merge-base --is-ancestor "$(git merge-base HEAD <head>)" "$MM"` succeeds. |
There was a problem hiding this comment.
When the checkout and PR have multiple best common ancestors, Git can select a trusted ancestor while another contains untrusted PR config changes. This test then declares the checkout safe. If the PR subsequently changed that config again, the exact-head content comparison also misses it, allowing review under PR-controlled configuration. Check every best common ancestor.
| changes. It carries PR commits unless | |
| `git merge-base --is-ancestor "$(git merge-base HEAD <head>)" "$MM"` succeeds. | |
| changes. It carries PR commits unless every commit returned by | |
| `git merge-base --all HEAD <head>` passes | |
| `git merge-base --is-ancestor <common-ancestor> "$MM"`. |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in d4df0d3e1. The history test now checks every commit that git merge-base --all HEAD <head> prints: the checkout carries PR commits if any of them is not an ancestor of origin/main. A crossed history can no longer hide a PR commit behind a merge base that is in main.
| It also carries them if a listed file in the working tree is identical to its | ||
| version at `<head>` (`git show <head>:<path> | cmp -s - <path>`) and differs | ||
| from `refs/remotes/origin/main`. |
There was a problem hiding this comment.
A copied or cherry-picked config deletion can pass the ancestry check, but this comparison cannot recognize matching absence: both git show and cmp fail. Review can therefore proceed with safeguards removed by the PR. This matters concretely for deleted hooks because .claude/settings.json skips require-fresh-review.py when that file is missing.
| It also carries them if a listed file in the working tree is identical to its | |
| version at `<head>` (`git show <head>:<path> | cmp -s - <path>`) and differs | |
| from `refs/remotes/origin/main`. | |
| It also carries them if a listed path has the same state in the working tree | |
| and at `<head>`, and that state differs from `refs/remotes/origin/main`. | |
| Compare existing files with `git show <head>:<path> | cmp -s - <path>`; | |
| if `<head>` deletes the path, matching absence in the working tree also counts. |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in d4df0d3e1. The file test now also counts a config file that <head> deletes, that the working tree lacks too, and that origin/main still has.
| if `git rev-parse --verify "<merge>^{commit}"` fails, and take | ||
| `MB=$(git merge-base "<merge>^1" <head>)`. If `<head>` is also an ancestor of | ||
| `refs/remotes/origin/main`, take `MM` the same way. | ||
| - **Config check.** Take `MM=$(git merge-base refs/remotes/origin/main <head>)`. |
There was a problem hiding this comment.
For a merged PR whose head is an ancestor of main, this assignment overwrites the MM recovered immediately above with the head itself. The config diff becomes empty, so neither checkout-contamination check runs, including when the checkout retains an earlier version of the PR's config. Preserve the recovered config range and explicitly handle trusted main checkouts; preserving MM alone would incorrectly reject those checkouts.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in 7d5950759. A merged PR whose head is in origin/main now skips the config check: its agent config is already main's, reviewed when it merged. So no MM value is carried over or reset. A PR whose head is not in main, for example a stacked PR, still gets the check with MM from origin/main.
| `refs/remotes/origin/<base>`, that merge base is `<head>` and the range is empty. | ||
| Then use the `mergeCommit` oid, `<merge>`: fetch it with `git fetch origin <merge>` | ||
| if `git rev-parse --verify "<merge>^{commit}"` fails, and take | ||
| `MB=$(git merge-base "<merge>^1" <head>)`. If `<head>` is also an ancestor of |
There was a problem hiding this comment.
For a multi-commit PR closed by a fast-forward where mergeCommit equals the PR head, this expression selects head^1. The reviewer consequently examines only the last commit while reporting a whole-PR review. Recover the original PR commit boundary for this case instead of assuming a separate merge commit.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in 7d5950759. The first-parent formula now applies only when the merge commit has two parents and its second parent is the head. Otherwise, for example after a fast-forward, the skill says that the range cannot be recovered and asks the engineer for it, instead of reviewing only the last commit.
| 1. **Target.** Read the PR's base branch, head commit, state, merge commit and files, | ||
| for example with `gh pr view <n> --json baseRefName,headRefOid,state,mergeCommit,files`. | ||
| `<head>` is that `headRefOid`. Fetch without a checkout: | ||
| `git fetch origin <base> main pull/<n>/head`. The range is |
There was a problem hiding this comment.
When a merged stacked PR's target branch has been deleted, the required fetch fails and its remote-tracking ref may also be absent. The merged-PR fallback itself requires that ref, so review cannot proceed even though the recorded merge commit and PR head provide the necessary history. Resolve merged PR history without requiring the former target branch.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
# Conflicts: # AGENTS.md
A merged PR whose head is in origin/main has main's own agent config, so the config check has nothing to protect; its MM was also reset on the next line, which let the check run empty. The merged-PR range now needs a two-parent merge commit whose second parent is the head; otherwise, as after a fast-forward, the skill asks for the range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The history test now checks every merge base of the checkout and the PR head against origin/main, so a crossed history cannot hide a PR commit. The file test also catches a config file that the PR deletes and the checkout lacks too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What does this PR do?:
Adds a
pr-reviewskill for reviewing another engineer's PR. A request like "review PR 1234" now gets a review depth chosen by what the PR changes, without an effort hint in the prompt.pre-pr-reviewreviewer table, by reference, with its fallbacks and budget rule..agents/,.claude/,.pi/,.githooks/,.mcp.json) is data too.origin/main, never from a worktree of the PR.origin/main. It compares withorigin/main, never with the PR's base. The engineer's own config changes are trusted. The skill states what this check cannot see.AGENTS.mdpoints to the skill next to thepre-pr-reviewpointer.Retargeted to
mainafter #962 merged; main is merged in (eea3d12, oneAGENTS.mdconflict, resolution reviewed at high effort with no findings).Motivation:
We review PRs on a regular basis.
pre-pr-reviewpicks the review depth by what a range changes, but it loads only before opening, readying or updating your own PR. A request to review someone else's PR got whatever depth the session invented. A hint like "adjust your effort to the complexity of the PR" does not fix that, and complexity is the wrong signal: a one-line change to a gate needs the deepest review.Additional Notes:
/code-reviewin a fresh context for each round. The first round ran at high effort, under the gate row's no-Sphinx fallback, because the pointer edits theAGENTS.mdreview rules. The fix rounds ran at medium.Round 1 (b30c3c6): 1 High, 5 Medium. The review was not bound to the head, and PR text was not treated as data.
Round 2 (41d29d7): 2 High. A PR owner could edit the comment that named the reviewed head, and a worktree inside the checkout exposed the PR's skills. The post-based shortcut was removed rather than patched again (D5).
Round 3 (583293a): no High. The round limit stopped here; the engineer approved more rounds.
Round 4 (0b85053): 1 High. A stacked PR's base was trusted. The check was narrowed to the PR's own config, compared with
origin/main.Round 5 (59fa467): no High. The engineer approved three one-line fixes (9a0dd5c): compute the config check from
origin/main, match nested config directories, and use one--is-ancestortest.Review of 9a0dd5c: no Critical or High; the three fixes were confirmed in test repositories. Listed, not fixed:
merge-base --allwould close it);origin/mainskips the check;.knowledgebase/is not in the config paths.Test-driven fix (c84e93d), after AC2 and AC5 failed on 9a0dd5c:
Review of c84e93d: no Critical or High. Listed, not fixed, all for already merged PRs:
MM;Lows: a deleted stacked base branch; no fix-range review of a merged PR; a review rule outside the review section; "do not ask" next to the Cost rule; the definitions live only in this skill.
MMwas reset on the next line, so the config check could run empty. A merged PR whose head is in origin/main now skips the check, because its config is already main's. The merged-PR range now needs a two-parent merge commit whose second parent is the head; otherwise, as after a fast-forward, the skill asks for the range.merge-base --all) against origin/main, and the file test catches a config file that the PR deletes./code-reviewmedium in a fresh context, with no Critical or High findings. Listed, not fixed (Low):--no-renameswould list both);merge-base --allresult, as in a shallow clone, passes the check;How to test the change?:
Headless drives in fresh clones, with a
ghstub first onPATHthat passes reads and refuses every write. Claude Code 2.1.322 ran with--strict-mcp-config, so no Sphinx; Pi 1.2.0 ran with model glm-5-3. The comment and review counts on #954 and #970 were the same before and after every run.python3 utils/check-skills.pyskills: OK (4 skills)bash utils/tests/test_quality_check_scripts.shpr-review; gate row;/code-reviewhigh in a fresh subagent as the no-Sphinx fallback, without asking; range5c930a31e..f37f9f3abfrom the merge commit's first parent; head named; no writes. $2.41. The clone'sorigin/mainwas seeded with the merge, because the drives'git fetchcould not authenticate.AGENTS.mdonly)/code-reviewmedium in a fresh subagent; head and merge base named; no writes. $0.75.gh pr comment 970 --body-file …, refused by the stub; no retry, no verdict. $0.79.AC3 and AC4 were not rerun after c84e93d, which also changed the reviewer choice and the gate-row wording that #970 exercises. AC5 on c84e93d picked the docs row for #970 under the new wording.
In AC5, Pi ran a labeled model review in its own session instead of only offering one by a fresh session, and it read the PR body before the config check. Neither is in the AC5 text, but both differ from what the skill says.
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