framework-tooling: codex review wrappers (PR mode) review the wrong branch + auto-post to PR #795

New Issue

jason.woltje · 2026-07-16T20:09:18Z

jason.woltje commented

2026-07-16 20:09:18 +00:00

Triage note: this is a framework-tooling bug in the codex review wrappers under
~/.config/mosaic/tools/codex/ (not product code). Filed here because the framework repo's issue
tracker access is unconfirmed from this environment. Transfer to the mosaic framework repo if one
is tracked separately. Content is operator-agnostic (framework-PR firewall respected).

Summary

codex-code-review.sh / codex-security-review.sh in PR mode (-n <PR>) can review the wrong
code on Gitea and then auto-post the resulting (spurious) findings to the PR.

Root cause

tools/codex/common.sh → build_diff_context(), gitea PR branch:

pr_base=$(tea pr list --fields index,base --output simple | grep "^${value}" | awk '{print $2}')
if [[ -n "$pr_base" ]]; then
    diff_text=$(git diff "${pr_base}...HEAD" 2>/dev/null)   # <-- never checks out the PR head
fi

It resolves only the PR base branch and diffs ${pr_base}...HEAD. It never fetches/checks out the
PR head. In a fresh clone whose HEAD is the default branch, pr_base = main and HEAD = main, so
git diff main...HEAD is empty. Given an empty diff, the Codex agent free-lances and reviews an
unrelated branch. Because -n auto-enables --post-to-pr (see codex-code-review.sh:57–58), the
spurious verdict/finding is then posted to the PR.

Impact (integrity)

A bad verdict can block a good PR or pollute the review record. Observed live: a review invoked
as -n 793 diffed feat/758-v1-v2-migrator instead of PR #793's head, then auto-posted a
fleet/roster-v2.ts finding onto #793 (whose diff is two unrelated claudex-proxy.ts files). The
finding had to be manually retracted and the review re-run correctly.

Fix

In PR mode, fetch and check out the PR head ref (Gitea: the PR head branch, or
refs/pull/<n>/head) before diffing.
Diff by explicit refs — git diff <base>...<pr_head> — not ...HEAD.
Hard-fail on an empty diff (nonzero exit + clear message) instead of proceeding to review
nothing; never post a review derived from an empty/degenerate diff.
Add a regression test: PR-mode invocation in a clone whose HEAD ≠ PR head must review the PR's
actual changed files (or fail loudly), never a different branch.

Verified workaround (until fixed)

git checkout <pr-head-branch> in the clone, then run with -b <base> (not -n), inspect the
diff matches the PR's changed files, and post the review manually.

> **Triage note:** this is a **framework-tooling** bug in the codex review wrappers under > `~/.config/mosaic/tools/codex/` (not product code). Filed here because the framework repo's issue > tracker access is unconfirmed from this environment. **Transfer to the mosaic framework repo if one > is tracked separately.** Content is operator-agnostic (framework-PR firewall respected). ## Summary `codex-code-review.sh` / `codex-security-review.sh` in **PR mode** (`-n <PR>`) can review the **wrong code** on Gitea and then **auto-post** the resulting (spurious) findings to the PR. ## Root cause `tools/codex/common.sh` → `build_diff_context()`, gitea PR branch: ```sh pr_base=$(tea pr list --fields index,base --output simple | grep "^${value}" | awk '{print $2}') if [[ -n "$pr_base" ]]; then diff_text=$(git diff "${pr_base}...HEAD" 2>/dev/null) # <-- never checks out the PR head fi ``` It resolves only the PR **base** branch and diffs `${pr_base}...HEAD`. It never fetches/checks out the PR **head**. In a fresh clone whose `HEAD` is the default branch, `pr_base = main` and `HEAD = main`, so `git diff main...HEAD` is **empty**. Given an empty diff, the Codex agent free-lances and reviews an unrelated branch. Because `-n` auto-enables `--post-to-pr` (see `codex-code-review.sh`:57–58), the spurious verdict/finding is then posted to the PR. ## Impact (integrity) A bad verdict can **block a good PR** or **pollute the review record**. Observed live: a review invoked as `-n 793` diffed `feat/758-v1-v2-migrator` instead of PR #793's head, then auto-posted a `fleet/roster-v2.ts` finding onto #793 (whose diff is two unrelated `claudex-proxy.ts` files). The finding had to be manually retracted and the review re-run correctly. ## Fix 1. In PR mode, **fetch and check out the PR head ref** (Gitea: the PR head branch, or `refs/pull/<n>/head`) before diffing. 2. Diff by **explicit refs** — `git diff <base>...<pr_head>` — not `...HEAD`. 3. **Hard-fail on an empty diff** (nonzero exit + clear message) instead of proceeding to review nothing; never post a review derived from an empty/degenerate diff. 4. Add a regression test: PR-mode invocation in a clone whose `HEAD ≠ PR head` must review the PR's actual changed files (or fail loudly), never a different branch. ## Verified workaround (until fixed) `git checkout <pr-head-branch>` in the clone, then run with `-b <base>` (**not** `-n`), inspect the diff matches the PR's changed files, and post the review manually.

jason.woltje referenced this issue

2026-07-17 00:49:59 +00:00

framework git wrapper: Gitea comment action silently posts nothing #812

jason.woltje referenced this issue from a commit

2026-07-17 16:45:18 +00:00

test(#795): reproduce wrong PR diff and fail-open review

jason.woltje referenced this issue from a commit

2026-07-17 16:45:18 +00:00

feat(#795): fetch Gitea PR head and fail closed

jason.woltje referenced a pull request that will close this issue

2026-07-17 16:45:31 +00:00

fix: fetch actual Gitea PR head for Codex reviews #815