Repository navigation
fix: pin adversarial branch review context to commit revisions - #840
Linxiushen wants to merge 3 commits into
Conversation
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. |
mangeshraut712
left a comment
mangeshraut712
left a comment
There was a problem hiding this comment.
Resolving HEAD and the base once and threading the SHAs (plus currentBranch) through is a clean way to make the collected context self-consistent. The spawnSync-mocking test that switches branches and moves main mid-collection is a convincing reproduction. Two observations below.
|
|
||
| return "The repository context below is a lightweight summary. Inspect the target diff yourself with read-only git commands before finalizing findings."; | ||
| const pinnedRange = options.comparison | ||
| ? ` For this branch review, use the pinned commit range ${options.comparison.commitRange}.` |
There was a problem hiding this comment.
In lightweight/self-collect mode, pinning the range doesn't fully pin what the reviewer reads. The model will very likely also open files from the working tree for surrounding context, and after the user switches branches (exactly the scenario in the new test) the working tree holds feature/other's content, not the reviewed head's. It might be worth extending the guidance to something like: "Read file contents at the pinned head with git show <headCommit>:<path> rather than from the working tree." That would need headCommit exposed on comparison. Otherwise the findings can still mix two snapshots even though the diff itself is pinned.
| function buildBranchComparison(cwd, baseRef) { | ||
| const mergeBase = gitChecked(cwd, ["merge-base", "HEAD", baseRef]).stdout.trim(); | ||
| const headCommit = gitChecked(cwd, ["rev-parse", "--verify", "HEAD"]).stdout.trim(); | ||
| const baseRevision = gitChecked(cwd, ["rev-parse", "--verify", "--end-of-options", baseRef]).stdout.trim(); |
There was a problem hiding this comment.
Small one: for an annotated tag used as the base (e.g. --base v1.2.0), rev-parse --verify returns the tag object id, not the commit. I checked locally (git 2.47): merge-base/diff/log still peel it, so behaviour is fine. But reviewRange / the guidance text would then show a tag-object SHA, which is confusing if the model or a human pastes it into other commands. --verify --end-of-options "${baseRef}^{commit}" peels it to the commit and also rejects a ref that doesn't point at a commit. (Same consideration for HEAD, though that's always a commit.)
![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: 4ed3792704
ℹ️ 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".
| return "Use the repository context below as primary evidence."; | ||
| } | ||
|
|
||
| return "The repository context below is a lightweight summary. Inspect the target diff yourself with read-only git commands before finalizing findings."; | ||
| const pinnedRange = options.comparison |
There was a problem hiding this comment.
When a small branch diff is inlined and another process checks out a different branch after collection (especially for advertised background reviews), this early return omits both the pinned range and the warning that the working tree may have moved. executeReviewRun starts the app-server turn only after collectReviewContext, and inline content contains the diff but not full surrounding files, so any reviewer tool read of those files will come from the new branch and can support a false finding. Include the comparison.headCommit/git show guidance for branch comparisons even when includeDiff is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8151d49d92
ℹ️ 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".
| ); | ||
| includeDiff = options.includeDiff ?? (fileCount <= maxInlineFiles && diffBytes <= maxInlineDiffBytes); | ||
| details = collectBranchContext(repoRoot, target.baseRef, { includeDiff, comparison }); | ||
| details = collectBranchContext(repoRoot, target.baseRef, { includeDiff, comparison, currentBranch }); |
There was a problem hiding this comment.
When another process checks out branch B after getCurrentBranch(repoRoot) captures branch A but before buildBranchComparison resolves HEAD, comparison.headCommit and the pinned diff come from B while this call passes A into collectBranchContext. The returned context therefore says Reviewing branch A for a review of B (and persists that metadata in JSON), so background review results can be attributed to the wrong change; resolve the captured branch ref to its commit, or capture the label and OID consistently.
Useful? React with 👍 / 👎.
Import upstream openai#840, and pass --effort to native reviews (from openai#837)
/codex:adversarial-review --scope branchcollects file counts, diffs and commit logs through separate Git commands. If another process checks out a different branch or advances a ref between those calls, the resulting review context can include changes from the other branch and disagree with its initial file count.Resolve HEAD and the base ref once, then use their immutable revisions throughout branch context collection. Reuse the initial branch label and give lightweight reviews the fixed commit range to inspect. Two regression tests perform a real checkout and base-ref update between Git calls, covering inline and lightweight context. This keeps the comparison consistent after ref resolution; it does not make repository reads a transaction.
Validation on Windows with Node 24.19.0 and Git 2.55.0:
node --test tests/git.test.mjs: 12 passed; the existing broken-symlink fixture fails atfs.symlinkSyncwith WindowsEPERM.tsc -p tsconfig.app-server.json), version consistency andgit diff --checkpassed. The TypeScript check used locally generated app-server types.AI assistance was used for implementation, reproduction and independent review.